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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sep 26, 2017, 00:13 UTC
Message-ID: <20170926001354.GN27425@aiede.mtv.corp.google.com>
In-Reply-To: <20170926000117.y3solltovyueq3zc@sigill.intra.peff.net>

Jeff King wrote:

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

>> All that really matters is the strerror() that the user would see.
>
> Agreed, though I didn't think those were necessarily standardized.

The standard allows them to vary for the sake of internationalization, but they are de facto constants (probably because of application authors like us abusing them ;-)).

[...]

Show 10 quoted lines

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

Yes. :/

Show 9 quoted lines

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

I have a basic aversion to ": Success" in error messages. Whenever I see such an error message, I stop trusting the program that produced it not to be seriously buggy. Maybe I'm the only one?

If no actual errno works, we could make a custom strerror-like function with our own custom errors (making them negative to avoid clashing with standard errno values), but this feels like overkill.

In the same spirit of misleadingly confusing too-long and too-short, there is also ERANGE ("Result too large"), which doesn't work here. There's also EPROTO "Protocol error", but that's about protocols, not file formats. More vague and therefor maybe more applicable is EBADMSG "Bad message". There's also ENOMSG "No message of the desired type".

If the goal is to support debugging, an error like EPIPE "Broken pipe" or ESPIPE "Invalid seek" would be likely to lead me in the right direction (wrong file size), even though it is misleading about how the error surfaced.

We could also avoid trying to be cute and use something generic like EIO "Input/output error".

Jonathan