Confusing git messages when disk is full.

Confusing git messages when disk is full.

[Jáchym Barvínek]

Hello, I would like to report what I consider a bug in git, I hope I'm
doing it the right way.
I was trying to run `git pull` in  my repository and got the following
error: "git pull
Your configuration specifies to merge with the ref 'refs/heads/master'
from the remote, but no such ref was fetched."
Which was very confusing to me, I found some answers to what might be the cause
but none was the right one. The actual cause was that the filesystem
had no more free space.
When I cleaned the space, `git pull` then gave the expected answer
("Already up-to-date.").
I think the message is confusing and git should be able to report to
the user that the cause
is full disk.
Regards, Jáchym Barvínek

Re: Confusing git messages when disk is full.

[Jeff King]

On Sun, Feb 12, 2017 at 05:37:30PM +0100, Jáchym Barvínek wrote:

> Hello, I would like to report what I consider a bug in git, I hope I'm
> doing it the right way.
> I was trying to run `git pull` in  my repository and got the following
> error: "git pull
> Your configuration specifies to merge with the ref 'refs/heads/master'
> from the remote, but no such ref was fetched."

It sounds like writing FETCH_HEAD failed, and git-pull became
confused that the ref wasn't fetched.

> Which was very confusing to me, I found some answers to what might be the cause
> but none was the right one. The actual cause was that the filesystem
> had no more free space.
> When I cleaned the space, `git pull` then gave the expected answer
> ("Already up-to-date.").
> I think the message is confusing and git should be able to report to
> the user that the cause
> is full disk.

If FETCH_HEAD failed to write because of a full disk (or any other
reason), then the right thing is for "git fetch" to write an error to
stderr, and git-pull should not continue the operation at all.

If we're not doing that, then that is certainly a bug.

-Peff

Re: Confusing git messages when disk is full.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Feb 12, 2017 at 05:37:30PM +0100, Jáchym Barvínek wrote:
> If FETCH_HEAD failed to write because of a full disk (or any other
> reason), then the right thing is for "git fetch" to write an error to
> stderr, and git-pull should not continue the operation at all.
>
> If we're not doing that, then that is certainly a bug.

One suspect would be store_updated_refs().  We do catch failure from
fopen("a") of FETCH_HEAD (it is truncated earlier in the code when
the --append option is not given), but all the writes go through
stdio without error checking.

I wonder if this lazy patch is sufficient.  I want to avoid having
to sprinkle

	if (fputs("\\n", fp))
		error(...);

all over the code.

 builtin/fetch.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index b5ad09d046..72347f0054 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -868,7 +868,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
  abort:
 	strbuf_release(&note);
 	free(url);
-	fclose(fp);
+	if (ferror(fp))
+		rc = -1;
+	if (fclose(fp))
+		rc = -1;
 	return rc;
 }
 

Re: Confusing git messages when disk is full.

[Jeff King]

On Wed, Feb 15, 2017 at 01:47:23PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Sun, Feb 12, 2017 at 05:37:30PM +0100, Jáchym Barvínek wrote:
> > If FETCH_HEAD failed to write because of a full disk (or any other
> > reason), then the right thing is for "git fetch" to write an error to
> > stderr, and git-pull should not continue the operation at all.
> >
> > If we're not doing that, then that is certainly a bug.
> 
> One suspect would be store_updated_refs().  We do catch failure from
> fopen("a") of FETCH_HEAD (it is truncated earlier in the code when
> the --append option is not given), but all the writes go through
> stdio without error checking.
> 
> I wonder if this lazy patch is sufficient.  I want to avoid having
> to sprinkle
> 
> 	if (fputs("\\n", fp))
> 		error(...);
> 
> all over the code.

Heh, I was just tracking down the exact same spot.

I think that yes, the lazy check-error-flag-at-the-end approach is fine
for stdio.

I tried to reproduce the original problem on a full loopback filesystem,
but got:

  fatal: update_ref failed for ref 'ORIG_HEAD': could not write to '.git/ORIG_HEAD'

I suspect you'd need the _exact_ right amount of free space to get all
of the predecessor steps done, and then run out of space right when
trying to flush the FETCH_HEAD contents.

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index b5ad09d046..72347f0054 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -868,7 +868,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>   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);

-Peff

Re: Confusing git messages when disk is full.

[Junio C Hamano]

Jeff King <peff@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 ;-)

Re: Confusing git messages when disk is full.

[Jeff King]

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

> Jeff King <peff@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

Re: Confusing git messages when disk is full.

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

I do not think we should try to do anything fancier to allow the
caller to tell ferror() and fclose() apart, as such a caller would
then need to do

	switch (xfclose(fp)) {
	case 0: /* happy */ break;
	case XFCLOSE_CLOSE: do "close failed" thing; break;
	case XFCLOSE_ERROR: do "other things failed" thing; break;
	}

and at that point, "other things failed" code would not have much to
work with to do more detailed diagnosis anyway (the errno is likely
not trustable), and it is not too much to write

	if (ferror(fp))
		do "saw some failure before" thing;
	if (fclose(fp))
		do "close failed" thing;

instead.
        

Re: Confusing git messages when disk is full.

[Jeff King]

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

> > 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).

> I do not think we should try to do anything fancier to allow the
> caller to tell ferror() and fclose() apart, as such a caller would
> then need to do

Absolutely. If they care, they can call the two separately.

You are right that errno is untrustworthy in the ferror() case, though.
Maybe that is reason not to add xfclose, and just force this caller to
do something like:

  if (ferror(fp))
	rc = error("unable to write to %s", filename);
  if (fclose(fp))
	rc = error_errno("unable to write to %s", filename);

Of course, if the earlier error persists through fclose, we'd print two
errors. This would all be much easier if the filehandles kept not just
an error bit, but the original errno. <sigh>

Maybe a not-terrible compromise is to fake the errno in the ferror case,
like:

  int xfclose(FILE *fp)
  {
	int error_flag = ferror(fp);

	/*
	 * If we get an error from fclose, the current errno value is
	 * trustworthy. But if it succeeds and we had a previous error,
	 * we need to report failure, but the value of errno could
	 * be unrelated. Make up a generic errno value.
	 */
	if (fclose(fp))
		return EOF;
	} else if (error_flag) {
		errno = EINVAL; /* or EIO? */
		return EOF;
	} else {
		return 0;
	}
  }

Or maybe that would just confuse people when they later get "invalid
argument" in the error message. I wish there was an errno value for "I
don't remember what the error was".

I dunno. This whole thing is ending up a lot more complicated than I had
hoped. I just didn't want to have to say "unable to write to %s" twice. ;)

-Peff

Re: Confusing git messages when disk is full.

[Andreas Schwab]

On Feb 15 2017, Jeff King <peff@peff.net> wrote:

> On Wed, Feb 15, 2017 at 02:50:19PM -0800, Junio C Hamano wrote:
>
>> > 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."

Re: Confusing git messages when disk is full.

[Jeff King]

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

> >> 	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

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

[Jeff King]

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:
> 
> > >> 	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 <schwab@linux-m68k.org>
Signed-off-by: Jeff King <peff@peff.net>
---
 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

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

[Michael Haggerty]

On 02/16/2017 10:31 PM, Jeff King wrote:
> 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:
>>
>>>>> 	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 <schwab@linux-m68k.org>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  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);
>  	}
> 

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

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

[Jeff King]

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

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

[Michael Haggerty]

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

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

[Jeff King]

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.

-Peff

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

[Jeff King]

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 <peff@peff.net>
---
 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

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

[Junio C Hamano]

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

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

[Jeff King]

On Fri, Feb 17, 2017 at 01:17:06PM -0800, Junio C Hamano 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.
> 
> 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.

I think our emails crossed, but our patches are obviously quite similar.
My commit message maybe explains a bit more of my thinking.

> 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?

Sure, but we have no clue what happened in between.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Feb 17, 2017 at 01:17:06PM -0800, Junio C Hamano wrote:
>
>> 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?
>
> Sure, but we have no clue what happened in between.

Hmm, so we are protecting against somebody who does "errno = 0"
explicitly, because she knows that she's dealt with the error from
stdio earlier?  Such a careful person would have called clearerr()
as well, I would guess.

So let's assume we only care about the case where some other error
was detected and errno was updated by a system library call.

> I think our emails crossed, but our patches are obviously quite similar.
> My commit message maybe explains a bit more of my thinking.

Yes, but ;-)

If we are trying to make sure that the caller would not say "failed
to close tempfile: ERRNO" with an ERRNO that is unrelated to any
stdio opration, I am not sure if the patch improves things.  The
caller did not fail to close (most likely we successfully closed
it), and no matter what futzing we do to errno, the message supplied
by such a caller will not be improved.

If the caller used "noticed an earlier error while closing tempfile:
ERRNO", such a message would describe the situation more correctly,
but then ERRNO that is not about stdio is probably acceptable in the
context of that message (the original ERRNO might be ENOSPC that is
even more specific than EIO, FWIW).  So I am not sure if the things
will improve from the status quo.

It's easy for us to either apply the patch and be done with it (or
drop and do the same), and in the bigger picture it wouldn't make
that much of a difference, I would think, so I can go either way.

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

[Jeff King]

On Fri, Feb 17, 2017 at 01:42:21PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Fri, Feb 17, 2017 at 01:17:06PM -0800, Junio C Hamano wrote:
> >
> >> 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?
> >
> > Sure, but we have no clue what happened in between.
> 
> Hmm, so we are protecting against somebody who does "errno = 0"
> explicitly, because she knows that she's dealt with the error from
> stdio earlier?  Such a careful person would have called clearerr()
> as well, I would guess.

I'm not sure I understand what you are saying here. If somebody calls
clearerr(), our ferror() handling does not trigger at all, and do not
care what is in errno either way. They can reset errno or not when they
clearerr(), but it is not relevant.

If you are asking about somebody who sets errno to "0" and _doesn't_
call clearerr(), then I don't know what that person is trying to
accomplish. Setting errno to "0" is not the right way to clear an error.
And they certainly should not be relying on it not to get overwritten
before we make it to the final ferror()/fclose().

> So let's assume we only care about the case where some other error
> was detected and errno was updated by a system library call.

Right.

> > I think our emails crossed, but our patches are obviously quite similar.
> > My commit message maybe explains a bit more of my thinking.
> 
> Yes, but ;-)
> 
> If we are trying to make sure that the caller would not say "failed
> to close tempfile: ERRNO" with an ERRNO that is unrelated to any
> stdio opration, I am not sure if the patch improves things.  The
> caller did not fail to close (most likely we successfully closed
> it), and no matter what futzing we do to errno, the message supplied
> by such a caller will not be improved.

Right. EIO is almost certainly _not_ the error we saw. But I would
rather consistently say "I/O error" and have the user scratch their
head, look up this thread, and say "ah, it was probably a deferred
error", as opposed to the alternative: the user sees something
nonsensical like ENOMEM or EBADF. Those are more misleading, and worse,
may change from run to run based on what other code runs or fails in
between.

> If the caller used "noticed an earlier error while closing tempfile:
> ERRNO", such a message would describe the situation more correctly,
> but then ERRNO that is not about stdio is probably acceptable in the
> context of that message (the original ERRNO might be ENOSPC that is
> even more specific than EIO, FWIW).  So I am not sure if the things
> will improve from the status quo.

Yes, that's I suggested that xfclose() is probably not a good direction.
The _best_ thing we can do is have the caller not report errno at all
(or even say "there was an earlier error, I have no idea what errno
was"). And xfclose() works in the opposite direction.

The only reason I do not think we should do so for close_tempfile() is
that the fclose is typically far away from the code that actually calls
error(). We'd have to pass the tristate (success, fail, fail-with-errno)
state up through the stack (most of the calls indirectly come from
commit_lock_file(), I would think).

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> If we are trying to make sure that the caller would not say "failed
>> to close tempfile: ERRNO" with an ERRNO that is unrelated to any
>> stdio opration, I am not sure if the patch improves things.  The
>> caller did not fail to close (most likely we successfully closed
>> it), and no matter what futzing we do to errno, the message supplied
>> by such a caller will not be improved.
>
> Right. EIO is almost certainly _not_ the error we saw. But I would
> rather consistently say "I/O error" and have the user scratch their
> head, look up this thread, and say "ah, it was probably a deferred
> error", as opposed to the alternative: the user sees something
> nonsensical like ENOMEM or EBADF. Those are more misleading, and worse,
> may change from run to run based on what other code runs or fails in
> between.

My point was actually not what errno we feed to strerror().  In that
example, what is more misleading is the fixed part of the error
message the caller of close_tempfile() used after seeing the funcion
fail, i.e. "failed to close".  strerror() part is used to explain
why we "failed to close", and of course any nonsensical errno that
we did not get from the failed stdio call would not explain it, but
a more misleading part is that we did not even "failed to close" it.

We just noticed an earlier error while attempting to close it.
strerror() in the message does not even have to be related to the
closing of the file handle.

>> If the caller used "noticed an earlier error while closing tempfile:
>> ERRNO", such a message would describe the situation more correctly,
>> but then ERRNO that is not about stdio is probably acceptable in the
>> context of that message (the original ERRNO might be ENOSPC that is
>> even more specific than EIO, FWIW).  So I am not sure if the things
>> will improve from the status quo.
>
> Yes, that's I suggested that xfclose() is probably not a good direction.
> The _best_ thing we can do is have the caller not report errno at all
> (or even say "there was an earlier error, I have no idea what errno
> was"). And xfclose() works in the opposite direction.

I think we are in agreement on this point ;-)

> The only reason I do not think we should do so for close_tempfile() is
> that the fclose is typically far away from the code that actually calls
> error(). We'd have to pass the tristate (success, fail, fail-with-errno)
> state up through the stack (most of the calls indirectly come from
> commit_lock_file(), I would think).

We _could_ clear errno to allow caller to tell them apart, though,
if we wanted to ;-)

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

[Jeff King]

On Fri, Feb 17, 2017 at 02:40:19PM -0800, Junio C Hamano wrote:

> > Right. EIO is almost certainly _not_ the error we saw. But I would
> > rather consistently say "I/O error" and have the user scratch their
> > head, look up this thread, and say "ah, it was probably a deferred
> > error", as opposed to the alternative: the user sees something
> > nonsensical like ENOMEM or EBADF. Those are more misleading, and worse,
> > may change from run to run based on what other code runs or fails in
> > between.
> 
> My point was actually not what errno we feed to strerror().  In that
> example, what is more misleading is the fixed part of the error
> message the caller of close_tempfile() used after seeing the funcion
> fail, i.e. "failed to close".  strerror() part is used to explain
> why we "failed to close", and of course any nonsensical errno that
> we did not get from the failed stdio call would not explain it, but
> a more misleading part is that we did not even "failed to close" it.
> 
> We just noticed an earlier error while attempting to close it.
> strerror() in the message does not even have to be related to the
> closing of the file handle.

Ah, I see.  I think the errno thing is a strict improvement over what is
there now. Actually having a separate error message is even better, but
it does end up rather verbose in the callers.

I'm also not sure that it's all that useful to distinguish errors from
fwrite() versus fclose(). In practice, errors will come from write() in
either case, and the caller does not have much control over when the
flushing happens. So any error saying "error closing file" is probably
assuming too much anyway. It should be "error writing file".

And I think in practice the messages end up quite generic anyway, as
they are really calling commit_lock_file(), which may also fail due to a
rename. So you get something like "unable to write 'foo': ", with errno
appended. That's _also_ potentially confusing when rename() fails.

Solving that would probably require passing down an "err" strbuf (or
other data structure) for the low-level code to fill in.

> > The only reason I do not think we should do so for close_tempfile() is
> > that the fclose is typically far away from the code that actually calls
> > error(). We'd have to pass the tristate (success, fail, fail-with-errno)
> > state up through the stack (most of the calls indirectly come from
> > commit_lock_file(), I would think).
> 
> We _could_ clear errno to allow caller to tell them apart, though,
> if we wanted to ;-)

Hmm. So basically "errno = 0" instead of EIO? That at least has the
advantage that you can tell it apart from a real EIO, and a caller
_could_ if they chose do:

  if (commit_lock_file(&lock)) {
	if (!errno)
		error("error writing to file");
	else
		error_errno("error closing file");
  }

But I am not sure I would want to annotate all such callers that way. It
would probably be less bad to just pass down a "quiet" flag or a strbuf
and have the low-level code fill it in. And that solves this problem
_and_ the rename() thing above.

But TBH, I am not sure if it is worth it. Nobody is complaining. This is
just a thing we noticed. I think setting errno to EIO or to "0" is a
strict improvement over what is there (where the errno is essentially
random) and the complexity doesn't escape the function.

So I think that "easy" thing falls far short of a solution, but it's at
least easy. I could take it or leave it at this point.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'm also not sure that it's all that useful to distinguish errors from
> fwrite() versus fclose(). In practice, errors will come from write() in
> either case, and the caller does not have much control over when the
> flushing happens. So any error saying "error closing file" is probably
> assuming too much anyway. It should be "error writing file".

Yes.

>> We _could_ clear errno to allow caller to tell them apart, though,
>> if we wanted to ;-)
>
> Hmm. So basically "errno = 0" instead of EIO? That at least has the
> advantage that you can tell it apart from a real EIO, and a caller
> _could_ if they chose do:
>
>   if (commit_lock_file(&lock)) {
> 	if (!errno)
> 		error("error writing to file");
> 	else
> 		error_errno("error closing file");
>   }

Exactly.

> But I am not sure I would want to annotate all such callers that way. It
> would probably be less bad to just pass down a "quiet" flag or a strbuf
> and have the low-level code fill it in. And that solves this problem
> _and_ the rename() thing above.
>
> But TBH, I am not sure if it is worth it. Nobody is complaining. This is
> just a thing we noticed. I think setting errno to EIO or to "0" is a
> strict improvement over what is there (where the errno is essentially
> random) and the complexity doesn't escape the function.
>
> So I think that "easy" thing falls far short of a solution, but it's at
> least easy. I could take it or leave it at this point.

Well, I already said that earlier in this thread, and ended up
queuing your patch on 'pu' ;-).

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

[Jeff King]

On Fri, Feb 17, 2017 at 03:52:36PM -0800, Junio C Hamano wrote:

> > So I think that "easy" thing falls far short of a solution, but it's at
> > least easy. I could take it or leave it at this point.
> 
> Well, I already said that earlier in this thread, and ended up
> queuing your patch on 'pu' ;-).

We're slowly converging on consensus in our apathy. ;)

-Peff