On Tue, Feb 14, 2017 at 05:28:36PM -0800, Junio C Hamano wrote:
> I care more about looking at errno ONLY after we saw our call to a
> system library function indicated an error, and I wanted to avoid
> doing:
>
> res = dry_run ? 0 : rmdir(path);
> saved_errno = errno;
> if (res) {
> ... do something else ...
> errno = saved_errno;
> call_something_that_uses_errno();
>
> When our call to rmdir() did not fail, or when we didn't even call
> rmdir() at all, what is in errno has nothing to do with what we are
> doing, and making a copy of it makes the code doubly confusing.
>
> Rather, I'd prefer to see:
>
> res = dry_run ? 0 : rmdir(path);
> if (res) {
> int saved_errno = errno;
> ... do something else ...
> errno = saved_errno;
> call_something_that_uses_errno();
>
> which makes it clear what is going on when reading the resulting
> code.
>
> For now, I'll queue a separate SQUASH??? and wait for others to
> comment.
I don't have a strong feeling either way, but I think your second
example there is probably preferable. The reason to save errno is
because of the "something else" that may affect it, and it puts the
saving close to that.
Duy's version above keeps the saved_errno close to the syscall that
caused it, which is nicer for making sure we're saving the right thing,
and didn't get fooled by:
res = rmdir(path);
... some other stuff ...
if (res) {
int saved_errno = errno;
... something else ...
errno = saved_errno;
That's wrong if "some other stuff" touches errno. But I think
"saved_errno" is not the right pattern there. It is "stuff away the
result _and_ errno for this thing so we can use it later".
IOW, I'd expect it to be more like:
rmdir_result = rmdir(path);
rmdir_errno = errno;
... some other stuff ...
if (rmdir_result)
show_error(rmdir_errno);
And that leads to the "gee, why don't we just encode error values as
negative integers" pattern. Which I agree is nicer, but I'm not sure I
want to get into wrapping every syscall to give it a better interface.
-Peff
