* [PATCH] http: cast result to FILE * @ 2019-02-01 19:30 Dan McGregor 2019-02-01 21:17 ` Junio C Hamano 2019-02-09 2:36 ` [PATCH v2] git-compat-util: undefine fileno if defined Dan McGregor 0 siblings, 2 replies; 16+ messages in thread From: Dan McGregor @ 2019-02-01 19:30 UTC (permalink / raw) To: git; +Cc: masayasuzuki, Dan McGregor Commit 8dd2e88a92 ("http: support file handles for HTTP_KEEP_ERROR", 2019-01-10) introduced an implicit assumption that rewind, fileno, and fflush are functions. At least on FreeBSD fileno is not, and as such passing a void * failed. Explicitly cast result to a FILE * when using standard functions that may ultimately be macros. Signed-off-by: Dan McGregor <dan.mcgregor@usask.ca> --- http.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/http.c b/http.c index 954bebf684..8b9476b151 100644 --- a/http.c +++ b/http.c @@ -1996,12 +1996,12 @@ static int http_request_reauth(const char *url, strbuf_reset(result); break; case HTTP_REQUEST_FILE: - if (fflush(result)) { + if (fflush((FILE *)result)) { error_errno("unable to flush a file"); return HTTP_START_FAILED; } - rewind(result); - if (ftruncate(fileno(result), 0) < 0) { + rewind((FILE *)result); + if (ftruncate(fileno((FILE *)result), 0) < 0) { error_errno("unable to truncate a file"); return HTTP_START_FAILED; } -- 2.20.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] http: cast result to FILE * 2019-02-01 19:30 [PATCH] http: cast result to FILE * Dan McGregor @ 2019-02-01 21:17 ` Junio C Hamano 2019-02-02 11:21 ` Duy Nguyen 2019-02-09 2:36 ` [PATCH v2] git-compat-util: undefine fileno if defined Dan McGregor 1 sibling, 1 reply; 16+ messages in thread From: Junio C Hamano @ 2019-02-01 21:17 UTC (permalink / raw) To: Dan McGregor; +Cc: git, masayasuzuki Dan McGregor <dan.mcgregor@usask.ca> writes: > Commit 8dd2e88a92 ("http: support file handles for HTTP_KEEP_ERROR", > 2019-01-10) introduced an implicit assumption that rewind, fileno, and > fflush are functions. At least on FreeBSD fileno is not, and as such > passing a void * failed. I am not strongly opposed to this patch, but shouldn't you be filing a bug report against FreeBSD instead? The implementation is free to define fileno(fh) as a macro, but it shouldn't force such a change to conformant programs. https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=206146 > Explicitly cast result to a FILE * when using standard functions that > may ultimately be macros. > > Signed-off-by: Dan McGregor <dan.mcgregor@usask.ca> > --- > http.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/http.c b/http.c > index 954bebf684..8b9476b151 100644 > --- a/http.c > +++ b/http.c > @@ -1996,12 +1996,12 @@ static int http_request_reauth(const char *url, > strbuf_reset(result); > break; > case HTTP_REQUEST_FILE: > - if (fflush(result)) { > + if (fflush((FILE *)result)) { > error_errno("unable to flush a file"); > return HTTP_START_FAILED; > } > - rewind(result); > - if (ftruncate(fileno(result), 0) < 0) { > + rewind((FILE *)result); > + if (ftruncate(fileno((FILE *)result), 0) < 0) { > error_errno("unable to truncate a file"); > return HTTP_START_FAILED; > } ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] http: cast result to FILE * 2019-02-01 21:17 ` Junio C Hamano @ 2019-02-02 11:21 ` Duy Nguyen 2019-02-04 11:44 ` Johannes Schindelin 0 siblings, 1 reply; 16+ messages in thread From: Duy Nguyen @ 2019-02-02 11:21 UTC (permalink / raw) To: Junio C Hamano; +Cc: Dan McGregor, Git Mailing List, Masaya Suzuki On Sat, Feb 2, 2019 at 4:21 AM Junio C Hamano <gitster@pobox.com> wrote: > > Dan McGregor <dan.mcgregor@usask.ca> writes: > > > Commit 8dd2e88a92 ("http: support file handles for HTTP_KEEP_ERROR", > > 2019-01-10) introduced an implicit assumption that rewind, fileno, and > > fflush are functions. At least on FreeBSD fileno is not, and as such > > passing a void * failed. > > I am not strongly opposed to this patch, Even if this is needed, should it be done behind git-compat-util.h instead? That way if fileno(void*) is used elsewhere, we don't have to do the casting again. > but shouldn't you be filing > a bug report against FreeBSD instead? The implementation is free to > define fileno(fh) as a macro, but it shouldn't force such a change > to conformant programs. > > https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=206146 > > > Explicitly cast result to a FILE * when using standard functions that > > may ultimately be macros. > > > > Signed-off-by: Dan McGregor <dan.mcgregor@usask.ca> > > --- > > http.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/http.c b/http.c > > index 954bebf684..8b9476b151 100644 > > --- a/http.c > > +++ b/http.c > > @@ -1996,12 +1996,12 @@ static int http_request_reauth(const char *url, > > strbuf_reset(result); > > break; > > case HTTP_REQUEST_FILE: > > - if (fflush(result)) { > > + if (fflush((FILE *)result)) { > > error_errno("unable to flush a file"); > > return HTTP_START_FAILED; > > } > > - rewind(result); > > - if (ftruncate(fileno(result), 0) < 0) { > > + rewind((FILE *)result); > > + if (ftruncate(fileno((FILE *)result), 0) < 0) { > > error_errno("unable to truncate a file"); > > return HTTP_START_FAILED; > > } -- Duy ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] http: cast result to FILE * 2019-02-02 11:21 ` Duy Nguyen @ 2019-02-04 11:44 ` Johannes Schindelin 2019-02-04 12:13 ` Duy Nguyen 0 siblings, 1 reply; 16+ messages in thread From: Johannes Schindelin @ 2019-02-04 11:44 UTC (permalink / raw) To: Duy Nguyen; +Cc: Junio C Hamano, Dan McGregor, Git Mailing List, Masaya Suzuki Hi Duy, On Sat, 2 Feb 2019, Duy Nguyen wrote: > On Sat, Feb 2, 2019 at 4:21 AM Junio C Hamano <gitster@pobox.com> wrote: > > > > Dan McGregor <dan.mcgregor@usask.ca> writes: > > > > > Commit 8dd2e88a92 ("http: support file handles for HTTP_KEEP_ERROR", > > > 2019-01-10) introduced an implicit assumption that rewind, fileno, and > > > fflush are functions. At least on FreeBSD fileno is not, and as such > > > passing a void * failed. > > > > I am not strongly opposed to this patch, > > Even if this is needed, should it be done behind git-compat-util.h > instead? That way if fileno(void*) is used elsewhere, we don't have to > do the casting again. The disadvantage, of course, would be that other call sites would not benefit from a manual auditing whether the argument has side effects (and thus, whether a macro using the argument multiple times would result in very unexpected multiple side effects). I'd rather not paper over this issue and thereby open even larger problems. Ciao, Dscho > > > but shouldn't you be filing > > a bug report against FreeBSD instead? The implementation is free to > > define fileno(fh) as a macro, but it shouldn't force such a change > > to conformant programs. > > > > https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=206146 > > > > > Explicitly cast result to a FILE * when using standard functions that > > > may ultimately be macros. > > > > > > Signed-off-by: Dan McGregor <dan.mcgregor@usask.ca> > > > --- > > > http.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/http.c b/http.c > > > index 954bebf684..8b9476b151 100644 > > > --- a/http.c > > > +++ b/http.c > > > @@ -1996,12 +1996,12 @@ static int http_request_reauth(const char *url, > > > strbuf_reset(result); > > > break; > > > case HTTP_REQUEST_FILE: > > > - if (fflush(result)) { > > > + if (fflush((FILE *)result)) { > > > error_errno("unable to flush a file"); > > > return HTTP_START_FAILED; > > > } > > > - rewind(result); > > > - if (ftruncate(fileno(result), 0) < 0) { > > > + rewind((FILE *)result); > > > + if (ftruncate(fileno((FILE *)result), 0) < 0) { > > > error_errno("unable to truncate a file"); > > > return HTTP_START_FAILED; > > > } > > > > -- > Duy > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] http: cast result to FILE * 2019-02-04 11:44 ` Johannes Schindelin @ 2019-02-04 12:13 ` Duy Nguyen 2019-02-04 18:10 ` Junio C Hamano 0 siblings, 1 reply; 16+ messages in thread From: Duy Nguyen @ 2019-02-04 12:13 UTC (permalink / raw) To: Johannes Schindelin Cc: Junio C Hamano, Dan McGregor, Git Mailing List, Masaya Suzuki On Mon, Feb 4, 2019 at 6:44 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > Hi Duy, > > On Sat, 2 Feb 2019, Duy Nguyen wrote: > > > On Sat, Feb 2, 2019 at 4:21 AM Junio C Hamano <gitster@pobox.com> wrote: > > > > > > Dan McGregor <dan.mcgregor@usask.ca> writes: > > > > > > > Commit 8dd2e88a92 ("http: support file handles for HTTP_KEEP_ERROR", > > > > 2019-01-10) introduced an implicit assumption that rewind, fileno, and > > > > fflush are functions. At least on FreeBSD fileno is not, and as such > > > > passing a void * failed. > > > > > > I am not strongly opposed to this patch, > > > > Even if this is needed, should it be done behind git-compat-util.h > > instead? That way if fileno(void*) is used elsewhere, we don't have to > > do the casting again. > > The disadvantage, of course, would be that other call sites would not > benefit from a manual auditing whether the argument has side effects (and > thus, whether a macro using the argument multiple times would result in > very unexpected multiple side effects). That's just a better reason to "fix" it in compat/. If you define a git_fileno() function and map fileno to it, then you won't have to deal with side effects of FreeBSD's fileno() macro. All evaluation happens before git_fileno() is called. > I'd rather not paper over this issue and thereby open even larger > problems. > > Ciao, > Dscho > > > > > > but shouldn't you be filing > > > a bug report against FreeBSD instead? The implementation is free to > > > define fileno(fh) as a macro, but it shouldn't force such a change > > > to conformant programs. > > > > > > https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=206146 > > > > > > > Explicitly cast result to a FILE * when using standard functions that > > > > may ultimately be macros. > > > > > > > > Signed-off-by: Dan McGregor <dan.mcgregor@usask.ca> > > > > --- > > > > http.c | 6 +++--- > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/http.c b/http.c > > > > index 954bebf684..8b9476b151 100644 > > > > --- a/http.c > > > > +++ b/http.c > > > > @@ -1996,12 +1996,12 @@ static int http_request_reauth(const char *url, > > > > strbuf_reset(result); > > > > break; > > > > case HTTP_REQUEST_FILE: > > > > - if (fflush(result)) { > > > > + if (fflush((FILE *)result)) { > > > > error_errno("unable to flush a file"); > > > > return HTTP_START_FAILED; > > > > } > > > > - rewind(result); > > > > - if (ftruncate(fileno(result), 0) < 0) { > > > > + rewind((FILE *)result); > > > > + if (ftruncate(fileno((FILE *)result), 0) < 0) { > > > > error_errno("unable to truncate a file"); > > > > return HTTP_START_FAILED; > > > > } > > > > > > > > -- > > Duy > > -- Duy ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] http: cast result to FILE * 2019-02-04 12:13 ` Duy Nguyen @ 2019-02-04 18:10 ` Junio C Hamano 2019-02-05 1:36 ` Dan McGregor 0 siblings, 1 reply; 16+ messages in thread From: Junio C Hamano @ 2019-02-04 18:10 UTC (permalink / raw) To: Duy Nguyen Cc: Johannes Schindelin, Dan McGregor, Git Mailing List, Masaya Suzuki Duy Nguyen <pclouds@gmail.com> writes: >> The disadvantage, of course, would be that other call sites would not >> benefit from a manual auditing whether the argument has side effects (and >> thus, whether a macro using the argument multiple times would result in >> very unexpected multiple side effects). > > That's just a better reason to "fix" it in compat/. If you define a > git_fileno() function and map fileno to it, then you won't have to > deal with side effects of FreeBSD's fileno() macro. All evaluation > happens before git_fileno() is called. Hmph, so the idea is to have /* do not include git-compat-util.h here */ int wrapped_fileno(FILE *f) { return fileno(f); } in compat/fileno.c and then do something like this #ifdef fileno #undef fileno #define fileno(x) wrapped_fileno(x) #endif for FreeBSD in git-compat-util.h or something like that? I think I can buy that. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] http: cast result to FILE * 2019-02-04 18:10 ` Junio C Hamano @ 2019-02-05 1:36 ` Dan McGregor 0 siblings, 0 replies; 16+ messages in thread From: Dan McGregor @ 2019-02-05 1:36 UTC (permalink / raw) To: Junio C Hamano, Duy Nguyen Cc: Johannes Schindelin, McGregor, Dan, Git Mailing List, Masaya Suzuki On February 4, 2019 12:10:54 p.m. CST, Junio C Hamano <gitster@pobox.com> wrote: >Duy Nguyen <pclouds@gmail.com> writes: > >>> The disadvantage, of course, would be that other call sites would >not >>> benefit from a manual auditing whether the argument has side effects >(and >>> thus, whether a macro using the argument multiple times would result >in >>> very unexpected multiple side effects). >> >> That's just a better reason to "fix" it in compat/. If you define a >> git_fileno() function and map fileno to it, then you won't have to >> deal with side effects of FreeBSD's fileno() macro. All evaluation >> happens before git_fileno() is called. > >Hmph, so the idea is to have > > /* do not include git-compat-util.h here */ > int wrapped_fileno(FILE *f) > { > return fileno(f); > } > >in compat/fileno.c and then do something like this > > #ifdef fileno > #undef fileno > #define fileno(x) wrapped_fileno(x) > #endif > >for FreeBSD in git-compat-util.h or something like that? > >I think I can buy that. Works for me too. FreeBSD's not alone in this, either. It looks like NetBSD does the same thing. I'll send a v2 tomorrow. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] git-compat-util: undefine fileno if defined 2019-02-01 19:30 [PATCH] http: cast result to FILE * Dan McGregor 2019-02-01 21:17 ` Junio C Hamano @ 2019-02-09 2:36 ` Dan McGregor 2019-02-09 18:34 ` Junio C Hamano 2019-02-12 13:45 ` Duy Nguyen 1 sibling, 2 replies; 16+ messages in thread From: Dan McGregor @ 2019-02-09 2:36 UTC (permalink / raw) To: git; +Cc: Dan McGregor Commit 8dd2e88a92 ("http: support file handles for HTTP_KEEP_ERROR", 2019-01-10) introduced an implicit assumption that rewind, fileno, and fflush are functions. At least on FreeBSD fileno is not, and as such passing a void * failed. All systems tested (FreeBSD and NetBSD) that define fineo as a macro also have a function defined. Undefine the macro on these systems so that the function is used. Signed-off-by: Dan McGregor <dan.mcgregor@usask.ca> --- git-compat-util.h | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/git-compat-util.h b/git-compat-util.h index 29a19902aa..b5489bbcf2 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -764,6 +764,15 @@ char *gitstrdup(const char *s); extern FILE *git_fopen(const char*, const char*); #endif +/* Some systems (the various BSDs for eaxmple) define + * fileno as a macro as an optimization. All systems I + * know about also define it as a real funcion, so use + * the real function to allow passing void *s to fileno. + */ +#ifdef fileno +# undef fileno +#endif + #ifdef SNPRINTF_RETURNS_BOGUS #ifdef snprintf #undef snprintf -- 2.20.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2] git-compat-util: undefine fileno if defined 2019-02-09 2:36 ` [PATCH v2] git-compat-util: undefine fileno if defined Dan McGregor @ 2019-02-09 18:34 ` Junio C Hamano 2019-02-12 13:45 ` Duy Nguyen 1 sibling, 0 replies; 16+ messages in thread From: Junio C Hamano @ 2019-02-09 18:34 UTC (permalink / raw) To: Dan McGregor; +Cc: git Dan McGregor <dan.mcgregor@usask.ca> writes: > Commit 8dd2e88a92 ("http: support file handles for HTTP_KEEP_ERROR", > 2019-01-10) introduced an implicit assumption that rewind, fileno, and > fflush are functions. At least on FreeBSD fileno is not, and as such > passing a void * failed. > > All systems tested (FreeBSD and NetBSD) that define fineo as a macro s/fineo/fileno/ > also have a function defined. Undefine the macro on these systems so > that the function is used. That smells like the patch is assuming a bit too much. A platform you did not inspect may have it as a macro but the implementation may still be usable without breakage like the one we saw on FreeBSD, for example. It also robs us the possibility of overriding fileno() with our own macro earlier in this file, like we do for many functions you see in the output from this: $ git grep '#define .* git' git-compat-util.h In general, I'd like to see people thinking twice about an overly simple approach when they justify it with "this should work everywhere I know of". > Signed-off-by: Dan McGregor <dan.mcgregor@usask.ca> > --- > git-compat-util.h | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/git-compat-util.h b/git-compat-util.h > index 29a19902aa..b5489bbcf2 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -764,6 +764,15 @@ char *gitstrdup(const char *s); > extern FILE *git_fopen(const char*, const char*); > #endif > > +/* Some systems (the various BSDs for eaxmple) define Style: /* * Our multi-line comment starts with a lone slash-star * and ends with a lone star-slash, like this. */ > + * fileno as a macro as an optimization. All systems I > + * know about also define it as a real funcion, so use > + * the real function to allow passing void *s to fileno. > + */ > +#ifdef fileno > +# undef fileno > +#endif > + > #ifdef SNPRINTF_RETURNS_BOGUS > #ifdef snprintf > #undef snprintf ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] git-compat-util: undefine fileno if defined 2019-02-09 2:36 ` [PATCH v2] git-compat-util: undefine fileno if defined Dan McGregor 2019-02-09 18:34 ` Junio C Hamano @ 2019-02-12 13:45 ` Duy Nguyen 2019-02-12 14:14 ` Duy Nguyen ` (2 more replies) 1 sibling, 3 replies; 16+ messages in thread From: Duy Nguyen @ 2019-02-12 13:45 UTC (permalink / raw) To: Dan McGregor; +Cc: git, Junio C Hamano On Fri, Feb 08, 2019 at 08:36:21PM -0600, Dan McGregor wrote: > Commit 8dd2e88a92 ("http: support file handles for HTTP_KEEP_ERROR", > 2019-01-10) introduced an implicit assumption that rewind, fileno, and > fflush are functions. At least on FreeBSD fileno is not, and as such > passing a void * failed. > > All systems tested (FreeBSD and NetBSD) that define fineo as a macro OpenBSD or NetBSD? From this [1], it looks like OpenBSD fails while NetBSD compiles ok (and fails to run some tests) [1] https://gitlab.com/git-vcs/git-ci/pipelines/47139741/failures > also have a function defined. Undefine the macro on these systems so > that the function is used. I don't think this is enough. At least fbsd defines this #define fileno(p) (!__isthreaded ? __sfileno(p) : (fileno)(p)) so one of the two functions will be used depending on __isthreaded flag. Your forcing to use fileno, ignoring __sfileno, is technically not correct. For the record, at least fbsd also defines feof, ferror, clearerr, getc and putc in the same way. But at least I don't see how something like feof(fp++) could cause bad side effects. So, how about something like this? A teeny bit longer than your version, but I think it's easier to control long term. -- 8< -- diff --git a/Makefile b/Makefile index 0e13a5b469..44cfc63fc4 100644 --- a/Makefile +++ b/Makefile @@ -433,6 +433,8 @@ all:: # # Define HAVE_GETDELIM if your system has the getdelim() function. # +# Define FILENO_IS_A_MACRO is fileno() is a macro, not a real function. +# # Define PAGER_ENV to a SP separated VAR=VAL pairs to define # default environment variables to be passed when a pager is spawned, e.g. # @@ -1800,6 +1802,11 @@ ifdef HAVE_WPGMPTR BASIC_CFLAGS += -DHAVE_WPGMPTR endif +ifdef FILENO_IS_A_MACRO + COMPAT_CFLAGS += -DFILENO_IS_A_MACRO + COMPAT_OBJS += compat/fileno.o +endif + ifeq ($(TCLTK_PATH),) NO_TCLTK = NoThanks endif diff --git a/compat/fileno.c b/compat/fileno.c new file mode 100644 index 0000000000..7b105f4cd7 --- /dev/null +++ b/compat/fileno.c @@ -0,0 +1,7 @@ +#define COMPAT_CODE +#include "../git-compat-util.h" + +int git_fileno(FILE *stream) +{ + return fileno(stream); +} diff --git a/config.mak.uname b/config.mak.uname index 786bb2f913..01f62674a4 100644 --- a/config.mak.uname +++ b/config.mak.uname @@ -221,6 +221,7 @@ ifeq ($(uname_S),FreeBSD) HAVE_BSD_KERN_PROC_SYSCTL = YesPlease PAGER_ENV = LESS=FRX LV=-c MORE=FRX FREAD_READS_DIRECTORIES = UnfortunatelyYes + FILENO_IS_A_MACRO = UnfortunatelyYes endif ifeq ($(uname_S),OpenBSD) NO_STRCASESTR = YesPlease @@ -234,6 +235,7 @@ ifeq ($(uname_S),OpenBSD) HAVE_BSD_KERN_PROC_SYSCTL = YesPlease PROCFS_EXECUTABLE_PATH = /proc/curproc/file FREAD_READS_DIRECTORIES = UnfortunatelyYes + FILENO_IS_A_MACRO = UnfortunatelyYes endif ifeq ($(uname_S),MirBSD) NO_STRCASESTR = YesPlease diff --git a/git-compat-util.h b/git-compat-util.h index 29a19902aa..6573808ebd 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -1234,6 +1234,14 @@ struct tm *git_gmtime_r(const time_t *, struct tm *); #define getc_unlocked(fh) getc(fh) #endif +#ifdef FILENO_IS_A_MACRO +int git_fileno(FILE *stream); +# ifndef COMPAT_CODE +# undef fileno +# define fileno(p) git_fileno(p) +# endif +#endif + /* * Our code often opens a path to an optional file, to work on its * contents when we can successfully open it. We can ignore a failure -- 8< -- -- Duy ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2] git-compat-util: undefine fileno if defined 2019-02-12 13:45 ` Duy Nguyen @ 2019-02-12 14:14 ` Duy Nguyen 2019-02-12 16:56 ` Junio C Hamano 2019-02-16 2:33 ` Dan McGregor 2 siblings, 0 replies; 16+ messages in thread From: Duy Nguyen @ 2019-02-12 14:14 UTC (permalink / raw) To: Dan McGregor; +Cc: Git Mailing List, Junio C Hamano On Tue, Feb 12, 2019 at 8:45 PM Duy Nguyen <pclouds@gmail.com> wrote: > > On Fri, Feb 08, 2019 at 08:36:21PM -0600, Dan McGregor wrote: > > Commit 8dd2e88a92 ("http: support file handles for HTTP_KEEP_ERROR", > > 2019-01-10) introduced an implicit assumption that rewind, fileno, and > > fflush are functions. At least on FreeBSD fileno is not, and as such > > passing a void * failed. > > > > All systems tested (FreeBSD and NetBSD) that define fineo as a macro > > OpenBSD or NetBSD? From this [1], it looks like OpenBSD fails while > NetBSD compiles ok (and fails to run some tests) According to this [2] looks like NetBSD is affected too, but only in no-thread mode. With pthreads, fileno is not redefined as a macro. That probably explains why it's ok from git-ci. #define fileno(p) __sfileno(p) #define __sfileno(p) \ ((p)->_file == -1 ? -1 : (int)(unsigned short)(p)->_file) [2] http://cvsweb.netbsd.org/bsdweb.cgi/src/include/stdio.h?rev=1.97&content-type=text/x-cvsweb-markup > For the record, at least fbsd also defines feof, ferror, clearerr, > getc and putc in the same way. But at least I don't see how something > like feof(fp++) could cause bad side effects. ... and __sfileno (and other friends) can cause bad side effects, sigh. It's probably best to avoid fancy function calls. -- Duy ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] git-compat-util: undefine fileno if defined 2019-02-12 13:45 ` Duy Nguyen 2019-02-12 14:14 ` Duy Nguyen @ 2019-02-12 16:56 ` Junio C Hamano 2019-02-16 2:33 ` Dan McGregor 2 siblings, 0 replies; 16+ messages in thread From: Junio C Hamano @ 2019-02-12 16:56 UTC (permalink / raw) To: Duy Nguyen; +Cc: Dan McGregor, git Duy Nguyen <pclouds@gmail.com> writes: > So, how about something like this? A teeny bit longer than your > version, but I think it's easier to control long term. > > -- 8< -- > diff --git a/Makefile b/Makefile > index 0e13a5b469..44cfc63fc4 100644 > --- a/Makefile > +++ b/Makefile > @@ -433,6 +433,8 @@ all:: > # > # Define HAVE_GETDELIM if your system has the getdelim() function. > # > +# Define FILENO_IS_A_MACRO is fileno() is a macro, not a real function. s/is fileno() is/if filno() is/; I agree with the direction of this patch takes us. I have a mixed feeling about the COMPAT_CODE macro trick (presumably you did not want to include stdio.h directly for "FILE *" and instead follow the usual "the first header included is the compat-util" pattern; I was imagining that we'd liberally include system headers inside compat/ directory. If we can agree on reusing this single macro to control conditional in git-compat-util.h when a similar need arises in the future, this may be a good solution. > +# > # Define PAGER_ENV to a SP separated VAR=VAL pairs to define > # default environment variables to be passed when a pager is spawned, e.g. > # > @@ -1800,6 +1802,11 @@ ifdef HAVE_WPGMPTR > BASIC_CFLAGS += -DHAVE_WPGMPTR > endif > > +ifdef FILENO_IS_A_MACRO > + COMPAT_CFLAGS += -DFILENO_IS_A_MACRO > + COMPAT_OBJS += compat/fileno.o > +endif > + > ifeq ($(TCLTK_PATH),) > NO_TCLTK = NoThanks > endif > diff --git a/compat/fileno.c b/compat/fileno.c > new file mode 100644 > index 0000000000..7b105f4cd7 > --- /dev/null > +++ b/compat/fileno.c > @@ -0,0 +1,7 @@ > +#define COMPAT_CODE > +#include "../git-compat-util.h" > + > +int git_fileno(FILE *stream) > +{ > + return fileno(stream); > +} > diff --git a/config.mak.uname b/config.mak.uname > index 786bb2f913..01f62674a4 100644 > --- a/config.mak.uname > +++ b/config.mak.uname > @@ -221,6 +221,7 @@ ifeq ($(uname_S),FreeBSD) > HAVE_BSD_KERN_PROC_SYSCTL = YesPlease > PAGER_ENV = LESS=FRX LV=-c MORE=FRX > FREAD_READS_DIRECTORIES = UnfortunatelyYes > + FILENO_IS_A_MACRO = UnfortunatelyYes > endif > ifeq ($(uname_S),OpenBSD) > NO_STRCASESTR = YesPlease > @@ -234,6 +235,7 @@ ifeq ($(uname_S),OpenBSD) > HAVE_BSD_KERN_PROC_SYSCTL = YesPlease > PROCFS_EXECUTABLE_PATH = /proc/curproc/file > FREAD_READS_DIRECTORIES = UnfortunatelyYes > + FILENO_IS_A_MACRO = UnfortunatelyYes > endif > ifeq ($(uname_S),MirBSD) > NO_STRCASESTR = YesPlease > diff --git a/git-compat-util.h b/git-compat-util.h > index 29a19902aa..6573808ebd 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -1234,6 +1234,14 @@ struct tm *git_gmtime_r(const time_t *, struct tm *); > #define getc_unlocked(fh) getc(fh) > #endif > > +#ifdef FILENO_IS_A_MACRO > +int git_fileno(FILE *stream); > +# ifndef COMPAT_CODE > +# undef fileno > +# define fileno(p) git_fileno(p) > +# endif > +#endif > + > /* > * Our code often opens a path to an optional file, to work on its > * contents when we can successfully open it. We can ignore a failure > -- 8< -- > > -- > Duy ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] git-compat-util: undefine fileno if defined 2019-02-12 13:45 ` Duy Nguyen 2019-02-12 14:14 ` Duy Nguyen 2019-02-12 16:56 ` Junio C Hamano @ 2019-02-16 2:33 ` Dan McGregor 2019-05-08 7:46 ` Junio C Hamano 2 siblings, 1 reply; 16+ messages in thread From: Dan McGregor @ 2019-02-16 2:33 UTC (permalink / raw) To: Duy Nguyen, McGregor, Dan; +Cc: git@vger.kernel.org, Junio C Hamano On February 12, 2019 7:45:37 a.m. CST, Duy Nguyen <pclouds@gmail.com> wrote: >On Fri, Feb 08, 2019 at 08:36:21PM -0600, Dan McGregor wrote: >> Commit 8dd2e88a92 ("http: support file handles for HTTP_KEEP_ERROR", >> 2019-01-10) introduced an implicit assumption that rewind, fileno, >and >> fflush are functions. At least on FreeBSD fileno is not, and as such >> passing a void * failed. >> >> All systems tested (FreeBSD and NetBSD) that define fineo as a macro > >OpenBSD or NetBSD? From this [1], it looks like OpenBSD fails while >NetBSD compiles ok (and fails to run some tests) > >[1] https://gitlab.com/git-vcs/git-ci/pipelines/47139741/failures > >> also have a function defined. Undefine the macro on these systems so >> that the function is used. > >I don't think this is enough. At least fbsd defines this > >#define fileno(p) (!__isthreaded ? __sfileno(p) : (fileno)(p)) > >so one of the two functions will be used depending on __isthreaded >flag. Your forcing to use fileno, ignoring __sfileno, is technically >not correct. > >For the record, at least fbsd also defines feof, ferror, clearerr, >getc and putc in the same way. But at least I don't see how something >like feof(fp++) could cause bad side effects. > >So, how about something like this? A teeny bit longer than your >version, but I think it's easier to control long term. Yes, this looks pretty reasonable to me too. > >-- 8< -- >diff --git a/Makefile b/Makefile >index 0e13a5b469..44cfc63fc4 100644 >--- a/Makefile >+++ b/Makefile >@@ -433,6 +433,8 @@ all:: > # > # Define HAVE_GETDELIM if your system has the getdelim() function. > # >+# Define FILENO_IS_A_MACRO is fileno() is a macro, not a real >function. >+# > # Define PAGER_ENV to a SP separated VAR=VAL pairs to define ># default environment variables to be passed when a pager is spawned, >e.g. > # >@@ -1800,6 +1802,11 @@ ifdef HAVE_WPGMPTR > BASIC_CFLAGS += -DHAVE_WPGMPTR > endif > >+ifdef FILENO_IS_A_MACRO >+ COMPAT_CFLAGS += -DFILENO_IS_A_MACRO >+ COMPAT_OBJS += compat/fileno.o >+endif >+ > ifeq ($(TCLTK_PATH),) > NO_TCLTK = NoThanks > endif >diff --git a/compat/fileno.c b/compat/fileno.c >new file mode 100644 >index 0000000000..7b105f4cd7 >--- /dev/null >+++ b/compat/fileno.c >@@ -0,0 +1,7 @@ >+#define COMPAT_CODE >+#include "../git-compat-util.h" >+ >+int git_fileno(FILE *stream) >+{ >+ return fileno(stream); >+} >diff --git a/config.mak.uname b/config.mak.uname >index 786bb2f913..01f62674a4 100644 >--- a/config.mak.uname >+++ b/config.mak.uname >@@ -221,6 +221,7 @@ ifeq ($(uname_S),FreeBSD) > HAVE_BSD_KERN_PROC_SYSCTL = YesPlease > PAGER_ENV = LESS=FRX LV=-c MORE=FRX > FREAD_READS_DIRECTORIES = UnfortunatelyYes >+ FILENO_IS_A_MACRO = UnfortunatelyYes > endif > ifeq ($(uname_S),OpenBSD) > NO_STRCASESTR = YesPlease >@@ -234,6 +235,7 @@ ifeq ($(uname_S),OpenBSD) > HAVE_BSD_KERN_PROC_SYSCTL = YesPlease > PROCFS_EXECUTABLE_PATH = /proc/curproc/file > FREAD_READS_DIRECTORIES = UnfortunatelyYes >+ FILENO_IS_A_MACRO = UnfortunatelyYes > endif > ifeq ($(uname_S),MirBSD) > NO_STRCASESTR = YesPlease >diff --git a/git-compat-util.h b/git-compat-util.h >index 29a19902aa..6573808ebd 100644 >--- a/git-compat-util.h >+++ b/git-compat-util.h >@@ -1234,6 +1234,14 @@ struct tm *git_gmtime_r(const time_t *, struct >tm *); > #define getc_unlocked(fh) getc(fh) > #endif > >+#ifdef FILENO_IS_A_MACRO >+int git_fileno(FILE *stream); >+# ifndef COMPAT_CODE >+# undef fileno >+# define fileno(p) git_fileno(p) >+# endif >+#endif >+ > /* > * Our code often opens a path to an optional file, to work on its > * contents when we can successfully open it. We can ignore a failure >-- 8< -- > >-- >Duy -- Sent from my Android device with K-9 Mail. Please excuse my brevity. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] git-compat-util: undefine fileno if defined 2019-02-16 2:33 ` Dan McGregor @ 2019-05-08 7:46 ` Junio C Hamano 2019-05-08 10:09 ` Duy Nguyen 0 siblings, 1 reply; 16+ messages in thread From: Junio C Hamano @ 2019-05-08 7:46 UTC (permalink / raw) To: Dan McGregor; +Cc: Duy Nguyen, McGregor, Dan, git@vger.kernel.org Dan McGregor <dkm560@usask.ca> writes: >>I don't think this is enough. At least fbsd defines this >> >>#define fileno(p) (!__isthreaded ? __sfileno(p) : (fileno)(p)) >> >>so one of the two functions will be used depending on __isthreaded >>flag. Your forcing to use fileno, ignoring __sfileno, is technically >>not correct. >> >>For the record, at least fbsd also defines feof, ferror, clearerr, >>getc and putc in the same way. But at least I don't see how something >>like feof(fp++) could cause bad side effects. >> >>So, how about something like this? A teeny bit longer than your >>version, but I think it's easier to control long term. > > Yes, this looks pretty reasonable to me too. Sorry for pinging this ancient thread, but while reviewing the stalled topics, this one caught my attention. The very original <20190201193004.88736-1-dan.mcgregor@usask.ca> said that the problem it wants to solve was that the code that passes (void*) parameter to fileno(), fflush() and rewind() misbehaved, as these are all macros on your system. We solved the problem for fileno() being a macro eventually with 18a4f6be ("git-compat-util: work around fileno(fp) that is a macro", 2019-02-12), but what about the other two? Here comes a weather-balloon to see if we should pursue tying this loose end. One thing to note is that this reveals that the build rule for vcs-svn stuff may be cleaned up before we do this as a separate step, but I think I saw another topics to move it out of the main build so perhaps I'll leave it out and leave it to be worried about separately ;-) -- >8 -- From: Junio C Hamano <gitster@pobox.com> Date: Wed, 8 May 2019 16:12:20 +0900 Subject: [PATCH] git-compat-util: rewind(fp)/fflush(fp) could be macros On various BSD's, rewind(fp) is implemented as a macro that directly accesses the fields in the FILE * object, which breaks a function that accepts a "void *fp" parameter and calls rewind(fp) and expect it to work. The same issue has been resolved for fileno(fp) earlier, which is shared by fflush(fp) as well. Work it around by adding a compile-time knob REWIND_IS_A_MACRO and FFLUSH_IS_A_MACRO for these two other functions that tells us to insert a real helper function in the middle of the callchain. I'll leave it up to those who do work on affected platforms to add entries to config.mak.uname. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Makefile | 18 ++++++++++++++++-- compat/fflush.c | 7 +++++++ compat/rewind.c | 7 +++++++ git-compat-util.h | 16 ++++++++++++++++ 4 files changed, 46 insertions(+), 2 deletions(-) create mode 100644 compat/fflush.c create mode 100644 compat/rewind.c diff --git a/Makefile b/Makefile index 6e8d017e8e..1e18092583 100644 --- a/Makefile +++ b/Makefile @@ -433,6 +433,10 @@ all:: # # Define HAVE_GETDELIM if your system has the getdelim() function. # +# Define FFLUSH_IS_A_MACRO if fflush() is a macro, not a real function. +# +# Define REWIND_IS_A_MACRO if rewind() is a macro, not a real function. +# # Define PAGER_ENV to a SP separated VAR=VAL pairs to define # default environment variables to be passed when a pager is spawned, e.g. # @@ -1795,6 +1799,16 @@ ifdef HAVE_WPGMPTR BASIC_CFLAGS += -DHAVE_WPGMPTR endif +ifdef FFLUSH_IS_A_MACRO + COMPAT_CFLAGS += -DFFLUSH_IS_A_MACRO + COMPAT_OBJS += compat/fflush.o +endif + +ifdef REWIND_IS_A_MACRO + COMPAT_CFLAGS += -DREWIND_IS_A_MACRO + COMPAT_OBJS += compat/rewind.o +endif + ifeq ($(TCLTK_PATH),) NO_TCLTK = NoThanks endif @@ -2404,8 +2418,8 @@ git-http-push$X: http.o http-push.o GIT-LDFLAGS $(GITLIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT) $(LIBS) git-remote-testsvn$X: remote-testsvn.o GIT-LDFLAGS $(GITLIBS) $(VCSSVN_LIB) - $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS) \ - $(VCSSVN_LIB) + $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \ + $(VCSSVN_LIB) $(LIBS) $(REMOTE_CURL_ALIASES): $(REMOTE_CURL_PRIMARY) $(QUIET_LNCP)$(RM) $@ && \ diff --git a/compat/fflush.c b/compat/fflush.c new file mode 100644 index 0000000000..55ac3e5542 --- /dev/null +++ b/compat/fflush.c @@ -0,0 +1,7 @@ +#define COMPAT_CODE_FFLUSH +#include "../git-compat-util.h" + +int git_fflush(FILE *stream) +{ + return fflush(stream); +} diff --git a/compat/rewind.c b/compat/rewind.c new file mode 100644 index 0000000000..e56656ff96 --- /dev/null +++ b/compat/rewind.c @@ -0,0 +1,7 @@ +#define COMPAT_CODE_REWIND +#include "../git-compat-util.h" + +void git_rewind(FILE *stream) +{ + rewind(stream); +} diff --git a/git-compat-util.h b/git-compat-util.h index 29a19902aa..f35a857785 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -1234,6 +1234,22 @@ struct tm *git_gmtime_r(const time_t *, struct tm *); #define getc_unlocked(fh) getc(fh) #endif +#ifdef FFLUSH_IS_A_MACRO +int git_fflush(FILE *stream); +# ifndef COMPAT_CODE_FFLUSH +# undef fflush +# define fflush(p) git_fflush(p) +# endif +#endif + +#ifdef REWIND_IS_A_MACRO +void git_rewind(FILE *stream); +# ifndef COMPAT_CODE_REWIND +# undef rewind +# define rewind(p) git_rewind(p) +# endif +#endif + /* * Our code often opens a path to an optional file, to work on its * contents when we can successfully open it. We can ignore a failure -- 2.21.0-777-g83232e3864 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2] git-compat-util: undefine fileno if defined 2019-05-08 7:46 ` Junio C Hamano @ 2019-05-08 10:09 ` Duy Nguyen 2019-05-08 10:16 ` Junio C Hamano 0 siblings, 1 reply; 16+ messages in thread From: Duy Nguyen @ 2019-05-08 10:09 UTC (permalink / raw) To: Junio C Hamano; +Cc: Dan McGregor, McGregor, Dan, git@vger.kernel.org On Wed, May 8, 2019 at 2:47 PM Junio C Hamano <gitster@pobox.com> wrote: > > Dan McGregor <dkm560@usask.ca> writes: > > >>I don't think this is enough. At least fbsd defines this > >> > >>#define fileno(p) (!__isthreaded ? __sfileno(p) : (fileno)(p)) > >> > >>so one of the two functions will be used depending on __isthreaded > >>flag. Your forcing to use fileno, ignoring __sfileno, is technically > >>not correct. > >> > >>For the record, at least fbsd also defines feof, ferror, clearerr, > >>getc and putc in the same way. But at least I don't see how something > >>like feof(fp++) could cause bad side effects. > >> > >>So, how about something like this? A teeny bit longer than your > >>version, but I think it's easier to control long term. > > > > Yes, this looks pretty reasonable to me too. > > Sorry for pinging this ancient thread, but while reviewing the > stalled topics, this one caught my attention. The very original > <20190201193004.88736-1-dan.mcgregor@usask.ca> said that the problem > it wants to solve was that the code that passes (void*) parameter to > fileno(), fflush() and rewind() misbehaved, as these are all macros > on your system. > > We solved the problem for fileno() being a macro eventually with > 18a4f6be ("git-compat-util: work around fileno(fp) that is a macro", > 2019-02-12), but what about the other two? I don't think the other two were the problem. Even in the mail you pointed to, only fileno() is acknowledged the problem in the commit message. At least for BSDs fflush() and rewind() are not macros. BSDs optimize a couple functions for the no-pthread case by inlining some short expressions directly [1]. This works well for read-only functions such as fileno(), feof(), ferror().. and pushing the limits a bit with fputc() and fgetc(), but fflush() and rewind() are (I think) just too complicated to do it this way. [1] https://github.com/freebsd/freebsd/blob/master/include/stdio.h#L451-L519 [2] https://public-inbox.org/git/20190209023621.75255-1-dan.mcgregor@usask.ca/ > Here comes a weather-balloon to see if we should pursue tying this > loose end. Let's see if there are still problems with more exotic platforms. I'm reluctant of adding unused compat/ code because to me compat/ is scary. I often don't have the right systems to test whenever I need to make changes in compat/ -- Duy ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] git-compat-util: undefine fileno if defined 2019-05-08 10:09 ` Duy Nguyen @ 2019-05-08 10:16 ` Junio C Hamano 0 siblings, 0 replies; 16+ messages in thread From: Junio C Hamano @ 2019-05-08 10:16 UTC (permalink / raw) To: Duy Nguyen; +Cc: Dan McGregor, McGregor, Dan, git@vger.kernel.org Duy Nguyen <pclouds@gmail.com> writes: > I don't think the other two were the problem. Even in the mail you > pointed to, only fileno() is acknowledged the problem in the commit > message. > > At least for BSDs fflush() and rewind() are not macros. I can drop the dm/some-stdio-functions-are-macro-on-freebsd topic safely then. That was the most important thing I wanted to know about ;-) > Let's see if there are still problems with more exotic platforms. I'm > reluctant of adding unused compat/ code because to me compat/ is > scary. Right. Thanks. ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-05-08 10:16 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-01 19:30 [PATCH] http: cast result to FILE * Dan McGregor 2019-02-01 21:17 ` Junio C Hamano 2019-02-02 11:21 ` Duy Nguyen 2019-02-04 11:44 ` Johannes Schindelin 2019-02-04 12:13 ` Duy Nguyen 2019-02-04 18:10 ` Junio C Hamano 2019-02-05 1:36 ` Dan McGregor 2019-02-09 2:36 ` [PATCH v2] git-compat-util: undefine fileno if defined Dan McGregor 2019-02-09 18:34 ` Junio C Hamano 2019-02-12 13:45 ` Duy Nguyen 2019-02-12 14:14 ` Duy Nguyen 2019-02-12 16:56 ` Junio C Hamano 2019-02-16 2:33 ` Dan McGregor 2019-05-08 7:46 ` Junio C Hamano 2019-05-08 10:09 ` Duy Nguyen 2019-05-08 10:16 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).