[PATCH] http: cast result to FILE *

[PATCH] http: cast result to FILE *

[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

Re: [PATCH] http: cast result to FILE *

[Junio C Hamano]

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;
>  		}

Re: [PATCH] http: cast result to FILE *

[Duy Nguyen]

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

Re: [PATCH] http: cast result to FILE *

[Johannes Schindelin]

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
> 

Re: [PATCH] http: cast result to FILE *

[Duy Nguyen]

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

Re: [PATCH] http: cast result to FILE *

[Junio C Hamano]

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.

Re: [PATCH] http: cast result to FILE *

[Dan McGregor]

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.

[PATCH v2] git-compat-util: undefine fileno if defined

[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

Re: [PATCH v2] git-compat-util: undefine fileno if defined

[Junio C Hamano]

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

Re: [PATCH v2] git-compat-util: undefine fileno if defined

[Duy Nguyen]

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

Re: [PATCH v2] git-compat-util: undefine fileno if defined

[Duy Nguyen]

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

Re: [PATCH v2] git-compat-util: undefine fileno if defined

[Junio C Hamano]

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

Re: [PATCH v2] git-compat-util: undefine fileno if defined

[Dan McGregor]

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.

Re: [PATCH v2] git-compat-util: undefine fileno if defined

[Junio C Hamano]

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

Re: [PATCH v2] git-compat-util: undefine fileno if defined

[Duy Nguyen]

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

Re: [PATCH v2] git-compat-util: undefine fileno if defined

[Junio C Hamano]

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.