Re: [PATCH 1/4] error: save and restore errno

From: Jeff King <peff@peff.net>
Date: Nov 19, 2014, 18:28 UTC
Message-ID: <20141119182812.GA9425@peff.net>
In-Reply-To: <xmqqvbmbrrba.fsf@gitster.dls.corp.google.com>

On Wed, Nov 19, 2014 at 10:14:17AM -0800, Junio C Hamano wrote:

Show 37 quoted lines

> >> What the above doesn't explain is why the caller cares about errno.
> >> Are they going to print another message with strerror(errno)?  Or are
> >> they going to consider some errors non-errors (like ENOENT when trying
> >> to unlink a file), in which case why is printing a message to stderr
> >> okay?
> >
> > I guess the unsaid bit is:
> >
> >   Unfortunately this may clobber the errno from the open() call. Even
> >   though error() sees the correct errno, the caller to which we are
> >   returning may see a bogus errno value.
> >
> I am not sure if that answers the question asked.
> 
> If you have
> 
> 	int frotz(...) {
> 		int fd = open(...);
>         	if (fd < 0)
>                 	return error("open failed (%s)", strerror(errno));
> 		return fd;
> 	}
> 
> and the caller calls it and cares about the errno from this open,
> what does the caller do?  Jonathan's worried about a codepath that
> may be familiar to us as we recently saw a patch similar to it:
> 
> 	int fd = frotz(...);
>         if (fd < 0) {
>         	if (errno == ENOENT || errno == EISDIR)
>                 	; /* not quite an error */
> 		else
> 			exit(1);
> 	}
> 
> If ENOENT/EISDIR is expected and a non-error, it is not useful for
> frotz() to give an error message on its own.

Sure, but isn't that out-of-scope for this patch? We know that some functions _are_ calling error() and taking care to keep errno valid, and we would prefer to do that with less work.

If you are arguing "there are literally zero cases where that makes sense; functions should either complain themselves, _or_ they should pass on a valid errno so the caller can decide whether to complain, but doing both is a recipe for pointless and unwanted error messages", then this patch is counter-productive (and we should be fixing those call sites of error() instead).

But I am not sure that there are zero legitimate cases. Just as a hypothetical, imagine you wanted to complain to stderr _and_ propagate the error value into a specific exit code. You'd want something like:

  fd = frotz(...);
  error("frotz failed (%s)", strerror(errno));
  exit(errno == ENOENT ? 1 : 2);

Granted, I do not think we do that particular pattern in git, but we do take pains to save errno across some error() calls. I do not know which of those are legitimate and which should be refactored. In the meantime, I do not think this patch makes anything worse.

> I think a more appropriate answer to Jonathan's question is why is
> the callee (i.e. frotz()) calling error() in the first place if an
> unconditional error message is an issue.

Exactly. It is not error()'s business to know what the caller wants to do, or whether or why it cares about retaining errno. But error(), since it is designed to be called in error codepaths, should aim to be minimally intrusive.

-Peff