public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Junio C Hamano <gitster@pobox•com>
To: Jeff King <peff@peff•net>
Cc: Jonathan Nieder <jrnieder@gmail•com>,
	Stefan Beller <sbeller@google•com>,
	sahlberg@google•com, git@vger•kernel.org
Subject: Re: [PATCH 1/4] error: save and restore errno
Date: Wed, 19 Nov 2014 10:14:17 -0800	[thread overview]
Message-ID: <xmqqvbmbrrba.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <20141119014722.GB2337@peff.net> (Jeff King's message of "Tue, 18 Nov 2014 20:47:23 -0500")

Jeff King <peff@peff•net> writes:

> On Tue, Nov 18, 2014 at 05:43:44PM -0800, Jonathan Nieder wrote:
>
>> Jeff King wrote:
>> 
>> > It's common to use error() to return from a function, like:
>> >
>> > 	if (open(...) < 0)
>> > 		return error("open failed");
>> >
>> > Unfortunately this may clobber the errno from the open()
>> > call. So we often end up with code like this:
>> >
>> >         if (open(...) < 0) {
>> > 		int saved_errno = errno;
>> > 		error("open failed");
>> > 		errno = saved_errno;
>> > 		return -1;
>> > 	}
>> >
>> > which is less nice.
>> 
>> 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.
>
> -Peff

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.

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.

next prev parent reply	other threads:[~2014-11-19 18:14 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-11-18 23:17 [PATCH] refs.c: handle locking failure during transaction better Stefan Beller
2014-11-18 23:34 ` Stefan Beller
2014-11-19  1:13   ` Stefan Beller
2014-11-19  1:35     ` [PATCH 0/4] error cleanups in lock_ref_sha1_basic Jeff King
2014-11-19  1:37       ` [PATCH 1/4] error: save and restore errno Jeff King
2014-11-19  1:41         ` Stefan Beller
2014-11-19  1:43         ` Jonathan Nieder
2014-11-19  1:47           ` Jeff King
2014-11-19 18:14             ` Junio C Hamano [this message]
2014-11-19 18:28               ` Jeff King
2014-11-19  1:37       ` [PATCH 2/4] lock_ref_sha1_basic: simplify errno handling Jeff King
2014-11-19  1:54         ` Jonathan Nieder
2014-11-21  9:25         ` Michael Haggerty
2014-11-19  1:37       ` [PATCH 3/4] lock_ref_sha1_basic: simplify error code path Jeff King
2014-11-19  2:00         ` Jonathan Nieder
2014-11-19  2:04           ` Jeff King
2014-11-19  2:07             ` Jonathan Nieder
2014-11-19 21:41               ` Junio C Hamano
2014-11-19 22:28               ` Jeff King
2014-11-19 22:34                 ` Junio C Hamano
2014-11-19 22:36                   ` Jeff King
2014-11-20  1:07                 ` Jonathan Nieder
2014-11-19  1:41       ` [PATCH 4/4] lock_ref_sha1_basic: do not die on locking errors Jeff King
2014-11-19  2:05         ` Jonathan Nieder

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqvbmbrrba.fsf@gitster.dls.corp.google.com \
    --to=gitster@pobox$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=jrnieder@gmail$(echo .)com \
    --cc=peff@peff$(echo .)net \
    --cc=sahlberg@google$(echo .)com \
    --cc=sbeller@google$(echo .)com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox