On Wed, Feb 15, 2017 at 02:28:10PM -0800, Junio C Hamano wrote:

> Jeff King <p...@peff.net> writes:
> 
> >>   abort:
> >>    strbuf_release(&note);
> >>    free(url);
> >> -  fclose(fp);
> >> +  if (ferror(fp))
> >> +          rc = -1;
> >> +  if (fclose(fp))
> >> +          rc = -1;
> >>    return rc;
> >
> > Yeah, I think this works. Normally you'd want to flush before checking
> > ferror(), but since you detect errors from fclose, too, it should be
> > fine.
> >
> > We probably should write something stderr, though. Maybe:
> >
> >   if (ferror(fp) || fclose(fp))
> >     rc = error_errno("unable to write to %s", filename);
> 
> Yes, and somehow make sure we do fclose(fp) even when we have an
> error already ;-)

Good catch. I think we use a nasty bitwise-OR elsewhere to do that.
Ah, here it is, in tempfile.c:

                /*
                 * Note: no short-circuiting here; we want to fclose()
                 * in any case!
                 */
                err = ferror(fp) | fclose(fp);

That works, but the fact that we need a comment is a good sign that it's
kind of gross. It's too bad stdio does not specify the return of fclose
to report an error in the close _or_ any previous error. I guess we
could wrap it with our own function.

-Peff

Reply via email to