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

From: Jeff King <peff@peff.net>
Date: Sep 26, 2017, 00:01 UTC
Message-ID: <20170926000117.y3solltovyueq3zc@sigill.intra.peff.net>
In-Reply-To: <20170925235510.GL27425@aiede.mtv.corp.google.com>

On Mon, Sep 25, 2017 at 04:55:10PM -0700, Jonathan Nieder wrote:

Show 13 quoted lines

> Jeff King wrote:
> 
> >   [EOVERFLOW]
> >     The file is a regular file, nbyte is greater than 0, the starting
> >     position is before the end-of-file, and the starting position is
> >     greater than or equal to the offset maximum established in the open
> >     file description associated with fildes.
> >
> > That's not _exactly_ what's going on here, but it's pretty close. And is
> > what you would get if you implemented read_exactly() in terms of
> > something like pread().
> 
> All that really matters is the strerror() that the user would see.

Agreed, though I didn't think those were necessarily standardized.

> For EOVERFLOW, that is
> 
> 	Value too large to be stored in data type

Interestingly that does not seem to be a good description for the case POSIX describes above.

Show 6 quoted lines

> For EILSEQ, it is
> 
> 	Illegal byte sequence
> 
> Neither is perfect, but the latter seems like a better fit for the kind
> of file format errors we're describing here.

I find that one actively misleading. It's not the bytes we saw, it's the lack of them.

> Of course, it's even
> better if we fix the callers and don't try to wedge this into errno.

Yes. This patch is just a stop-gap. Perhaps we should abandon it entirely. But I couldn't find a way to fix all the callers. If you have a function that just returns "-1" when it sees a read_in_full() error, how does _its_ caller tell the difference?

I guess the answer is that the interface of the sub-function calling read_in_full() needs to change.

> If you are okay with the too-long/too-short confusion in EOVERFLOW, then
> there is EMSGSIZE:
> 
> 	Message too long

I don't really like that one either.

I actually prefer "0" of the all of the options discussed. At least it is immediately clear that it is not a syscall error.

-Peff