Re: [PATCH 2/3] run-command: Be more informative about what failed
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 31, 2013, 16:24 UTC
- Message-ID
- <7vfw1hiami.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1359597666-10108-3-git-send-email-sboyd@codeaurora.org>
Stephen Boyd <sboyd@codeaurora.org> writes:
Show 7 quoted lines
> 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 <sboyd@codeaurora.org> > ---
Show 44 quoted lines
> 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;
> }