Re: [PATCH v2] clean: use warning_errno() when appropriate

Duy Nguyen [off-list ref] writes:

On Wed, Feb 15, 2017 at 1:13 AM, Junio C Hamano [off-list ref] wrote:
...

quoted

The reason to store the errno in saved_errno here is not because we
want to help code after "if (res) {...}", but the patch sent as-is
gives that impression and is confusing to the readers.

Perhaps all hunks of this patch share the same issue?  I could
locally amend, of course, but I'd like to double check before doing
so myself---perhaps you did it this way for a good reason that I am
missing?

One thing I like about putting saved_errno right next to the related
syscall is, the syscall is visible from the diff (previously some are
out of context). This is really minor though.

I agree that this is minor.

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.

Thanks.

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