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

From: Jeff King <peff@peff.net>
Date: Feb 15, 2017, 01:36 UTC
Message-ID: <20170215013620.uec3fbmrcafwas6s@sigill.intra.peff.net>
In-Reply-To: <xmqqfujg5jjv.fsf@gitster.mtv.corp.google.com>

On Tue, Feb 14, 2017 at 05:28:36PM -0800, Junio C Hamano wrote:

Show 29 quoted lines

> 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