From 5709e0363a891b72eb9e9756d7fb121d9bf6a7c7 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sat, 4 Jul 2009 21:26:39 +0200 Subject: run_command: return exit code as positive value As a general guideline, functions in git's code return zero to indicate success and negative values to indicate failure. The run_command family of functions followed this guideline. But there are actually two different kinds of failure: - failures of system calls; - non-zero exit code of the program that was run. Usually, a non-zero exit code of the program is a failure and means a failure to the caller. Except that sometimes it does not. For example, the exit code of merge programs (e.g. external merge drivers) conveys information about how the merge failed, and not all exit calls are actually failures. Furthermore, the return value of run_command is sometimes used as exit code by the caller. This change arranges that the exit code of the program is returned as a positive value, which can now be regarded as the "result" of the function. System call failures continue to be reported as negative values. Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano --- run-command.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) (limited to 'run-command.c') diff --git a/run-command.c b/run-command.c index eb2efc3307..a4e309eeb9 100644 --- a/run-command.c +++ b/run-command.c @@ -241,14 +241,7 @@ static int wait_or_whine(pid_t pid) if (!WIFEXITED(status)) return -ERR_RUN_COMMAND_WAITPID_NOEXIT; code = WEXITSTATUS(status); - switch (code) { - case 127: - return -ERR_RUN_COMMAND_EXEC; - case 0: - return 0; - default: - return -code; - } + return code == 127 ? -ERR_RUN_COMMAND_EXEC : code; } } -- cgit v1.2.3 From 0ac77ec3150f43a5c2a6b1e47e9db5aafe53fb72 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sat, 4 Jul 2009 21:26:40 +0200 Subject: run_command: report system call errors instead of returning error codes The motivation for this change is that system call failures are serious errors that should be reported to the user, but only few callers took the burden to decode the error codes that the functions returned into error messages. If at all, then only an unspecific error message was given. A prominent example is this: $ git upload-pack . | : fatal: unable to run 'git-upload-pack' In this example, git-upload-pack, the external command invoked through the git wrapper, dies due to SIGPIPE, but the git wrapper does not bother to report the real cause. In fact, this very error message is copied to the syslog if git-daemon's client aborts the connection early. With this change, system call failures are reported immediately after the failure and only a generic failure code is returned to the caller. In the above example the error is now to the point: $ git upload-pack . | : error: git-upload-pack died of signal Note that there is no error report if the invoked program terminated with a non-zero exit code, because it is reasonable to expect that the invoked program has already reported an error. (But many run_command call sites nevertheless write a generic error message.) There was one special return code that was used to identify the case where run_command failed because the requested program could not be exec'd. This special case is now treated like a system call failure with errno set to ENOENT. No error is reported in this case, because the call site in git.c expects this as a normal result. Therefore, the callers that carefully decoded the return value still check for this condition. Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano --- run-command.c | 89 ++++++++++++++++++++++++++++++++--------------------------- 1 file changed, 49 insertions(+), 40 deletions(-) (limited to 'run-command.c') diff --git a/run-command.c b/run-command.c index a4e309eeb9..e273c6c451 100644 --- a/run-command.c +++ b/run-command.c @@ -19,6 +19,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; /* * In case of errors we must keep the promise to close FDs @@ -28,9 +29,10 @@ int start_command(struct child_process *cmd) need_in = !cmd->no_stdin && cmd->in < 0; if (need_in) { if (pipe(fdin) < 0) { + failed_errno = errno; if (cmd->out > 0) close(cmd->out); - return -ERR_RUN_COMMAND_PIPE; + goto fail_pipe; } cmd->in = fdin[1]; } @@ -40,11 +42,12 @@ int start_command(struct child_process *cmd) && cmd->out < 0; if (need_out) { if (pipe(fdout) < 0) { + failed_errno = errno; if (need_in) close_pair(fdin); else if (cmd->in) close(cmd->in); - return -ERR_RUN_COMMAND_PIPE; + goto fail_pipe; } cmd->out = fdout[0]; } @@ -52,6 +55,7 @@ int start_command(struct child_process *cmd) need_err = !cmd->no_stderr && cmd->err < 0; if (need_err) { if (pipe(fderr) < 0) { + failed_errno = errno; if (need_in) close_pair(fdin); else if (cmd->in) @@ -60,7 +64,11 @@ int start_command(struct child_process *cmd) close_pair(fdout); else if (cmd->out) close(cmd->out); - return -ERR_RUN_COMMAND_PIPE; +fail_pipe: + error("cannot create pipe for %s: %s", + cmd->argv[0], strerror(failed_errno)); + errno = failed_errno; + return -1; } cmd->err = fderr[0]; } @@ -122,6 +130,9 @@ int start_command(struct child_process *cmd) strerror(errno)); exit(127); } + if (cmd->pid < 0) + error("cannot fork() for %s: %s", cmd->argv[0], + strerror(failed_errno = errno)); #else int s0 = -1, s1 = -1, s2 = -1; /* backups of stdin, stdout, stderr */ const char **sargv = cmd->argv; @@ -173,6 +184,9 @@ int start_command(struct child_process *cmd) } cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env); + failed_errno = errno; + if (cmd->pid < 0 && errno != ENOENT) + error("cannot spawn %s: %s", cmd->argv[0], strerror(errno)); if (cmd->env) free_environ(env); @@ -189,7 +203,6 @@ int start_command(struct child_process *cmd) #endif if (cmd->pid < 0) { - int err = errno; if (need_in) close_pair(fdin); else if (cmd->in) @@ -200,9 +213,8 @@ int start_command(struct child_process *cmd) close(cmd->out); if (need_err) close_pair(fderr); - return err == ENOENT ? - -ERR_RUN_COMMAND_EXEC : - -ERR_RUN_COMMAND_FORK; + errno = failed_errno; + return -1; } if (need_in) @@ -221,33 +233,41 @@ int start_command(struct child_process *cmd) return 0; } -static int wait_or_whine(pid_t pid) +static int wait_or_whine(pid_t pid, const char *argv0) { - for (;;) { - int status, code; - pid_t waiting = waitpid(pid, &status, 0); - - if (waiting < 0) { - if (errno == EINTR) - continue; - error("waitpid failed (%s)", strerror(errno)); - return -ERR_RUN_COMMAND_WAITPID; - } - if (waiting != pid) - return -ERR_RUN_COMMAND_WAITPID_WRONG_PID; - if (WIFSIGNALED(status)) - return -ERR_RUN_COMMAND_WAITPID_SIGNAL; - - if (!WIFEXITED(status)) - return -ERR_RUN_COMMAND_WAITPID_NOEXIT; + int status, code = -1; + pid_t waiting; + int failed_errno = 0; + + while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR) + ; /* nothing */ + + if (waiting < 0) { + failed_errno = errno; + error("waitpid for %s failed: %s", argv0, strerror(errno)); + } else if (waiting != pid) { + error("waitpid is confused (%s)", argv0); + } else if (WIFSIGNALED(status)) { + error("%s died of signal", argv0); + } else if (WIFEXITED(status)) { code = WEXITSTATUS(status); - return code == 127 ? -ERR_RUN_COMMAND_EXEC : code; + /* + * Convert special exit code when execvp failed. + */ + if (code == 127) { + code = -1; + failed_errno = ENOENT; + } + } else { + error("waitpid is confused (%s)", argv0); } + errno = failed_errno; + return code; } int finish_command(struct child_process *cmd) { - return wait_or_whine(cmd->pid); + return wait_or_whine(cmd->pid, cmd->argv[0]); } int run_command(struct child_process *cmd) @@ -331,10 +351,7 @@ int start_async(struct async *async) int finish_async(struct async *async) { #ifndef __MINGW32__ - int ret = 0; - - if (wait_or_whine(async->pid)) - ret = error("waitpid (async) failed"); + int ret = wait_or_whine(async->pid, "child process"); #else DWORD ret = 0; if (WaitForSingleObject(async->tid, INFINITE) != WAIT_OBJECT_0) @@ -378,15 +395,7 @@ int run_hook(const char *index_file, const char *name, ...) hook.env = env; } - ret = start_command(&hook); + ret = run_command(&hook); free(argv); - if (ret) { - warning("Could not spawn %s", argv[0]); - return ret; - } - ret = finish_command(&hook); - if (ret == -ERR_RUN_COMMAND_WAITPID_SIGNAL) - warning("%s exited due to uncaught signal", argv[0]); - return ret; } -- cgit v1.2.3 From b99d5f40d6a5cba7d7cd7599063b3cd78aa4d219 Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sat, 4 Jul 2009 21:26:41 +0200 Subject: run_command: encode deadly signal number in the return value We now write the signal number in the error message if the program terminated by a signal. The negative return value is constructed such that after truncation to 8 bits it looks like a POSIX shell's $?: $ echo 0000 | { git upload-pack .; echo $? >&2; } | : error: git-upload-pack died of signal 13 141 Previously, the exit code was 255 instead of 141. Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano --- run-command.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) (limited to 'run-command.c') diff --git a/run-command.c b/run-command.c index e273c6c451..30c2b3dd88 100644 --- a/run-command.c +++ b/run-command.c @@ -248,7 +248,14 @@ static int wait_or_whine(pid_t pid, const char *argv0) } else if (waiting != pid) { error("waitpid is confused (%s)", argv0); } else if (WIFSIGNALED(status)) { - error("%s died of signal", argv0); + code = WTERMSIG(status); + error("%s died of signal %d", argv0, code); + /* + * This return value is chosen so that code & 0xff + * mimics the exit code that a POSIX shell would report for + * a program that died from this signal. + */ + code -= 128; } else if (WIFEXITED(status)) { code = WEXITSTATUS(status); /* -- cgit v1.2.3 From c024beb56da679839d61f352d088b9a86823233a Mon Sep 17 00:00:00 2001 From: Johannes Sixt Date: Sat, 4 Jul 2009 21:26:42 +0200 Subject: run_command: report failure to execute the program, but optionally don't In the case where a program was not found, it was still the task of the caller to report an error to the user. Usually, this is an interesting case but only few callers actually reported a specific error (though many call sites report a generic error message regardless of the cause). With this change the error is reported by run_command, but since there is one call site in git.c that does not want that, an option is added to struct child_process, which is used to turn the error off. Signed-off-by: Johannes Sixt Signed-off-by: Junio C Hamano --- run-command.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) (limited to 'run-command.c') diff --git a/run-command.c b/run-command.c index 30c2b3dd88..b613bddc71 100644 --- a/run-command.c +++ b/run-command.c @@ -185,7 +185,7 @@ fail_pipe: cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, env); failed_errno = errno; - if (cmd->pid < 0 && errno != ENOENT) + if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT)) error("cannot spawn %s: %s", cmd->argv[0], strerror(errno)); if (cmd->env) @@ -233,7 +233,7 @@ fail_pipe: return 0; } -static int wait_or_whine(pid_t pid, const char *argv0) +static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure) { int status, code = -1; pid_t waiting; @@ -264,6 +264,9 @@ static int wait_or_whine(pid_t pid, const char *argv0) if (code == 127) { code = -1; failed_errno = ENOENT; + if (!silent_exec_failure) + error("cannot run %s: %s", argv0, + strerror(ENOENT)); } } else { error("waitpid is confused (%s)", argv0); @@ -274,7 +277,7 @@ static int wait_or_whine(pid_t pid, const char *argv0) int finish_command(struct child_process *cmd) { - return wait_or_whine(cmd->pid, cmd->argv[0]); + return wait_or_whine(cmd->pid, cmd->argv[0], cmd->silent_exec_failure); } int run_command(struct child_process *cmd) @@ -294,6 +297,7 @@ static void prepare_run_command_v_opt(struct child_process *cmd, cmd->no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0; cmd->git_cmd = opt & RUN_GIT_CMD ? 1 : 0; cmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0; + cmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0; } int run_command_v_opt(const char **argv, int opt) @@ -358,7 +362,7 @@ int start_async(struct async *async) int finish_async(struct async *async) { #ifndef __MINGW32__ - int ret = wait_or_whine(async->pid, "child process"); + int ret = wait_or_whine(async->pid, "child process", 0); #else DWORD ret = 0; if (WaitForSingleObject(async->tid, INFINITE) != WAIT_OBJECT_0) -- cgit v1.2.3 From 5a7a3671b74c043216549b94a718da04cc3ffcd6 Mon Sep 17 00:00:00 2001 From: David Soria Parra Date: Tue, 4 Aug 2009 11:28:40 +0200 Subject: run-command.c: squelch a "use before assignment" warning i686-apple-darwin9-gcc-4.0.1 (GCC) 4.0.1 (Apple Inc. build 5490) compiler (and probably others) mistakenly thinks variable failed_errno is used before assigned. Work it around by giving it a fake initialization. Signed-off-by: David Soria Parra Signed-off-by: Junio C Hamano --- run-command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'run-command.c') diff --git a/run-command.c b/run-command.c index b613bddc71..71f83368c4 100644 --- a/run-command.c +++ b/run-command.c @@ -19,7 +19,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; + int failed_errno = failed_errno; /* * In case of errors we must keep the promise to close FDs -- cgit v1.2.3