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

From: Jeff King <peff@peff.net>
Date: Feb 17, 2017, 08:07 UTC
Message-ID: <20170217080759.2357wzdiuymcyosw@sigill.intra.peff.net>
In-Reply-To: <923e328c-7fea-a9e4-1059-3bd6b8e58164@alum.mit.edu>

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

Show 5 quoted lines

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

Show 14 quoted lines

> 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