On Wed, Feb 15, 2017 at 02:28:10PM -0800, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> >> abort:
> >> strbuf_release(¬e);
> >> 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