On Fri, Feb 17, 2017 at 02:40:19PM -0800, Junio C Hamano wrote:
> > Right. EIO is almost certainly _not_ the error we saw. But I would
> > rather consistently say "I/O error" and have the user scratch their
> > head, look up this thread, and say "ah, it was probably a deferred
> > error", as opposed to the alternative: the user sees something
> > nonsensical like ENOMEM or EBADF. Those are more misleading, and worse,
> > may change from run to run based on what other code runs or fails in
> > between.
>
> My point was actually not what errno we feed to strerror(). In that
> example, what is more misleading is the fixed part of the error
> message the caller of close_tempfile() used after seeing the funcion
> fail, i.e. "failed to close". strerror() part is used to explain
> why we "failed to close", and of course any nonsensical errno that
> we did not get from the failed stdio call would not explain it, but
> a more misleading part is that we did not even "failed to close" it.
>
> We just noticed an earlier error while attempting to close it.
> strerror() in the message does not even have to be related to the
> closing of the file handle.
Ah, I see. I think the errno thing is a strict improvement over what is
there now. Actually having a separate error message is even better, but
it does end up rather verbose in the callers.
I'm also not sure that it's all that useful to distinguish errors from
fwrite() versus fclose(). In practice, errors will come from write() in
either case, and the caller does not have much control over when the
flushing happens. So any error saying "error closing file" is probably
assuming too much anyway. It should be "error writing file".
And I think in practice the messages end up quite generic anyway, as
they are really calling commit_lock_file(), which may also fail due to a
rename. So you get something like "unable to write 'foo': ", with errno
appended. That's _also_ potentially confusing when rename() fails.
Solving that would probably require passing down an "err" strbuf (or
other data structure) for the low-level code to fill in.
> > The only reason I do not think we should do so for close_tempfile() is
> > that the fclose is typically far away from the code that actually calls
> > error(). We'd have to pass the tristate (success, fail, fail-with-errno)
> > state up through the stack (most of the calls indirectly come from
> > commit_lock_file(), I would think).
>
> We _could_ clear errno to allow caller to tell them apart, though,
> if we wanted to ;-)
Hmm. So basically "errno = 0" instead of EIO? That at least has the
advantage that you can tell it apart from a real EIO, and a caller
_could_ if they chose do:
if (commit_lock_file(&lock)) {
if (!errno)
error("error writing to file");
else
error_errno("error closing file");
}
But I am not sure I would want to annotate all such callers that way. It
would probably be less bad to just pass down a "quiet" flag or a strbuf
and have the low-level code fill it in. And that solves this problem
_and_ the rename() thing above.
But TBH, I am not sure if it is worth it. Nobody is complaining. This is
just a thing we noticed. I think setting errno to EIO or to "0" is a
strict improvement over what is there (where the errno is essentially
random) and the complexity doesn't escape the function.
So I think that "easy" thing falls far short of a solution, but it's at
least easy. I could take it or leave it at this point.
-Peff