Re: [PATCH 1/4] error: save and restore errno
Jeff King p...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] error: save and restore errno
On Wed, Nov 19, 2014 at 10:14:17AM -0800, Junio C Hamano wrote:
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/4] error: save and restore errno
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. Let's teach error() to save and restore
errno in each call, so that the original errno is preserved.
This is slightly less efficient for callers which do not
care, but error code paths are generally not performance
critical anyway.
Signed-off-by: Jeff King p...@peff.net
---
It's pretty minor to just handle errno in the callers, but I feel like
I've wanted this at least a dozen times, and it seems like it cannot
possibly hurt (i.e., I imagine there are callers where we _should_ be
doing the errno dance but have not realized it).
usage.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/usage.c b/usage.c
index ed14645..ee44d57 100644
--- a/usage.c
+++ b/usage.c
@@ -142,10 +142,13 @@ void NORETURN die_errno(const char *fmt, ...)
int error(const char *err, ...)
{
va_list params;
+ int saved_errno = errno;
va_start(params, err);
error_routine(err, params);
va_end(params);
+
+ errno = saved_errno;
return -1;
}
--
2.1.2.596.g7379948
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] error: save and restore errno
This one makes my day.
A really good fix as it minimizes maintenance burden for checking
incoming patches for that pattern.
Reviewed-by: Stefan Beller sbel...@google.com
On Tue, Nov 18, 2014 at 5:37 PM, Jeff King p...@peff.net 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. Let's teach error() to save and restore
errno in each call, so that the original errno is preserved.
This is slightly less efficient for callers which do not
care, but error code paths are generally not performance
critical anyway.
Signed-off-by: Jeff King p...@peff.net
---
It's pretty minor to just handle errno in the callers, but I feel like
I've wanted this at least a dozen times, and it seems like it cannot
possibly hurt (i.e., I imagine there are callers where we _should_ be
doing the errno dance but have not realized it).
usage.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/usage.c b/usage.c
index ed14645..ee44d57 100644
--- a/usage.c
+++ b/usage.c
@@ -142,10 +142,13 @@ void NORETURN die_errno(const char *fmt, ...)
int error(const char *err, ...)
{
va_list params;
+ int saved_errno = errno;
va_start(params, err);
error_routine(err, params);
va_end(params);
+
+ errno = saved_errno;
return -1;
}
--
2.1.2.596.g7379948
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] error: save and restore errno
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?
All that said, given that there are real examples of code already
doing this, the patch seems sane.
[...]
usage.c | 3 +++
1 file changed, 3 insertions(+)
Reviewed-by: Jonathan Nieder jrnie...@gmail.com
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] error: save and restore errno
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
