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

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

Jeff King [off-list ref] writes:

quoted

quoted

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

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