Re: [PATCH 2/3] run-command: Be more informative about what failed
Stephen Boyd sb...@codeaurora.org writes: While debugging an error with verify_signed_buffer() the error messages from run-command weren't very useful: error: cannot create pipe for gpg: Too many open files error: could not run gpg. because they didn't indicate *which* pipe couldn't be created. For the message emitted here with your update (or without for that matter) to be useful, it has to hold that there is a single leaker, that leaker fails in this codepath, and that there is nobody else involved. Otherwise, you may be able to tell that one caller could not create its stdin, but the reason it couldn't may be because somebody else consumed all the available file descriptors. I am not opposed to this change per-se, but I am not sure that saying stdin etc. makes the message more useful for the purpose of debugging. For example, the above error now prints: error: cannot create stderr pipe for gpg: Too many open files error: could not run gpg. I'd prefer to see these names spelled out (e.g. standard error) in any case. Thanks. Signed-off-by: Stephen Boyd sb...@codeaurora.org --- run-command.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/run-command.c b/run-command.c index 12d4ddb..016dd05 100644 --- a/run-command.c +++ b/run-command.c @@ -274,6 +274,7 @@ int start_command(struct child_process *cmd) int need_in, need_out, need_err; int fdin[2], fdout[2], fderr[2]; int failed_errno = failed_errno; + char *str; /* * In case of errors we must keep the promise to close FDs @@ -286,6 +287,7 @@ int start_command(struct child_process *cmd) failed_errno = errno; if (cmd-out 0) close(cmd-out); + str = stdin; goto fail_pipe; } cmd-in = fdin[1]; @@ -301,6 +303,7 @@ int start_command(struct child_process *cmd) close_pair(fdin); else if (cmd-in) close(cmd-in); + str = stdout; goto fail_pipe; } cmd-out = fdout[0]; @@ -318,9 +321,10 @@ int start_command(struct child_process *cmd) close_pair(fdout); else if (cmd-out) close(cmd-out); + str = stderr; fail_pipe: - error(cannot create pipe for %s: %s, - cmd-argv[0], strerror(failed_errno)); + error(cannot create %s pipe for %s: %s, + str, cmd-argv[0], strerror(failed_errno)); errno = failed_errno; return -1; } -- 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 2/3] run-command: Be more informative about what failed
On 01/31/13 08:24, Junio C Hamano wrote: Stephen Boyd sb...@codeaurora.org writes: While debugging an error with verify_signed_buffer() the error messages from run-command weren't very useful: error: cannot create pipe for gpg: Too many open files error: could not run gpg. because they didn't indicate *which* pipe couldn't be created. For the message emitted here with your update (or without for that matter) to be useful, it has to hold that there is a single leaker, that leaker fails in this codepath, and that there is nobody else involved. Otherwise, you may be able to tell that one caller could not create its stdin, but the reason it couldn't may be because somebody else consumed all the available file descriptors. I am not opposed to this change per-se, but I am not sure that saying stdin etc. makes the message more useful for the purpose of debugging. It helped me avoid firing up gdb, but if you don't see much use feel free to ignore this patch. For example, the above error now prints: error: cannot create stderr pipe for gpg: Too many open files error: could not run gpg. I'd prefer to see these names spelled out (e.g. standard error) in any case. Sure, I can do that. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- 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 2/3] run-command: Be more informative about what failed
On Thu, Jan 31, 2013 at 08:24:21AM -0800, Junio C Hamano wrote: Stephen Boyd sb...@codeaurora.org writes: While debugging an error with verify_signed_buffer() the error messages from run-command weren't very useful: error: cannot create pipe for gpg: Too many open files error: could not run gpg. because they didn't indicate *which* pipe couldn't be created. For the message emitted here with your update (or without for that matter) to be useful, it has to hold that there is a single leaker, that leaker fails in this codepath, and that there is nobody else involved. Otherwise, you may be able to tell that one caller could not create its stdin, but the reason it couldn't may be because somebody else consumed all the available file descriptors. I am not opposed to this change per-se, but I am not sure that saying stdin etc. makes the message more useful for the purpose of debugging. Yeah, I had the same feeling. All that failed is pipe(), which does not have anything to do with what we are going to use the pipe for. So it gives some context, perhaps, but does not necessarily tell us anything useful. But it is not much code, and sometimes it is surprising what information can be helpful when debugging, so like you, I am not opposed, just doubtful. -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 2/3] run-command: Be more informative about what failed
Jeff King p...@peff.net writes: But it is not much code, and sometimes it is surprising what information can be helpful when debugging, so like you, I am not opposed, just doubtful. Yes, exactly my feeling. Perhaps I should just amend the 'stdin' and friends away without asking Stephen to reroll. In the other two I did not see any issues. I've queued all three of them including this one but as separate topics. Thanks. -- 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 2/3] run-command: Be more informative about what failed
While debugging an error with verify_signed_buffer() the error messages from run-command weren't very useful: error: cannot create pipe for gpg: Too many open files error: could not run gpg. because they didn't indicate *which* pipe couldn't be created. Print which pipe failed to be created in the error message so we can more easily debug similar problems in the future. For example, the above error now prints: error: cannot create stderr pipe for gpg: Too many open files error: could not run gpg. Signed-off-by: Stephen Boyd sb...@codeaurora.org --- run-command.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/run-command.c b/run-command.c index 12d4ddb..016dd05 100644 --- a/run-command.c +++ b/run-command.c @@ -274,6 +274,7 @@ int start_command(struct child_process *cmd) int need_in, need_out, need_err; int fdin[2], fdout[2], fderr[2]; int failed_errno = failed_errno; + char *str; /* * In case of errors we must keep the promise to close FDs @@ -286,6 +287,7 @@ int start_command(struct child_process *cmd) failed_errno = errno; if (cmd-out 0) close(cmd-out); + str = stdin; goto fail_pipe; } cmd-in = fdin[1]; @@ -301,6 +303,7 @@ int start_command(struct child_process *cmd) close_pair(fdin); else if (cmd-in) close(cmd-in); + str = stdout; goto fail_pipe; } cmd-out = fdout[0]; @@ -318,9 +321,10 @@ int start_command(struct child_process *cmd) close_pair(fdout); else if (cmd-out) close(cmd-out); + str = stderr; fail_pipe: - error(cannot create pipe for %s: %s, - cmd-argv[0], strerror(failed_errno)); + error(cannot create %s pipe for %s: %s, + str, cmd-argv[0], strerror(failed_errno)); errno = failed_errno; return -1; } -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation -- 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
