(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@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html