Re: [PATCH] tempfile: avoid "ferror | fclose" trick

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

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