Re: Confusing git messages when disk is full.

From: Jeff King <peff@peff.net>
Date: Feb 15, 2017, 23:18 UTC
Message-ID: <20170215231832.bzg3ygz4ualcvqlc@sigill.intra.peff.net>
In-Reply-To: <xmqq37ff2hn8.fsf@gitster.mtv.corp.google.com>

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

Show 16 quoted lines

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