Re: [PATCH] copy.c: make copy_fd preserve meaningful errno
On 11/21/2014 10:14 AM, Michael Haggerty wrote: Couldn't we save ourselves a lot of this save_errno boilerplate by making error() and warning() preserve errno? [...] Never mind; I see that Peff already submitted a patch to this effect. Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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] copy.c: make copy_fd preserve meaningful errno
Michael Haggerty mhag...@alum.mit.edu writes: On 11/21/2014 10:14 AM, Michael Haggerty wrote: Couldn't we save ourselves a lot of this save_errno boilerplate by making error() and warning() preserve errno? [...] Never mind; I see that Peff already submitted a patch to this effect. My understanding of the conclusion of those four patches was that only a single updated one is needed, and moving save/restore inside error() did not have to survive. http://article.gmane.org/gmane.comp.version-control.git/259911 -- 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] copy.c: make copy_fd preserve meaningful errno
On Fri, Nov 21, 2014 at 09:48:19AM -0800, Junio C Hamano wrote: Michael Haggerty mhag...@alum.mit.edu writes: On 11/21/2014 10:14 AM, Michael Haggerty wrote: Couldn't we save ourselves a lot of this save_errno boilerplate by making error() and warning() preserve errno? [...] Never mind; I see that Peff already submitted a patch to this effect. My understanding of the conclusion of those four patches was that only a single updated one is needed, and moving save/restore inside error() did not have to survive. http://article.gmane.org/gmane.comp.version-control.git/259911 Yeah, the callsite I had intended ended up needing to save it across more than just error(). And I think that is probably why we have never done any errno-handling inside error() before (it is certainly not the first time I thought of doing such a thing): real-world cases tend to be a little more complicated. That being said, if this copy() case is one that could benefit, I do not have any problem with picking up (or just reusing the concept of) my 1/4 from that series. It probably does not _hurt_ anything, and if it can help in even a few cases, it may be worth it. -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
Re: [PATCH] copy.c: make copy_fd preserve meaningful errno
Jeff King p...@peff.net writes: On Fri, Nov 21, 2014 at 09:48:19AM -0800, Junio C Hamano wrote: Michael Haggerty mhag...@alum.mit.edu writes: On 11/21/2014 10:14 AM, Michael Haggerty wrote: Couldn't we save ourselves a lot of this save_errno boilerplate by making error() and warning() preserve errno? [...] Never mind; I see that Peff already submitted a patch to this effect. My understanding of the conclusion of those four patches was that only a single updated one is needed, and moving save/restore inside error() did not have to survive. http://article.gmane.org/gmane.comp.version-control.git/259911 Yeah, the callsite I had intended ended up needing to save it across more than just error(). And I think that is probably why we have never done any errno-handling inside error() before (it is certainly not the first time I thought of doing such a thing): real-world cases tend to be a little more complicated. That being said, if this copy() case is one that could benefit, I do not have any problem with picking up (or just reusing the concept of) my 1/4 from that series. It probably does not _hurt_ anything, and if it can help in even a few cases, it may be worth it. Yeah, I agree. My summary was did not have to survive, not had to die. Like you, I do not mind the change as long as other code paths benefit from it. -- 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] copy.c: make copy_fd preserve meaningful errno
Jonathan Nieder jrnie...@gmail.com writes: Do any callers care about errno? Does the function's API documentation say it will make errno meaningful on error, so people making changes to copy_fd in the future know to maintain that property? *searches* Looks like callers are: convert.c::filter_buffer_or_fd, which doesn't care copy.c::copy_file, which also doesn't care lockfile.c::hold_lock_file_for_append, which started caring in order to propagate errno in v2.2.0-rc0~53^2~2 (restore errno before returning, 2014-10-01). But no callers of that function care yet. So this is about fixing a bug-waiting-to-happen in hold_lock_file_for_append. That would be enough to motivate the change. OK. Perhaps convert.c wants to be fixed? +int save_errno = errno; +error(copy-fd: read returned %s, strerror(errno)); +errno = save_errno; +return -1; Any caller is presumably going to turn around and print strerror(errno) again, producing repetitive output. Can we do better? E.g., if the signature were int copy_fd(int ifd, int ofd, struct strbuf *err); then we could write the error message to the err strbuf for the caller to print. The problem you are solving is because the caller may want to do its own error message, stop the callee from emitting the error message unconditionally, but if we are addressing the caller may want to..., I think we should find a single solution that addresses other kind fo things the caller may want to do. For example, two callers may want to phrase the same error condition differently, e.g. depending on what the user asked to do. We'd want something better than the ERRORMSG trick used in unpack-trees.c, which does not scale, and I think passing some data that represents here is how the caller wants the errors to be handled and presented is probably a part of the solution, but strbuf *err is not that. In short I am not a very big fan of passing around strbuf *err to low level helper API functions myself. But the approach does not make things much worse than it currently is, other than code churns to pass an extra pointer around. -- 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] copy.c: make copy_fd preserve meaningful errno
Junio C Hamano gits...@pobox.com writes: In short I am not a very big fan of passing around strbuf *err to low level helper API functions myself. But the approach does not make things much worse than it currently is, other than code churns to pass an extra pointer around. Sorry I left the conclusion out of the message. As it does not make things much worse, and does give slightly better flexibility on error message emission to the callers, let's go with the strbuf *err arpporach for now. Until we hit a wall we cannot climb over, at which time we may need to redo it, but let's first see how it goes. -- 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] copy.c: make copy_fd preserve meaningful errno
From: Ronnie Sahlberg sahlb...@google.com
Update copy_fd to return a meaningful errno on failure and also
preserve the existing errno variable.
Signed-off-by: Ronnie Sahlberg sahlb...@google.com
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
Signed-off-by: Stefan Beller sbel...@google.com
---
As announced in [1], I'm going to take over the
ref-transactions-reflog series by Ronnie Sahlberg.
This patch was sent previously to the list as part of
that series[2], but it seems to be unrelated to me.
Thanks,
Stefan
[1] http://www.mail-archive.com/git@vger.kernel.org/msg61051.html
[2] http://www.spinics.net/lists/git/msg240784.html
copy.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/copy.c b/copy.c
index f2970ec..a8d366e 100644
--- a/copy.c
+++ b/copy.c
@@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)
if (!len)
break;
if (len 0) {
- return error(copy-fd: read returned %s,
-strerror(errno));
+ int save_errno = errno;
+ error(copy-fd: read returned %s, strerror(errno));
+ errno = save_errno;
+ return -1;
+ }
+ if (write_in_full(ofd, buffer, len) 0) {
+ int save_errno = errno;
+ error(copy-fd: write returned %s, strerror(errno));
+ errno = save_errno;
+ return -1;
}
- if (write_in_full(ofd, buffer, len) 0)
- return error(copy-fd: write returned %s,
-strerror(errno));
}
return 0;
}
--
2.2.0.rc1.24.g562add4
--
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] copy.c: make copy_fd preserve meaningful errno
Stefan Beller sbel...@google.com writes:
This patch was sent previously to the list as part of
that series[2], but it seems to be unrelated to me.
I am fine to queue obvious and trivial bits first before the larger
main course. For now I'll queue this one and also the series that
has been queued for a while, but at some point I suspect we would
have to drop the latter.
Thanks.
[1] http://www.mail-archive.com/git@vger.kernel.org/msg61051.html
[2] http://www.spinics.net/lists/git/msg240784.html
copy.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/copy.c b/copy.c
index f2970ec..a8d366e 100644
--- a/copy.c
+++ b/copy.c
@@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)
if (!len)
break;
if (len 0) {
- return error(copy-fd: read returned %s,
- strerror(errno));
+ int save_errno = errno;
+ error(copy-fd: read returned %s, strerror(errno));
+ errno = save_errno;
+ return -1;
+ }
+ if (write_in_full(ofd, buffer, len) 0) {
+ int save_errno = errno;
+ error(copy-fd: write returned %s, strerror(errno));
+ errno = save_errno;
+ return -1;
}
- if (write_in_full(ofd, buffer, len) 0)
- return error(copy-fd: write returned %s,
- strerror(errno));
}
return 0;
}
--
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] copy.c: make copy_fd preserve meaningful errno
I am reviewing the series and about to resend it with very minor nits
fixed. I just want to point out this fix is orthogonal to the series
and can be picked up no matter how long the reviewing/discussion of
the series goes.
Thanks,
Stefan
On Mon, Nov 17, 2014 at 3:08 PM, Junio C Hamano gits...@pobox.com wrote:
Stefan Beller sbel...@google.com writes:
This patch was sent previously to the list as part of
that series[2], but it seems to be unrelated to me.
I am fine to queue obvious and trivial bits first before the larger
main course. For now I'll queue this one and also the series that
has been queued for a while, but at some point I suspect we would
have to drop the latter.
Thanks.
[1] http://www.mail-archive.com/git@vger.kernel.org/msg61051.html
[2] http://www.spinics.net/lists/git/msg240784.html
copy.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/copy.c b/copy.c
index f2970ec..a8d366e 100644
--- a/copy.c
+++ b/copy.c
@@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)
if (!len)
break;
if (len 0) {
- return error(copy-fd: read returned %s,
- strerror(errno));
+ int save_errno = errno;
+ error(copy-fd: read returned %s, strerror(errno));
+ errno = save_errno;
+ return -1;
+ }
+ if (write_in_full(ofd, buffer, len) 0) {
+ int save_errno = errno;
+ error(copy-fd: write returned %s, strerror(errno));
+ errno = save_errno;
+ return -1;
}
- if (write_in_full(ofd, buffer, len) 0)
- return error(copy-fd: write returned %s,
- strerror(errno));
}
return 0;
}
--
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] copy.c: make copy_fd preserve meaningful errno
Hi,
Stefan Beller wrote:
This patch was sent previously to the list as part of
that series[2], but it seems to be unrelated to me.
Thanks. Good call.
[...]
From: Ronnie Sahlberg sahlb...@google.com
Update copy_fd to return a meaningful errno on failure and also
preserve the existing errno variable.
Some functions in git make errno meaningful on error and others don't.
In general, the more we only use errno immediately after a system
call, the better, so based on the above description this seems like a
step in the wrong direction.
Do any callers care about errno? Does the function's API
documentation say it will make errno meaningful on error, so people
making changes to copy_fd in the future know to maintain that
property?
*searches*
Looks like callers are:
convert.c::filter_buffer_or_fd, which doesn't care
copy.c::copy_file, which also doesn't care
lockfile.c::hold_lock_file_for_append, which started caring
in order to propagate errno in v2.2.0-rc0~53^2~2 (restore errno
before returning, 2014-10-01). But no callers of that function
care yet.
So this is about fixing a bug-waiting-to-happen in
hold_lock_file_for_append. That would be enough to motivate the
change.
[...]
copy.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
Please also update the API documentation in cache.h so we remember not
to backslide in the future.
[...]
--- a/copy.c
+++ b/copy.c
@@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)
if (!len)
break;
if (len 0) {
- return error(copy-fd: read returned %s,
- strerror(errno));
+ int save_errno = errno;
+ error(copy-fd: read returned %s, strerror(errno));
+ errno = save_errno;
+ return -1;
Any caller is presumably going to turn around and print strerror(errno)
again, producing repetitive output.
Can we do better? E.g., if the signature were
int copy_fd(int ifd, int ofd, struct strbuf *err);
then we could write the error message to the err strbuf for the
caller to print. The error handling would be more explicit so
there would be no need to protect errno from clobbering by other
system calls (both here and in callers).
Something like this:
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
diff --git i/cache.h w/cache.h
index 99ed096..ddaa30f 100644
--- i/cache.h
+++ w/cache.h
@@ -1479,7 +1479,7 @@ extern const char *git_mailmap_blob;
extern void maybe_flush_or_die(FILE *, const char *);
__attribute__((format (printf, 2, 3)))
extern void fprintf_or_die(FILE *, const char *fmt, ...);
-extern int copy_fd(int ifd, int ofd);
+extern int copy_fd(int ifd, int ofd, struct strbuf *err);
extern int copy_file(const char *dst, const char *src, int mode);
extern int copy_file_with_time(const char *dst, const char *src, int mode);
extern void write_or_die(int fd, const void *buf, size_t count);
diff --git i/convert.c w/convert.c
index 9a5612e..e301447 100644
--- i/convert.c
+++ w/convert.c
@@ -358,7 +358,11 @@ static int filter_buffer_or_fd(int in, int out, void *data)
if (params-src) {
write_err = (write_in_full(child_process.in, params-src,
params-size) 0);
} else {
- write_err = copy_fd(params-fd, child_process.in);
+ struct strbuf err = STRBUF_INIT;
+ write_err = copy_fd(params-fd, child_process.in, err);
+ if (write_err)
+ error(copy-fd: %s, err.buf);
+ strbuf_release(err);
}
if (close(child_process.in))
diff --git i/copy.c w/copy.c
index f2970ec..828661a 100644
--- i/copy.c
+++ w/copy.c
@@ -1,19 +1,22 @@
#include cache.h
-int copy_fd(int ifd, int ofd)
+int copy_fd(int ifd, int ofd, struct strbuf *err)
{
+ assert(err);
+
while (1) {
char buffer[8192];
ssize_t len = xread(ifd, buffer, sizeof(buffer));
if (!len)
break;
if (len 0) {
- return error(copy-fd: read returned %s,
-strerror(errno));
+ strbuf_addf(err, read returned %s, strerror(errno));
+ return -1;
+ }
+ if (write_in_full(ofd, buffer, len) 0) {
+ strbuf_addf(err, write returned %s, strerror(errno));
+ return -1;
}
- if (write_in_full(ofd, buffer, len) 0)
- return error(copy-fd: write returned %s,
-strerror(errno));
}
return 0;
}
@@ -33,7 +36,8 @@ static int copy_times(const char *dst, const char *src)
int copy_file(const char *dst, const char *src, int mode)
{
- int fdi, fdo, status;
+ int fdi, fdo;
+ struct strbuf err =
Re: [PATCH] copy.c: make copy_fd preserve meaningful errno
On Mon, Nov 17, 2014 at 3:35 PM, Jonathan Nieder jrnie...@gmail.com wrote:
Hi,
Stefan Beller wrote:
This patch was sent previously to the list as part of
that series[2], but it seems to be unrelated to me.
Thanks. Good call.
[...]
From: Ronnie Sahlberg sahlb...@google.com
Update copy_fd to return a meaningful errno on failure and also
preserve the existing errno variable.
Some functions in git make errno meaningful on error and others don't.
In general, the more we only use errno immediately after a system
call, the better, so based on the above description this seems like a
step in the wrong direction.
I did reword the commit message badly. Before it just read
Update copy_fd to return a meaningful errno.
In fact the proposed patch doesn't guarantee the errno, which is set
at the beginning of the function to be the same at the end.
What it really should preserve is the errno from xread, while
evaluating error(copy-fd: read returned %s, strerror(errno));
So the function call of error(...) or strerror(...) may change the errno.
Do any callers care about errno? Does the function's API
documentation say it will make errno meaningful on error, so people
making changes to copy_fd in the future know to maintain that
property?
*searches*
Looks like callers are:
convert.c::filter_buffer_or_fd, which doesn't care
copy.c::copy_file, which also doesn't care
lockfile.c::hold_lock_file_for_append, which started caring
in order to propagate errno in v2.2.0-rc0~53^2~2 (restore errno
before returning, 2014-10-01). But no callers of that function
care yet.
So this is about fixing a bug-waiting-to-happen in
hold_lock_file_for_append. That would be enough to motivate the
change.
[...]
copy.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
Please also update the API documentation in cache.h so we remember not
to backslide in the future.
[...]
--- a/copy.c
+++ b/copy.c
@@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)
if (!len)
break;
if (len 0) {
- return error(copy-fd: read returned %s,
- strerror(errno));
+ int save_errno = errno;
+ error(copy-fd: read returned %s, strerror(errno));
+ errno = save_errno;
+ return -1;
Any caller is presumably going to turn around and print strerror(errno)
again, producing repetitive output.
Can we do better? E.g., if the signature were
int copy_fd(int ifd, int ofd, struct strbuf *err);
then we could write the error message to the err strbuf for the
caller to print. The error handling would be more explicit so
there would be no need to protect errno from clobbering by other
system calls (both here and in callers).
Something like this:
I like this approach, though your patch is not about the original
intention as this one
(having strerror(...) not messing with the errno), but rather
accumulating the errors not
in numbers but string buffers?
Signed-off-by: Jonathan Nieder jrnie...@gmail.com
diff --git i/cache.h w/cache.h
index 99ed096..ddaa30f 100644
--- i/cache.h
+++ w/cache.h
@@ -1479,7 +1479,7 @@ extern const char *git_mailmap_blob;
extern void maybe_flush_or_die(FILE *, const char *);
__attribute__((format (printf, 2, 3)))
extern void fprintf_or_die(FILE *, const char *fmt, ...);
-extern int copy_fd(int ifd, int ofd);
+extern int copy_fd(int ifd, int ofd, struct strbuf *err);
extern int copy_file(const char *dst, const char *src, int mode);
extern int copy_file_with_time(const char *dst, const char *src, int mode);
extern void write_or_die(int fd, const void *buf, size_t count);
diff --git i/convert.c w/convert.c
index 9a5612e..e301447 100644
--- i/convert.c
+++ w/convert.c
@@ -358,7 +358,11 @@ static int filter_buffer_or_fd(int in, int out, void
*data)
if (params-src) {
write_err = (write_in_full(child_process.in, params-src,
params-size) 0);
} else {
- write_err = copy_fd(params-fd, child_process.in);
+ struct strbuf err = STRBUF_INIT;
+ write_err = copy_fd(params-fd, child_process.in, err);
+ if (write_err)
+ error(copy-fd: %s, err.buf);
+ strbuf_release(err);
}
if (close(child_process.in))
diff --git i/copy.c w/copy.c
index f2970ec..828661a 100644
--- i/copy.c
+++ w/copy.c
@@ -1,19 +1,22 @@
#include cache.h
-int copy_fd(int ifd, int ofd)
+int copy_fd(int ifd, int ofd, struct strbuf *err)
{
+ assert(err);
+
while (1) {
char buffer[8192];
ssize_t len = xread(ifd, buffer, sizeof(buffer));
if (!len)
break;
if (len 0) {
- return error(copy-fd: read returned %s,
-
Re: [PATCH] copy.c: make copy_fd preserve meaningful errno
(meta-comment: please snip out the context you are not responding to, to make reading easier) Stefan Beller wrote: On Mon, Nov 17, 2014 at 3:35 PM, Jonathan Nieder jrnie...@gmail.com wrote: Stefan Beller wrote: Update copy_fd to return a meaningful errno on failure and also preserve the existing errno variable. Some functions in git make errno meaningful on error and others don't. In general, the more we only use errno immediately after a system call, the better, so based on the above description this seems like a step in the wrong direction. I did reword the commit message badly. Before it just read Update copy_fd to return a meaningful errno. In fact the proposed patch doesn't guarantee the errno, which is set at the beginning of the function to be the same at the end. What it really should preserve is the errno from xread, while evaluating error(copy-fd: read returned %s, strerror(errno)); So the function call of error(...) or strerror(...) may change the errno. A successful call to strerror() is guaranteed not to change errno, but a call to error() does I/O so it can clobber errno. The basic question is whether errno is and should be part of copy_fd()'s contract. Until v2.2.0-rc0~53^2~2 (2014-10-01), it wasn't. Even after that change, there's no user-visible effect to clobbering errno (so this patch is about maintainability, as opposed to fixing a user-visible bad behavior). [...] Can we do better? E.g., if the signature were int copy_fd(int ifd, int ofd, struct strbuf *err); then we could write the error message to the err strbuf for the caller to print. The error handling would be more explicit so there would be no need to protect errno from clobbering by other system calls (both here and in callers). Something like this: I like this approach, though your patch is not about the original intention as this one (having strerror(...) not messing with the errno), but rather accumulating the errors not in numbers but string buffers? After this patch, setting errno is not part of the contract of copy_fd, so the bug Ronnie was fixing is gone. But it's a little more invasive. What do you think? Thanks, Jonathan -- 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] copy.c: make copy_fd preserve meaningful errno
On Mon, Nov 17, 2014 at 4:48 PM, Jonathan Nieder jrnie...@gmail.com wrote: (meta-comment: please snip out the context you are not responding to, to make reading easier) will do After this patch, setting errno is not part of the contract of copy_fd, so the bug Ronnie was fixing is gone. But it's a little more invasive. What do you think? I really like that approach and would be happy if we'd take it instead of the one I sent. -- 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
