Re: [PATCH 3/7] read_in_full: reset errno before reading

From: Jeff King <peff@peff.net>
Date: Sep 27, 2017, 04:07 UTC
Message-ID: <20170927040721.6av4o7cjrrkzizzo@sigill.intra.peff.net>
In-Reply-To: <xmqqvak6ayk0.fsf@gitster.mtv.corp.google.com>

On Tue, Sep 26, 2017 at 01:11:59PM +0900, Junio C Hamano wrote:

Show 16 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> >> #ifndef EUNDERFLOW
> >> # ifdef ENODATA
> >> #  define EUNDERFLOW ENODATA
> >> # else
> >> #  define EUNDERFLOW ESPIPE
> >> # endif
> >> #endif
> >
> > Right, I think our mails just crossed but I'm leaning in this direction.
> 
> Hmph, I may be slow (or may be skimming the exchanges too fast), but
> what exactly is wrong with "0"?  As long as we do not have to tell
> two or more "not exactly an error from syscall in errno" cases, I
> would think "0" is the best value to use.

The main reason to avoid "0" is just that the "read error: Success" message is a bit funny.

Show 14 quoted lines

> If the syserror message _is_ the issue, then we'd need to either
> pick an existing errno that is available everywhere (with possibly
> suboptimal message), or pick something and prepare a fallback for
> platforms that lack the errno, so picking "0" as the value and use
> whatever logic we would have used for the "fallback" would not sound
> too bad.  I.e.
> 
> 	if (read_in_full(..., size) != size)
> 		if (errno)
> 			die_errno("oops");
> 		else
> 			die("short read");
> 
> If the callsite were too numerous,

You actually don't need errno for that. You can write:

  ret = read_in_full(..., size);
  if (ret < 0)
	die_errno("oops");
  else if (ret != size)
	die("short read");

So I think using errno as a sentinel value to tell between the two cases doesn't have much value.

> #define die_errno_or(msg1, msg2) if (errno) die_errno(msg1); else die(msg2)
> 
> perhaps?

If you just care about dying, you can wrap the whole read_in_full() in a helper (which is what my original patch 7 did). But I found there were cases that didn't want to die. Of course _most_ of those don't bother looking at errno in the first place. I think the only one that does is index_core(), which calls error_errno().

So with two helpers (one for die and one for error), I think we could cover the existing cases. Or we could convert the single error() case to just handle the logic inline.

One of the reasons I dislike the helper I showed earlier is that it involves assembling a lego sentence. But the alternative is requiring the caller to write both sentences from scratch (and writing a good sentence for the short read really is long: you expected X bytes but got Y).

I guess just tweaking the errno value does not really give a good "short read" error either. You get "unable to read foo: No data available", without the X/Y information.

Sometimes a custom "short read" message can be better, if it's "could not read enough bytes for packfile header" or similar. You don't need to know the exact number then.

So I dunno. Maybe I have just talked myself into actually just fixing all of the read_in_full() call sites manually.

-Peff