Re: Confusing git messages when disk is full.

On Feb 15 2017, Jeff King [off-list ref] wrote:

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

quoted

quoted

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.

Sure.  I am happy to add something like this:

	/*
	 * closes a FILE *, returns 0 if closing and all the
	 * previous stdio operations on fp were successful,
	 * otherwise non-zero.
	 */
	int xfclose(FILE *fp)
	{
		return ferror(fp) | fclose(fp);
	}

Yes, that's exactly what I had in mind (might be worth calling out the
bitwise-OR, though, just to make it clear it's not a typo).

Since the order of evaluation is unspecified, it would be better to
force sequencing ferror before fclose.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

On Thu, Feb 16, 2017 at 11:10:18AM +0100, Andreas Schwab wrote:

quoted

quoted

	int xfclose(FILE *fp)
	{
		return ferror(fp) | fclose(fp);
	}

Yes, that's exactly what I had in mind (might be worth calling out the
bitwise-OR, though, just to make it clear it's not a typo).

Since the order of evaluation is unspecified, it would be better to
force sequencing ferror before fclose.

Good point. Arguably the call in tempfile.c is buggy.

-Peff

On Thu, Feb 16, 2017 at 11:43:59AM -0500, Jeff King wrote:

On Thu, Feb 16, 2017 at 11:10:18AM +0100, Andreas Schwab wrote:

quoted

quoted

quoted

	int xfclose(FILE *fp)
	{
		return ferror(fp) | fclose(fp);
	}

Yes, that's exactly what I had in mind (might be worth calling out the
bitwise-OR, though, just to make it clear it's not a typo).

Since the order of evaluation is unspecified, it would be better to
force sequencing ferror before fclose.

Good point. Arguably the call in tempfile.c is buggy.

Here's a fix.

I think close_tempfile() suffers from the same errno problem discussed
earlier in this thread (i.e., that after calling it, you may get an
error return with a random, unrelated errno value if ferror() failed but
fclose() did not).

-- >8 --
Subject: [PATCH] tempfile: avoid "ferror | fclose" trick

The current code wants to record an error condition from
either ferror() or fclose(), but makes sure that we always
call both functions. So it can't use logical-OR "||", which
would short-circuit when ferror() is true. Instead, it uses
bitwise-OR "|" to evaluate both functions and set one or
more bits in the "err" flag if they reported a failure.

Unlike logical-OR, though, bitwise-OR does not introduce a
sequence point, and the order of evaluation for its operands
is unspecified. So a compiler would be free to generate code
which calls fclose() first, and then ferror() on the
now-freed filehandle.

There's no indication that this has happened in practice,
but let's write it out in a way that follows the standard.

Noticed-by: Andreas Schwab [off-list ref]
Signed-off-by: Jeff King <redacted>
---
 tempfile.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/tempfile.c b/tempfile.c
index 2990c9242..ffcc27237 100644
--- a/tempfile.c
+++ b/tempfile.c

@@ -247,12 +247,8 @@ int close_tempfile(struct tempfile *tempfile)
 	tempfile->fd = -1;
 	if (fp) {
 		tempfile->fp = NULL;
-
-		/*
-		 * Note: no short-circuiting here; we want to fclose()
-		 * in any case!
-		 */
-		err = ferror(fp) | fclose(fp);
+		err = ferror(fp);
+		err |= fclose(fp);
 	} else {
 		err = close(fd);
 	}

-- 
2.12.0.rc1.559.gd292418bf

On 02/16/2017 10:31 PM, Jeff King wrote:

Thanks for fixing this; the old code was definitely wrong.

As you pointed out, if ferror() fails, it doesn't set errno properly. At
least one caller tries to strerror(errno), so it would probably be good
to store *something* in there, probably EIO.

To be really pedantic, there's also the question of what errno the
caller would want if *both* ferror() and fclose() fail. Normally I would
say "the first error that occurred", but in this case we don't know the
correct errno from the error reported by ferror(), so maybe the fclose()
errno is more likely to hint at the underlying reason for the failure.

So I (reluctantly) propose

	if (ferror(fp)) {
		if (!fclose(fp)) {
			/*
			 * ferror() doesn't set errno, so we have to
			 * set one. (By contrast, when fclose() fails
			 * too, we leave *its* errno in place.)
			 */
			errno = EIO;
		}
		return -1;
	}
	return fclose();

Michael

On Fri, Feb 17, 2017 at 09:00:09AM +0100, Michael Haggerty wrote:

As you pointed out, if ferror() fails, it doesn't set errno properly. At
least one caller tries to strerror(errno), so it would probably be good
to store *something* in there, probably EIO.

Yeah, we discussed this up-thread a bit, and my "solution" was similar
to yours. I don't like it, because EIO is a real thing that can happen,
too, and it would certainly be surprising to a user to see. But it's
probably better than the alternative, which is getting whatever random
value happened to be in errno.

The only downside is that if the value of errno _was_ valid (because the
last thing you did really was writing to the filehandle, then we'd
overwrite it).

To be really pedantic, there's also the question of what errno the
caller would want if *both* ferror() and fclose() fail. Normally I would
say "the first error that occurred", but in this case we don't know the
correct errno from the error reported by ferror(), so maybe the fclose()
errno is more likely to hint at the underlying reason for the failure.

Yes, I think we're better to take what fclose gives us, if we can.

So I (reluctantly) propose

	if (ferror(fp)) {
		if (!fclose(fp)) {
			/*
			 * ferror() doesn't set errno, so we have to
			 * set one. (By contrast, when fclose() fails
			 * too, we leave *its* errno in place.)
			 */
			errno = EIO;
		}
		return -1;
	}
	return fclose();

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.

-Peff

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.

Michael

On Fri, Feb 17, 2017 at 11:42:25AM +0100, Michael Haggerty wrote:

On 02/17/2017 09:07 AM, Jeff King wrote:

quoted

[...]
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.

-Peff

On Fri, Feb 17, 2017 at 03:54:42PM -0500, Jeff King wrote:

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.

So here's that patch with a justification.

At this point, this snippet of code would be appropriate to pull into
xfclose() if we wanted. But possibly that is the wrong direction, as it
encourages callers to do:

  if (xfclose(fp))
	err = error_errno("failure writing to ...");

when they could do:

  if (ferror(fp))
	err = error("failure writing to ...");
  if (fclose(fp))
        err = error_errno("failure writing to ...");

While longer, it's arguably better for them to distinguish the two
cases. It's only worth doing the errno magic when the close is deep
inside a callstack, and passing out the two cases is awkward.

-- >8 --
Subject: tempfile: set errno to a known value before calling ferror()

In close_tempfile(), we return an error if ferror()
indicated a previous failure, or if fclose() failed. In the
latter case, errno is set and it is useful for callers to
report it.

However, if _only_ ferror() triggers, then the value of
errno is based on whatever syscall happened to last fail,
which may not be related to our filehandle at all. A caller
cannot tell the difference between the two cases, and may
use "die_errno()" or similar to report a nonsense errno value.

One solution would be to actually pass back separate return
values for the two cases, so a caller can write a more
appropriate message for each case. But that makes the
interface clunky.

Instead, let's just set errno to the generic EIO in this case.
That's not as descriptive as we'd like, but at least it's
predictable. So it's better than the status quo in all cases
but one: when the last syscall really did involve a failure
on our filehandle, we'll be wiping that out. But that's a
fragile thing for us to rely on.

In any case, we'll let the errno result from fclose() take
precedence over our value, as we know that's recent and
accurate (and many I/O errors will persist through the
fclose anyway).

Signed-off-by: Jeff King <redacted>
---
 tempfile.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tempfile.c b/tempfile.c
index ffcc27237..684371067 100644
--- a/tempfile.c
+++ b/tempfile.c

@@ -247,8 +247,13 @@ 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))
+				errno = EIO;
+		} else {
+			err = fclose(fp);
+		}
 	} else {
 		err = close(fd);
 	}

-- 
2.12.0.rc1.612.ga5f664feb