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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Feb 4, 2019, 11:44 UTC
Message-ID: <nycvar.QRO.7.76.6.1902041243220.41@tvgsbejvaqbjf.bet>
In-Reply-To: <CACsJy8B_=qbeimp5=RS-r2gwEjVV9rDE_2_tk_DDqz6rJazvFw@mail.gmail.com>

Hi Duy,

On Sat, 2 Feb 2019, Duy Nguyen wrote:

Show 14 quoted lines

> 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

Show 42 quoted lines

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