Re: [PATCH] copy.c: make copy_fd preserve meaningful errno

[Jonathan Nieder <jrnieder@gmail.com>]

(meta-comment: please snip out the context you are not responding to,
 to make reading easier)
Stefan Beller wrote:
> On Mon, Nov 17, 2014 at 3:35 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Stefan Beller wrote:

>>> Update copy_fd to return a meaningful errno on failure and also
>>> preserve the existing errno variable.
>>
>> Some functions in git make errno meaningful on error and others don't.
>> In general, the more we only use errno immediately after a system
>> call, the better, so based on the above description this seems like a
>> step in the wrong direction.
>
> I did reword the commit message badly. Before it just read
> "Update copy_fd to return a meaningful errno".
>
> In fact the proposed patch doesn't guarantee the errno, which is set
> at the beginning of the function to be the same at the end.
>
> What it really should preserve is the errno from xread, while
> evaluating error("copy-fd: read returned %s", strerror(errno));
> So the function call of error(...) or strerror(...) may change the errno.

A successful call to strerror() is guaranteed not to change errno, but
a call to error() does I/O so it can clobber errno.

The basic question is whether errno is and should be part of
copy_fd()'s contract.  Until v2.2.0-rc0~53^2~2 (2014-10-01), it
wasn't.  Even after that change, there's no user-visible effect to
clobbering errno (so this patch is about maintainability, as opposed
to fixing a user-visible bad behavior).

[...]
>> Can we do better?  E.g., if the signature were
>>
>>         int copy_fd(int ifd, int ofd, struct strbuf *err);
>>
>> then we could write the error message to the err strbuf for the
>> caller to print.  The error handling would be more explicit so
>> there would be no need to protect errno from clobbering by other
>> system calls (both here and in callers).
>>
>> Something like this:
>
> I like this approach, though your patch is not about the original
> intention as this one
> (having strerror(...) not messing with the errno), but rather
> accumulating the errors not
> in numbers but string buffers?

After this patch, setting errno is not part of the contract of
copy_fd, so the bug Ronnie was fixing is gone.

But it's a little more invasive.  What do you think?

Thanks,
Jonathan