* [RFC PATCH v2 0/2] Imporove remote helper exec failure reporting @ 2010-01-09 13:45 Ilari Liusvaara 2010-01-09 13:45 ` [RFC PATCH v2 1/2] Report exec errors from run-command Ilari Liusvaara 2010-01-09 13:45 ` [RFC PATCH v2 2/2] Improve transport helper exec failure reporting Ilari Liusvaara 0 siblings, 2 replies; 11+ messages in thread From: Ilari Liusvaara @ 2010-01-09 13:45 UTC (permalink / raw) To: git (This is newer than updated patch v3). I decided to go on a bit further with new features, and that's why this is back as RFC. Diffrences from first RFC round: - Split loop-over-close to its own function and warn but not retry on bizarre error codes (basically anything except EBADF and EINTR). - Don't rely on pipe(2) to preserve the fd array on failure. - Add some testcases. - Fix buggy behaviour if report pipe read end winds up as fd #0. - Set silent_exec_failure on executing remote helper. - Make exec reporting in presence of preexec_cb controlled by flag to solve pager deadlock problems without preventing using preexec_cb with exec reporting entierely (the way pager is invoked is just fundamentially incompatible with exec reporting). - Add flag to make run-command give different code for exec failing due to ENOENT than for other failures (including other ways to get ENOENT). - change chdir failure to be reported as error (and pass error code back). BTW: Nothing seems to use that chdir feature, aside of new testcase. - Report errors to parent's stderr. - Don't retry waitpid on bizarre error codes (basically anything except EINTR). - Make run_command actually complain about exec failing (as API docs say). Ilari Liusvaara (2): Report exec errors from run-command Improve transport helper exec failure reporting Documentation/technical/api-run-command.txt | 2 + Makefile | 1 + run-command.c | 196 +++++++++++++++++++++++++-- run-command.h | 2 + t/t0061-run-command.sh | 20 +++ test-run-command.c | 74 ++++++++++ transport-helper.c | 27 +++- 7 files changed, 305 insertions(+), 17 deletions(-) create mode 100755 t/t0061-run-command.sh create mode 100644 test-run-command.c ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFC PATCH v2 1/2] Report exec errors from run-command 2010-01-09 13:45 [RFC PATCH v2 0/2] Imporove remote helper exec failure reporting Ilari Liusvaara @ 2010-01-09 13:45 ` Ilari Liusvaara 2010-01-10 13:04 ` [PATCH 0/4] Detect exec errors in start_command early Johannes Sixt 2010-01-09 13:45 ` [RFC PATCH v2 2/2] Improve transport helper exec failure reporting Ilari Liusvaara 1 sibling, 1 reply; 11+ messages in thread From: Ilari Liusvaara @ 2010-01-09 13:45 UTC (permalink / raw) To: git Previously run-command was unable to report errors happening in exec call. Change it to pass errno from failed exec to errno value at return. Also add special mode that diffrentiates between ENOENT from exec and ENOENT from other source. The errno value passing can be done by opening close-on-exec pipe and piping the error code through in case of failure. In case of success, close-on-exec closes the pipe on successful exec and parent process gets end of file on read. Signed-off-by: Johannes Sixt <j6t@kdbg.org> Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi> --- Documentation/technical/api-run-command.txt | 2 + Makefile | 1 + run-command.c | 196 +++++++++++++++++++++++++-- run-command.h | 2 + t/t0061-run-command.sh | 20 +++ test-run-command.c | 74 ++++++++++ 6 files changed, 282 insertions(+), 13 deletions(-) create mode 100755 t/t0061-run-command.sh create mode 100644 test-run-command.c diff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt index b26c281..f37d8ba 100644 --- a/Documentation/technical/api-run-command.txt +++ b/Documentation/technical/api-run-command.txt @@ -49,6 +49,8 @@ The functions above do the following: . If the program was not found, then -1 is returned and errno is set to ENOENT; a diagnostic is printed only if .silent_exec_failure is 0. + (if extended_error_code flag is true, instead of -1, the return value + is -2 in this case). . Otherwise, the program is run. If it terminates regularly, its exit code is returned. No diagnistic is printed, even if the exit code is diff --git a/Makefile b/Makefile index a2fce5a..92eed68 100644 --- a/Makefile +++ b/Makefile @@ -1753,6 +1753,7 @@ TEST_PROGRAMS_NEED_X += test-path-utils TEST_PROGRAMS_NEED_X += test-sha1 TEST_PROGRAMS_NEED_X += test-sigchain TEST_PROGRAMS_NEED_X += test-index-version +TEST_PROGRAMS_NEED_X += test-run-command TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X)) diff --git a/run-command.c b/run-command.c index 47ced57..b1af9c4 100644 --- a/run-command.c +++ b/run-command.c @@ -15,6 +15,20 @@ static inline void dup_devnull(int to) close(fd); } +static inline void force_close(int fd) +{ + /* + * Assume all errors except EINTR are persistent and + * that EBADF signals already closed fd. + */ + while (close(fd) < 0 && errno != EBADF) + if (errno != EINTR) { + warning("Close returned error %s", + strerror(errno)); + break; + } +} + static const char **prepare_shell_cmd(const char **argv) { int argc, nargc = 0; @@ -49,6 +63,8 @@ static const char **prepare_shell_cmd(const char **argv) } #ifndef WIN32 +int child_err = -1; + static int execv_shell_cmd(const char **argv) { const char **nargv = prepare_shell_cmd(argv); @@ -57,6 +73,26 @@ static int execv_shell_cmd(const char **argv) free(nargv); return -1; } + +static void error_child(const char *err, ...) +{ + char msg[4096]; + int len; + va_list args; + va_start(args, err); + len = vsnprintf(msg, sizeof(msg), err, args); + va_end(args); + if (len >= sizeof(msg)) + len = sizeof(msg) - 1; + + if (child_err < 0) + return; + + write(child_err, "error: ", 7); + write(child_err, msg, len); + write(child_err, "\n", 1); +} + #endif int start_command(struct child_process *cmd) @@ -64,6 +100,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; + int func_ret = 0; /* * In case of errors we must keep the promise to close FDs @@ -120,9 +157,91 @@ fail_pipe: trace_argv_printf(cmd->argv, "trace: run_command:"); #ifndef WIN32 +{ + int report_pipe[2] = {-1, -1}; + + if (pipe(report_pipe) < 0) { + report_pipe[0] = -1; + report_pipe[1] = -1; + warning("Can't open pipe for exec status report: %s\n", + strerror(errno)); + } + fflush(NULL); cmd->pid = fork(); - if (!cmd->pid) { + if (cmd->pid > 0) { + int r = 0, ret = 0; + force_close(report_pipe[1]); +read_again: + if (report_pipe[0] >= 0) + r = read(report_pipe[0], &ret, sizeof(ret)); + if (r < 0 && (errno == EAGAIN || errno == EINTR || + errno == EWOULDBLOCK)) + goto read_again; + else if (r < 0) + warning("Can't read exec status report: %s\n", + strerror(errno)); + else if (r == 0) + ; + else if (r < sizeof(ret)) { + warning("Received incomplete exec status report.\n"); + } else { + failed_errno = ret; + /* + * Clean up the process that did the failed execution + * so no zombies remain. + */ + while (waitpid(cmd->pid, &ret, 0) < 0 && errno == EINTR) + /* Nothing. */ ; + if (WIFEXITED(ret) && WEXITSTATUS(ret) == 127) + /* Program was not found. */ + func_ret = -2; + cmd->pid = -1; + } + } else if (!cmd->pid) { + int r = 0; + int errored = 0; + int flags; + force_close(report_pipe[0]); + + flags = fcntl(report_pipe[1], F_GETFD); + if (flags < 0) + r = -1; + r = (r || fcntl(report_pipe[1], F_SETFD, flags | FD_CLOEXEC)); + if (r) { + warning("Can't FD_CLOEXEC pipe for exec status report: %s", + strerror(errno)); + force_close(report_pipe[1]); + report_pipe[1] = -1; + } + + /* + * Redirect the channel to write syscall error messages to + * before redirecting the process's stderr so that all error + * messages in subsequent call paths use the parent's stderr. + */ + if (cmd->no_stderr || need_err) { + child_err = dup(2); + if (child_err < 0 && errno != EBADF) + /* EBADF => No stderr, nowhere to go. */ + warning("Can't dup fd for child exec errors: %s", + strerror(errno)); + if (child_err < 0) + goto no_error_chan; + + flags = fcntl(child_err, F_GETFL); + if (flags < 0) + flags = 0; + if (fcntl(child_err, F_SETFL, flags | FD_CLOEXEC) < 0) { + warning("Can't FD_CLOEXEC fd for child exec errors: %s", + strerror(errno)); + force_close(report_pipe[1]); + child_err = -1; + } + } else + child_err = 2; +no_error_chan: + if (cmd->no_stdin) dup_devnull(0); else if (need_in) { @@ -152,9 +271,15 @@ fail_pipe: close(cmd->out); } - if (cmd->dir && chdir(cmd->dir)) - die_errno("exec '%s': cd to '%s' failed", cmd->argv[0], - cmd->dir); + if (cmd->dir && chdir(cmd->dir)) { + failed_errno = errno; + error_child("exec '%s': cd to '%s' failed: %s", + cmd->argv[0], cmd->dir, strerror(errno)); + trace_printf("trace: exec '%s': cd to '%s' failed: %s\n", + cmd->argv[0], cmd->dir, strerror(errno)); + errored = 1; + goto throw_fault; + } if (cmd->env) { for (; *cmd->env; cmd->env++) { if (strchr(*cmd->env, '=')) @@ -163,8 +288,23 @@ fail_pipe: unsetenv(*cmd->env); } } - if (cmd->preexec_cb) + if (cmd->preexec_cb) { + /* + * We don't know what pre-exec callbacks do, and they + * may do something that causes deadlock with exec + * reporting. The sole user of this hook seems to + * be pager, and it is run through shell, so one + * wouldn't get useful error from exec reporting + * and would get useful error from shell anyway. So + * just disable exec reporting for such comamnds. + * (unless explicitly specified otherwise). + */ + if (!cmd->report_preexec_cb) { + force_close(report_pipe[1]); + report_pipe[1] = -1; + } cmd->preexec_cb(); + } if (cmd->git_cmd) { execv_git_cmd(cmd->argv); } else if (cmd->use_shell) { @@ -172,13 +312,36 @@ fail_pipe: } else { execvp(cmd->argv[0], (char *const*) cmd->argv); } + failed_errno = errno; + trace_printf("trace: exec '%s' failed: %s\n", cmd->argv[0], strerror(errno)); - exit(127); - } - if (cmd->pid < 0) + + if (!cmd->silent_exec_failure || errno != ENOENT) + error_child("cannot exec '%s': %s", + cmd->argv[0], strerror(failed_errno)); +throw_fault: + r = 0; +write_again: + if (report_pipe[1] >= 0) + r = write(report_pipe[1], &failed_errno, + sizeof(failed_errno)); + if (r < 0 && (errno == EAGAIN || errno == EINTR || + errno == EWOULDBLOCK)) + goto write_again; + + /* + * Diffrentiate between program doesn't exist and other + * errors. + */ + if (errored || errno != ENOENT) + exit(128); + else + exit(127); + } else 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 */ @@ -233,6 +396,8 @@ fail_pipe: failed_errno = errno; if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT)) error("cannot spawn %s: %s", cmd->argv[0], strerror(errno)); + if (cmd->pid < 0 && errno == ENOENT) + func_ret = -2; if (cmd->env) free_environ(env); @@ -261,7 +426,10 @@ fail_pipe: if (need_err) close_pair(fderr); errno = failed_errno; - return -1; + if (cmd->extended_error_code && func_ret) + return func_ret; + else + return -1; } if (need_in) @@ -280,7 +448,8 @@ fail_pipe: return 0; } -static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure) +static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure, + int extended_err) { int status, code = -1; pid_t waiting; @@ -309,7 +478,7 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure) * Convert special exit code when execvp failed. */ if (code == 127) { - code = -1; + code = extended_err ? -2 : -1; failed_errno = ENOENT; if (!silent_exec_failure) error("cannot run %s: %s", argv0, @@ -324,7 +493,8 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure) int finish_command(struct child_process *cmd) { - return wait_or_whine(cmd->pid, cmd->argv[0], cmd->silent_exec_failure); + return wait_or_whine(cmd->pid, cmd->argv[0], cmd->silent_exec_failure, + cmd->extended_error_code); } int run_command(struct child_process *cmd) @@ -410,7 +580,7 @@ int start_async(struct async *async) int finish_async(struct async *async) { #ifndef WIN32 - int ret = wait_or_whine(async->pid, "child process", 0); + int ret = wait_or_whine(async->pid, "child process", 0, 0); #else DWORD ret = 0; if (WaitForSingleObject(async->tid, INFINITE) != WAIT_OBJECT_0) diff --git a/run-command.h b/run-command.h index 967ba8c..f6b4caf 100644 --- a/run-command.h +++ b/run-command.h @@ -34,6 +34,8 @@ struct child_process { unsigned silent_exec_failure:1; unsigned stdout_to_stderr:1; unsigned use_shell:1; + unsigned extended_error_code:1; + unsigned report_preexec_cb:1; void (*preexec_cb)(void); }; diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh new file mode 100755 index 0000000..0c217b1 --- /dev/null +++ b/t/t0061-run-command.sh @@ -0,0 +1,20 @@ +#!/bin/sh +# +# Copyright (c) 2009 Ilari Liusvaara +# + +test_description='Test run command' + +. ./test-lib.sh + +test_expect_success "reporting ENOENT (non-extended)" \ +"test-run-command 2" + +test_expect_success "reporting ENOENT (extended)" \ +"test-run-command 1" + +test_expect_success "reporting chdir failed (extended)" \ +"test-run-command 3" + + +test_done diff --git a/test-run-command.c b/test-run-command.c new file mode 100644 index 0000000..db664f3 --- /dev/null +++ b/test-run-command.c @@ -0,0 +1,74 @@ +/* + * test-run-command.c: test run command API. + * + * (C) 2009 Ilari Liusvaara <ilari.liusvaara@elisanet.fi> + * + * This code is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include "git-compat-util.h" +#include "run-command.h" +#include <string.h> +#include <errno.h> + +int main(int argc, char **argv) +{ + char* procs[2]; + struct child_process proc; + char test = argv[1][0]; + memset(&proc, 0, sizeof(proc)); + + if(argc < 2) + return 1; + + + switch(test) { + case '1': + procs[0] = "does-not-exist-62896869286"; + procs[1] = NULL; + proc.argv = (const char **)procs; + proc.extended_error_code = 1; + + if (start_command(&proc) != -2) { + fprintf(stderr, "FAIL A\n"); + return 1; + } + if (test == '1' && errno == ENOENT) + return 0; + case '2': + procs[0] = "does-not-exist-62896869286"; + procs[1] = NULL; + proc.argv = (const char **)procs; + proc.extended_error_code = 0; + + if (start_command(&proc) != -1) { + fprintf(stderr, "FAIL A\n"); + return 1; + } + if (errno == ENOENT) + return 0; + case '3': +#ifdef WIN32 + /* This can't work on Win32, so just skip it. */ + return 0; +#endif + procs[0] = "does-not-exist-62896869286"; + procs[1] = NULL; + proc.argv = (const char **)procs; + proc.dir = "does-not-exist-52632632733"; + proc.extended_error_code = 1; + + if (start_command(&proc) != -1) { + fprintf(stderr, "FAIL A\n"); + return 1; + } + if (errno == ENOENT) + return 0; + } + + fprintf(stderr, "FAIL B\n"); + + return 1; +} -- 1.6.6.3.gaa2e1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 0/4] Detect exec errors in start_command early 2010-01-09 13:45 ` [RFC PATCH v2 1/2] Report exec errors from run-command Ilari Liusvaara @ 2010-01-10 13:04 ` Johannes Sixt 2010-01-10 13:07 ` [PATCH 1/4] start_command: report child process setup errors to the parent's stderr Johannes Sixt ` (3 more replies) 0 siblings, 4 replies; 11+ messages in thread From: Johannes Sixt @ 2010-01-10 13:04 UTC (permalink / raw) To: Ilari Liusvaara; +Cc: git On Samstag, 9. Januar 2010, Ilari Liusvaara wrote: > 6 files changed, 282 insertions(+), 13 deletions(-) IMHO, you are going completely overboard with your patch. You check every single possible and unlikely error condition in the new code that you introde, but ignore that there are quite a number of calls in the existing code whose error conditions go checked. And as a result, you "only" report errno values of some calls, and ignore that there is at least one potential die() call hidden in execv_git_cmd(). IOW, your code is not a catch-all and only an isolated solution. I hope I can do better. I developed this series on top of il/vcs-helper. But patches 1-3/4 are actually an independent topic and also suitable to be merged into jk/run-command-use-shell to implement improved DWIM whether a shell is needed. Patch 4/4 is to address your problem of weak error reporting with transport helpers. I think I understand now what your problem is -- namely that previously no error was reported if a transport helper program did not exist. The reason for the missing error message is IMO a design weakness in the protocol: It requires that the parent talks first - and by talking to an early-died child process it dies from SIGPIPE. I experimented with signal(SIGPIPE, SIG_IGN) to get around this, but it didn't work (the SIGPIPE arrived anyway). Anyway, patch 4/4 goes on top of il/vcs-helper after merging the topic consisting of 1-3/4. Ilari Liusvaara (1): Improve error message when a transport helper was not found Johannes Sixt (3): start_command: report child process setup errors to the parent's stderr run-command: move wait_or_whine earlier start_command: detect execvp failures early Makefile | 1 + run-command.c | 177 +++++++++++++++++++++++++++++++++++------------- t/t0061-run-command.sh | 14 ++++ test-run-command.c | 35 ++++++++++ transport-helper.c | 14 +++- 5 files changed, 191 insertions(+), 50 deletions(-) create mode 100755 t/t0061-run-command.sh create mode 100644 test-run-command.c ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/4] start_command: report child process setup errors to the parent's stderr 2010-01-10 13:04 ` [PATCH 0/4] Detect exec errors in start_command early Johannes Sixt @ 2010-01-10 13:07 ` Johannes Sixt 2010-01-10 13:08 ` [PATCH 2/4] run-command: move wait_or_whine earlier Johannes Sixt ` (2 subsequent siblings) 3 siblings, 0 replies; 11+ messages in thread From: Johannes Sixt @ 2010-01-10 13:07 UTC (permalink / raw) To: Ilari Liusvaara; +Cc: git When the child process's environment is set up in start_command(), error messages were written to wherever the parent redirected the child's stderr channel. However, even if the parent redirected the child's stderr, errors during this setup process, including the exec itself, are usually an indication of a problem in the parent's environment. Therefore, the error messages should go to the parent's stderr. Redirection of the child's error messages is usually only used to redirect hook error messages during client-server exchanges. In these cases, hook setup errors could be regarded as information leak. This patch makes a copy of stderr if necessary and uses a special die routine that is used for all die() calls in the child that sends the errors messages to the parent's stderr. The trace call that reported a failed execvp is removed (because it writes to stderr) and replaced by die_errno() with special treatment of ENOENT. The improvement in the error message can be seen with this sequence: mkdir .git/hooks/pre-commit git commit Previously, the error message was error: cannot run .git/hooks/pre-commit: No such file or directory and now it is fatal: cannot exec '.git/hooks/pre-commit': Permission denied Signed-off-by: Johannes Sixt <j6t@kdbg.org> --- This should address your concern that "errors go to who-knows-where", that I meanwhile share with you. run-command.c | 46 +++++++++++++++++++++++++++++++++++++++++++--- 1 files changed, 43 insertions(+), 3 deletions(-) diff --git a/run-command.c b/run-command.c index cf2d8f7..02c7bfb 100644 --- a/run-command.c +++ b/run-command.c @@ -15,6 +15,30 @@ static inline void dup_devnull(int to) close(fd); } +#ifndef WIN32 +static int child_err = 2; + +static NORETURN void die_child(const char *err, va_list params) +{ + char msg[4096]; + int len = vsnprintf(msg, sizeof(msg), err, params); + if (len > sizeof(msg)) + len = sizeof(msg); + + write(child_err, "fatal: ", 7); + write(child_err, msg, len); + write(child_err, "\n", 1); + exit(128); +} + +static inline void set_cloexec(int fd) +{ + int flags = fcntl(fd, F_GETFD); + if (flags >= 0) + fcntl(fd, F_SETFD, flags | FD_CLOEXEC); +} +#endif + int start_command(struct child_process *cmd) { int need_in, need_out, need_err; @@ -79,6 +103,17 @@ fail_pipe: fflush(NULL); cmd->pid = fork(); if (!cmd->pid) { + /* + * Redirect the channel to write syscall error messages to + * before redirecting the process's stderr so that all die() + * in subsequent call paths use the parent's stderr. + */ + if (cmd->no_stderr || need_err) { + child_err = dup(2); + set_cloexec(child_err); + } + set_die_routine(die_child); + if (cmd->no_stdin) dup_devnull(0); else if (need_in) { @@ -126,9 +161,14 @@ fail_pipe: } else { execvp(cmd->argv[0], (char *const*) cmd->argv); } - trace_printf("trace: exec '%s' failed: %s\n", cmd->argv[0], - strerror(errno)); - exit(127); + /* + * Do not check for cmd->silent_exec_failure; the parent + * process will check it when it sees this exit code. + */ + if (errno == ENOENT) + exit(127); + else + die_errno("cannot exec '%s'", cmd->argv[0]); } if (cmd->pid < 0) error("cannot fork() for %s: %s", cmd->argv[0], -- 1.6.6.115.gd1ab3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/4] run-command: move wait_or_whine earlier 2010-01-10 13:04 ` [PATCH 0/4] Detect exec errors in start_command early Johannes Sixt 2010-01-10 13:07 ` [PATCH 1/4] start_command: report child process setup errors to the parent's stderr Johannes Sixt @ 2010-01-10 13:08 ` Johannes Sixt 2010-01-10 13:11 ` [PATCH 3/4] start_command: detect execvp failures early Johannes Sixt 2010-01-10 13:18 ` [PATCH 4/4] Improve error message when a transport helper was not found Johannes Sixt 3 siblings, 0 replies; 11+ messages in thread From: Johannes Sixt @ 2010-01-10 13:08 UTC (permalink / raw) To: Ilari Liusvaara; +Cc: git We want to reuse it from start_command. Signed-off-by: Johannes Sixt <j6t@kdbg.org> --- run-command.c | 84 ++++++++++++++++++++++++++++---------------------------- 1 files changed, 42 insertions(+), 42 deletions(-) diff --git a/run-command.c b/run-command.c index 02c7bfb..dccac37 100644 --- a/run-command.c +++ b/run-command.c @@ -39,6 +39,48 @@ static inline void set_cloexec(int fd) } #endif +static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure) +{ + 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)) { + 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); + /* + * Convert special exit code when execvp failed. + */ + 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); + } + errno = failed_errno; + return code; +} + int start_command(struct child_process *cmd) { int need_in, need_out, need_err; @@ -272,48 +314,6 @@ fail_pipe: return 0; } -static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure) -{ - 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)) { - 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); - /* - * Convert special exit code when execvp failed. - */ - 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); - } - errno = failed_errno; - return code; -} - int finish_command(struct child_process *cmd) { return wait_or_whine(cmd->pid, cmd->argv[0], cmd->silent_exec_failure); -- 1.6.6.115.gd1ab3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/4] start_command: detect execvp failures early 2010-01-10 13:04 ` [PATCH 0/4] Detect exec errors in start_command early Johannes Sixt 2010-01-10 13:07 ` [PATCH 1/4] start_command: report child process setup errors to the parent's stderr Johannes Sixt 2010-01-10 13:08 ` [PATCH 2/4] run-command: move wait_or_whine earlier Johannes Sixt @ 2010-01-10 13:11 ` Johannes Sixt 2010-01-14 21:31 ` Junio C Hamano 2010-01-10 13:18 ` [PATCH 4/4] Improve error message when a transport helper was not found Johannes Sixt 3 siblings, 1 reply; 11+ messages in thread From: Johannes Sixt @ 2010-01-10 13:11 UTC (permalink / raw) To: Ilari Liusvaara; +Cc: git Previously, failures during execvp could be detected only by finish_command. However, in some situations it is beneficial for the parent process to know earlier that the child process will not run. The idea to use a pipe to signal failures to the parent process and the test case were lifted from patches by Ilari Liusvaara. Signed-off-by: Johannes Sixt <j6t@kdbg.org> --- Makefile | 1 + run-command.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++- t/t0061-run-command.sh | 14 ++++++++++++++ test-run-command.c | 35 +++++++++++++++++++++++++++++++++++ 4 files changed, 96 insertions(+), 1 deletions(-) create mode 100755 t/t0061-run-command.sh create mode 100644 test-run-command.c diff --git a/Makefile b/Makefile index 87fc7ff..22c1546 100644 --- a/Makefile +++ b/Makefile @@ -1785,6 +1785,7 @@ TEST_PROGRAMS += test-genrandom$X TEST_PROGRAMS += test-match-trees$X TEST_PROGRAMS += test-parse-options$X TEST_PROGRAMS += test-path-utils$X +TEST_PROGRAMS += test-run-command$X TEST_PROGRAMS += test-sha1$X TEST_PROGRAMS += test-sigchain$X diff --git a/run-command.c b/run-command.c index dccac37..473b7f8 100644 --- a/run-command.c +++ b/run-command.c @@ -17,6 +17,12 @@ static inline void dup_devnull(int to) #ifndef WIN32 static int child_err = 2; +static int child_notifier = -1; + +static void notify_parent() +{ + write(child_notifier, "", 1); +} static NORETURN void die_child(const char *err, va_list params) { @@ -142,6 +148,11 @@ fail_pipe: trace_argv_printf(cmd->argv, "trace: run_command:"); #ifndef WIN32 +{ + int notify_pipe[2]; + if (pipe(notify_pipe)) + notify_pipe[0] = notify_pipe[1] = -1; + fflush(NULL); cmd->pid = fork(); if (!cmd->pid) { @@ -156,6 +167,11 @@ fail_pipe: } set_die_routine(die_child); + close(notify_pipe[0]); + set_cloexec(notify_pipe[1]); + child_notifier = notify_pipe[1]; + atexit(notify_parent); + if (cmd->no_stdin) dup_devnull(0); else if (need_in) { @@ -196,8 +212,16 @@ fail_pipe: unsetenv(*cmd->env); } } - if (cmd->preexec_cb) + if (cmd->preexec_cb) { + /* + * We cannot predict what the pre-exec callback does. + * Forgo parent notification. + */ + close(child_notifier); + child_notifier = -1; + cmd->preexec_cb(); + } if (cmd->git_cmd) { execv_git_cmd(cmd->argv); } else { @@ -215,6 +239,27 @@ fail_pipe: if (cmd->pid < 0) error("cannot fork() for %s: %s", cmd->argv[0], strerror(failed_errno = errno)); + + /* + * Wait for child's execvp. If the execvp succeeds (or if fork() + * failed), EOF is seen immediately by the parent. Otherwise, the + * child process sends a single byte. + * Note that use of this infrastructure is completely advisory, + * therefore, we keep error checks minimal. + */ + close(notify_pipe[1]); + if (read(notify_pipe[0], ¬ify_pipe[1], 1) == 1) { + /* + * At this point we know that fork() succeeded, but execvp() + * failed. Errors have been reported to our stderr. + */ + wait_or_whine(cmd->pid, cmd->argv[0], + cmd->silent_exec_failure); + failed_errno = errno; + cmd->pid = -1; + } + close(notify_pipe[0]); +} #else { int s0 = -1, s1 = -1, s2 = -1; /* backups of stdin, stdout, stderr */ diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh new file mode 100755 index 0000000..10b26e4 --- /dev/null +++ b/t/t0061-run-command.sh @@ -0,0 +1,14 @@ +#!/bin/sh +# +# Copyright (c) 2009 Ilari Liusvaara +# + +test_description='Test run command' + +. ./test-lib.sh + +test_expect_success 'start_command reports ENOENT' ' + test-run-command start-command-ENOENT ./does-not-exist +' + +test_done diff --git a/test-run-command.c b/test-run-command.c new file mode 100644 index 0000000..a1d5306 --- /dev/null +++ b/test-run-command.c @@ -0,0 +1,35 @@ +/* + * test-run-command.c: test run command API. + * + * (C) 2009 Ilari Liusvaara <ilari.liusvaara@elisanet.fi> + * + * This code is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include "git-compat-util.h" +#include "run-command.h" +#include <string.h> +#include <errno.h> + +int main(int argc, char **argv) +{ + struct child_process proc; + + memset(&proc, 0, sizeof(proc)); + + if(argc < 3) + return 1; + proc.argv = (const char **)argv+2; + + if (!strcmp(argv[1], "start-command-ENOENT")) { + if (start_command(&proc) < 0 && errno == ENOENT) + return 0; + fprintf(stderr, "FAIL %s\n", argv[1]); + return 1; + } + + fprintf(stderr, "check usage\n"); + return 1; +} -- 1.6.6.115.gd1ab3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] start_command: detect execvp failures early 2010-01-10 13:11 ` [PATCH 3/4] start_command: detect execvp failures early Johannes Sixt @ 2010-01-14 21:31 ` Junio C Hamano 2010-01-14 21:53 ` Johannes Sixt 0 siblings, 1 reply; 11+ messages in thread From: Junio C Hamano @ 2010-01-14 21:31 UTC (permalink / raw) To: Johannes Sixt; +Cc: Ilari Liusvaara, git Johannes Sixt <j6t@kdbg.org> writes: > Previously, failures during execvp could be detected only by > finish_command. However, in some situations it is beneficial for the > parent process to know earlier that the child process will not run. > > The idea to use a pipe to signal failures to the parent process and > the test case were lifted from patches by Ilari Liusvaara. I wonder if we can do this without pipe, perhaps using "vfork, exec, then update a variable".... ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] start_command: detect execvp failures early 2010-01-14 21:31 ` Junio C Hamano @ 2010-01-14 21:53 ` Johannes Sixt 2010-01-14 22:40 ` Junio C Hamano 0 siblings, 1 reply; 11+ messages in thread From: Johannes Sixt @ 2010-01-14 21:53 UTC (permalink / raw) To: Junio C Hamano; +Cc: Ilari Liusvaara, git On Donnerstag, 14. Januar 2010, Junio C Hamano wrote: > Johannes Sixt <j6t@kdbg.org> writes: > > Previously, failures during execvp could be detected only by > > finish_command. However, in some situations it is beneficial for the > > parent process to know earlier that the child process will not run. > > > > The idea to use a pipe to signal failures to the parent process and > > the test case were lifted from patches by Ilari Liusvaara. > > I wonder if we can do this without pipe, perhaps using "vfork, exec, then > update a variable".... Except that some systems implement vfork(2) in terms of fork(2), I heard. Moreover, I think that we do way too many things before the exec; isn't that dangerous? -- Hannes ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] start_command: detect execvp failures early 2010-01-14 21:53 ` Johannes Sixt @ 2010-01-14 22:40 ` Junio C Hamano 0 siblings, 0 replies; 11+ messages in thread From: Junio C Hamano @ 2010-01-14 22:40 UTC (permalink / raw) To: Johannes Sixt; +Cc: Ilari Liusvaara, git Johannes Sixt <j6t@kdbg.org> writes: > On Donnerstag, 14. Januar 2010, Junio C Hamano wrote: >> Johannes Sixt <j6t@kdbg.org> writes: >> > Previously, failures during execvp could be detected only by >> > finish_command. However, in some situations it is beneficial for the >> > parent process to know earlier that the child process will not run. >> > >> > The idea to use a pipe to signal failures to the parent process and >> > the test case were lifted from patches by Ilari Liusvaara. >> >> I wonder if we can do this without pipe, perhaps using "vfork, exec, then >> update a variable".... > > Except that some systems implement vfork(2) in terms of fork(2), I heard. As long as they implement it correctly, we don't care if it is implemented in terms of fork or knife or chopstick. The only thing we care about in this crazy-idea was the shared address space the child can write into until it exec or _exit while the parent is in a frozen state. > Moreover, I think that we do way too many things before the exec; isn't that > dangerous? Yeah, we do seem to do quite a lot more than I thought. Scratch that crazy idea, then. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/4] Improve error message when a transport helper was not found 2010-01-10 13:04 ` [PATCH 0/4] Detect exec errors in start_command early Johannes Sixt ` (2 preceding siblings ...) 2010-01-10 13:11 ` [PATCH 3/4] start_command: detect execvp failures early Johannes Sixt @ 2010-01-10 13:18 ` Johannes Sixt 3 siblings, 0 replies; 11+ messages in thread From: Johannes Sixt @ 2010-01-10 13:18 UTC (permalink / raw) To: Ilari Liusvaara; +Cc: git From: Ilari Liusvaara <ilari.liusvaara@elisanet.fi> Perviously, the error message was: git: 'remote-foo' is not a git-command. See 'git --help'. By not treating the transport helper as a git command, a more suitable error is reported: fatal: Unable to find remote helper for 'foo' Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi> Signed-off-by: Johannes Sixt <j6t@kdbg.org> --- While I agree mostly with the implementation of your patch, the justification was completely off, and I think it was because you did not notice that the parent process died from SIGPIPE. When patches 1-3/4 are applied, the parent does not die from SIGPIPE anymore, and the issue is now only minor (an unsuitable error message instead of no error message). Therefore, I rewrote the commit message to tone it down. -- Hannes transport-helper.c | 14 ++++++++++---- 1 files changed, 10 insertions(+), 4 deletions(-) diff --git a/transport-helper.c b/transport-helper.c index 6ece0d9..d1bc3af 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -102,6 +102,7 @@ static struct child_process *get_helper(struct transport *transport) int refspec_nr = 0; int refspec_alloc = 0; int duped; + int code; if (data->helper) return data->helper; @@ -111,13 +112,18 @@ static struct child_process *get_helper(struct transport *transport) helper->out = -1; helper->err = 0; helper->argv = xcalloc(4, sizeof(*helper->argv)); - strbuf_addf(&buf, "remote-%s", data->name); + strbuf_addf(&buf, "git-remote-%s", data->name); helper->argv[0] = strbuf_detach(&buf, NULL); helper->argv[1] = transport->remote->name; helper->argv[2] = remove_ext_force(transport->url); - helper->git_cmd = 1; - if (start_command(helper)) - die("Unable to run helper: git %s", helper->argv[0]); + helper->git_cmd = 0; + helper->silent_exec_failure = 1; + code = start_command(helper); + if (code < 0 && errno == ENOENT) + die("Unable to find remote helper for '%s'", data->name); + else + exit(code); + data->helper = helper; data->no_disconnect_req = 0; -- 1.6.6.115.gd1ab3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [RFC PATCH v2 2/2] Improve transport helper exec failure reporting 2010-01-09 13:45 [RFC PATCH v2 0/2] Imporove remote helper exec failure reporting Ilari Liusvaara 2010-01-09 13:45 ` [RFC PATCH v2 1/2] Report exec errors from run-command Ilari Liusvaara @ 2010-01-09 13:45 ` Ilari Liusvaara 1 sibling, 0 replies; 11+ messages in thread From: Ilari Liusvaara @ 2010-01-09 13:45 UTC (permalink / raw) To: git Previously transport-helper exec failure error reporting was pretty much useless as it didn't report errors from execve, only from pipe and fork. Now that run-command passes errno from exec, use the improved support to actually print useful errors if execution fails. Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi> --- transport-helper.c | 27 +++++++++++++++++++++++---- 1 files changed, 23 insertions(+), 4 deletions(-) diff --git a/transport-helper.c b/transport-helper.c index 5147296..72be965 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -102,6 +102,7 @@ static struct child_process *get_helper(struct transport *transport) int refspec_nr = 0; int refspec_alloc = 0; int duped; + int code; if (data->helper) return data->helper; @@ -111,13 +112,31 @@ static struct child_process *get_helper(struct transport *transport) helper->out = -1; helper->err = 0; helper->argv = xcalloc(4, sizeof(*helper->argv)); - strbuf_addf(&buf, "remote-%s", data->name); + strbuf_addf(&buf, "git-remote-%s", data->name); helper->argv[0] = strbuf_detach(&buf, NULL); helper->argv[1] = transport->remote->name; helper->argv[2] = remove_ext_force(transport->url); - helper->git_cmd = 1; - if (start_command(helper)) - die("Unable to run helper: git %s", helper->argv[0]); + helper->argv[2] = transport->url; + helper->git_cmd = 0; + helper->silent_exec_failure = 1; + helper->extended_error_code = 1; + /* + * Print errors even if start_command should have printed them in + * order to get proper fatal-level error message and include + * errnos to make inevitable incomplete problem reports more useful + * (those have nasty tendency of including only the last line that + * looks like error). + */ + code = start_command(helper); + if (code == -2) + die("Unable to find remote helper for '%s'", + data->name); + else if(code == -1) + die_errno("Error running helper for '%s'", data->name); + else if(code != 0) + /* Shouldn't happen. */ + die_errno("code %i running helper for '%s'", code, data->name); + data->helper = helper; data->no_disconnect_req = 0; -- 1.6.6.3.gaa2e1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2010-01-14 22:40 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-01-09 13:45 [RFC PATCH v2 0/2] Imporove remote helper exec failure reporting Ilari Liusvaara 2010-01-09 13:45 ` [RFC PATCH v2 1/2] Report exec errors from run-command Ilari Liusvaara 2010-01-10 13:04 ` [PATCH 0/4] Detect exec errors in start_command early Johannes Sixt 2010-01-10 13:07 ` [PATCH 1/4] start_command: report child process setup errors to the parent's stderr Johannes Sixt 2010-01-10 13:08 ` [PATCH 2/4] run-command: move wait_or_whine earlier Johannes Sixt 2010-01-10 13:11 ` [PATCH 3/4] start_command: detect execvp failures early Johannes Sixt 2010-01-14 21:31 ` Junio C Hamano 2010-01-14 21:53 ` Johannes Sixt 2010-01-14 22:40 ` Junio C Hamano 2010-01-10 13:18 ` [PATCH 4/4] Improve error message when a transport helper was not found Johannes Sixt 2010-01-09 13:45 ` [RFC PATCH v2 2/2] Improve transport helper exec failure reporting Ilari Liusvaara
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).