Re: [PATCH] tempfile: avoid "ferror | fclose" trick
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 17, 2017, 21:17 UTC
- Message-ID
- <xmqq37fcsejx.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170217205442.wnldfsxbj3dnnqvj@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 19 quoted lines
> On Fri, Feb 17, 2017 at 11:42:25AM +0100, Michael Haggerty wrote: > >> On 02/17/2017 09:07 AM, Jeff King wrote: >> > [...] >> > That's similar to what I wrote earlier, but if we don't mind overwriting >> > errno unconditionally, I think just: >> > >> > errno = EIO; /* covers ferror(), overwritten by failing fclose() */ >> > err |= ferror(fp); >> > err |= fclose(fp); >> > >> > does the same thing. >> >> True; I'd forgotten the convention that non-failing functions are >> allowed to change errno. Your solution is obviously simpler and faster. > > I guess we are simultaneously assuming that it is OK to munge errno on > success in our function, but that fclose() will not do so. Which seems a > bit hypocritical. Maybe the "if" dance is better.
Yes. When both ferror() and fclose() are successful, we would prefer to see the original errno unmolested, so the "if" dance, even though it looks uglier, is better. The ugliness is limited to the implementation anyway ;-)
But it does look ugly, especially when nested inside the existing code like so.
Stepping back a bit, would this be really needed? Even if the ferror() does not update errno, the original stdio operation that failed would have, no?
-- >8 -- Subject: close_tempfile(): set errno when ferror() notices a previous error
In close_tempfile(), we may notice that previous stdio operations failed when we inspect ferror(tempfile->fp). As ferror() does not set errno, and the caller of close_tempfile(), since it encountered and ignored the original error, is likely to have called other system library functions to cause errno to be modified, the caller cannot really tell anything meaningful by looking at errno after we return an error from here.
Set errno to an arbitrary value EIO when ferror() sees an error but fclose() succeeds. If fclose() fails, we just let the caller see errno from that failure.
--- tempfile.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/tempfile.c b/tempfile.c index ffcc272375..d2c6de83a9 100644 --- a/tempfile.c +++ b/tempfile.c @@ -247,8 +247,20 @@ int close_tempfile(struct tempfile *tempfile) tempfile->fd = -1; if (fp) { tempfile->fp = NULL; - err = ferror(fp); - err |= fclose(fp); + if (ferror(fp)) { + err = -1; + if (!fclose(fp)) + /* + * There was some error detected by ferror() + * but it is likely that the true errno has + * long gone. Leave something generic to make + * it clear that the caller cannot rely on errno + * at this point. + */ + errno = EIO; + } else { + err = fclose(fp); + } } else { err = close(fd); }