{"thread":{"id":"22144","subject":"[RFC PATCH v2 0/2] Imporove remote helper exec failure reporting","startedAt":"2010-01-09T13:45:55Z","lastAt":"2010-01-14T22:40:27Z","messageCount":11,"participants":["Ilari Liusvaara","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"131181","messageId":"1263044757-12560-1-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"22144","inReplyTo":null,"subject":"[RFC PATCH v2 0/2] Imporove remote helper exec failure reporting","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2010-01-09T13:45:55Z","receivedAt":"2010-01-09T13:45:55Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"(This is newer than updated patch v3).\n\nI decided to go on a bit further with new features, and that's why this\nis back as RFC.\n\nDiffrences from first RFC round:\n\n- Split loop-over-close to its own function and warn but not retry on\n  bizarre error codes (basically anything except EBADF and EINTR).\n- Don't rely on pipe(2) to preserve the fd array on failure.\n- Add some testcases.\n- Fix buggy behaviour if report pipe read end winds up as fd #0.\n- Set silent_exec_failure on executing remote helper.\n- Make exec reporting in presence of preexec_cb controlled by flag to\n  solve pager deadlock problems without preventing using preexec_cb\n  with exec reporting entierely (the way pager is invoked is just\n  fundamentially incompatible with exec reporting).\n- Add flag to make run-command give different code for exec failing due to\n  ENOENT than for other failures (including other ways to get ENOENT).\n- change chdir failure to be reported as error (and pass error code back).\n  BTW: Nothing seems to use that chdir feature, aside of new testcase.\n- Report errors to parent's stderr.\n- Don't retry waitpid on bizarre error codes (basically anything except\n  EINTR).\n- Make run_command actually complain about exec failing (as API docs\n  say).\n\nIlari Liusvaara (2):\n  Report exec errors from run-command\n  Improve transport helper exec failure reporting\n\n Documentation/technical/api-run-command.txt |    2 +\n Makefile                                    |    1 +\n run-command.c                               |  196 +++++++++++++++++++++++++--\n run-command.h                               |    2 +\n t/t0061-run-command.sh                      |   20 +++\n test-run-command.c                          |   74 ++++++++++\n transport-helper.c                          |   27 +++-\n 7 files changed, 305 insertions(+), 17 deletions(-)\n create mode 100755 t/t0061-run-command.sh\n create mode 100644 test-run-command.c\n"},{"id":"131183","messageId":"1263044757-12560-2-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"22144","inReplyTo":"1263044757-12560-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"[RFC PATCH v2 1/2] Report exec errors from run-command","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2010-01-09T13:45:56Z","receivedAt":"2010-01-09T13:45:56Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"Previously run-command was unable to report errors happening in exec\ncall. Change it to pass errno from failed exec to errno value at\nreturn. Also add special mode that diffrentiates between ENOENT\nfrom exec and ENOENT from other source.\n\nThe errno value passing can be done by opening close-on-exec pipe and\npiping the error code through in case of failure. In case of success,\nclose-on-exec closes the pipe on successful exec and parent process\ngets end of file on read.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\nSigned-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>\n---\n Documentation/technical/api-run-command.txt |    2 +\n Makefile                                    |    1 +\n run-command.c                               |  196 +++++++++++++++++++++++++--\n run-command.h                               |    2 +\n t/t0061-run-command.sh                      |   20 +++\n test-run-command.c                          |   74 ++++++++++\n 6 files changed, 282 insertions(+), 13 deletions(-)\n create mode 100755 t/t0061-run-command.sh\n create mode 100644 test-run-command.c\n\ndiff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt\nindex b26c281..f37d8ba 100644\n--- a/Documentation/technical/api-run-command.txt\n+++ b/Documentation/technical/api-run-command.txt\n@@ -49,6 +49,8 @@ The functions above do the following:\n \n . If the program was not found, then -1 is returned and errno is set to\n   ENOENT; a diagnostic is printed only if .silent_exec_failure is 0.\n+  (if extended_error_code flag is true, instead of -1, the return value\n+  is -2 in this case).\n \n . Otherwise, the program is run. If it terminates regularly, its exit\n   code is returned. No diagnistic is printed, even if the exit code is\ndiff --git a/Makefile b/Makefile\nindex a2fce5a..92eed68 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1753,6 +1753,7 @@ TEST_PROGRAMS_NEED_X += test-path-utils\n TEST_PROGRAMS_NEED_X += test-sha1\n TEST_PROGRAMS_NEED_X += test-sigchain\n TEST_PROGRAMS_NEED_X += test-index-version\n+TEST_PROGRAMS_NEED_X += test-run-command\n \n TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))\n \ndiff --git a/run-command.c b/run-command.c\nindex 47ced57..b1af9c4 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -15,6 +15,20 @@ static inline void dup_devnull(int to)\n \tclose(fd);\n }\n \n+static inline void force_close(int fd)\n+{\n+\t/*\n+\t * Assume all errors except EINTR are persistent and\n+\t * that EBADF signals already closed fd.\n+\t */\n+\twhile (close(fd) < 0 && errno != EBADF)\n+\t\tif (errno != EINTR) {\n+\t\t\twarning(\"Close returned error %s\",\n+\t\t\t\tstrerror(errno));\n+\t\t\tbreak;\n+\t\t}\n+}\n+\n static const char **prepare_shell_cmd(const char **argv)\n {\n \tint argc, nargc = 0;\n@@ -49,6 +63,8 @@ static const char **prepare_shell_cmd(const char **argv)\n }\n \n #ifndef WIN32\n+int child_err = -1;\n+\n static int execv_shell_cmd(const char **argv)\n {\n \tconst char **nargv = prepare_shell_cmd(argv);\n@@ -57,6 +73,26 @@ static int execv_shell_cmd(const char **argv)\n \tfree(nargv);\n \treturn -1;\n }\n+\n+static void error_child(const char *err, ...)\n+{\n+\tchar msg[4096];\n+\tint len;\n+\tva_list args;\n+\tva_start(args, err);\n+\tlen = vsnprintf(msg, sizeof(msg), err, args);\n+\tva_end(args);\n+\tif (len >= sizeof(msg))\n+\t\tlen = sizeof(msg) - 1;\n+\n+\tif (child_err < 0)\n+\t\treturn;\n+\n+\twrite(child_err, \"error: \", 7);\n+\twrite(child_err, msg, len);\n+\twrite(child_err, \"\\n\", 1);\n+}\n+\n #endif\n \n int start_command(struct child_process *cmd)\n@@ -64,6 +100,7 @@ int start_command(struct child_process *cmd)\n \tint need_in, need_out, need_err;\n \tint fdin[2], fdout[2], fderr[2];\n \tint failed_errno = failed_errno;\n+\tint func_ret = 0;\n \n \t/*\n \t * In case of errors we must keep the promise to close FDs\n@@ -120,9 +157,91 @@ fail_pipe:\n \ttrace_argv_printf(cmd->argv, \"trace: run_command:\");\n \n #ifndef WIN32\n+{\n+\tint report_pipe[2] = {-1, -1};\n+\n+\tif (pipe(report_pipe) < 0) {\n+\t\treport_pipe[0] = -1;\n+\t\treport_pipe[1] = -1;\n+\t\twarning(\"Can't open pipe for exec status report: %s\\n\",\n+\t\t\tstrerror(errno));\n+\t}\n+\n \tfflush(NULL);\n \tcmd->pid = fork();\n-\tif (!cmd->pid) {\n+\tif (cmd->pid > 0) {\n+\t\tint r = 0, ret = 0;\n+\t\tforce_close(report_pipe[1]);\n+read_again:\n+\t\tif (report_pipe[0] >= 0)\n+\t\t\tr = read(report_pipe[0], &ret, sizeof(ret));\n+\t\tif (r < 0 && (errno == EAGAIN || errno == EINTR ||\n+\t\t\terrno == EWOULDBLOCK))\n+\t\t\tgoto read_again;\n+\t\telse if (r < 0)\n+\t\t\twarning(\"Can't read exec status report: %s\\n\",\n+\t\t\t\tstrerror(errno));\n+\t\telse if (r == 0)\n+\t\t\t;\n+\t\telse if (r < sizeof(ret)) {\n+\t\t\twarning(\"Received incomplete exec status report.\\n\");\n+\t\t} else {\n+\t\t\tfailed_errno = ret;\n+\t\t\t/*\n+\t\t\t * Clean up the process that did the failed execution\n+\t\t\t * so no zombies remain.\n+\t\t\t */\n+\t\t\twhile (waitpid(cmd->pid, &ret, 0) < 0 && errno == EINTR)\n+\t\t\t\t/* Nothing. */ ;\n+\t\t\tif (WIFEXITED(ret) && WEXITSTATUS(ret) == 127)\n+\t\t\t\t/* Program was not found. */\n+\t\t\t\tfunc_ret = -2;\n+\t\t\tcmd->pid = -1;\n+\t\t}\n+\t} else if (!cmd->pid) {\n+\t\tint r = 0;\n+\t\tint errored = 0;\n+\t\tint flags;\n+\t\tforce_close(report_pipe[0]);\n+\n+\t\tflags = fcntl(report_pipe[1], F_GETFD);\n+\t\tif (flags < 0)\n+\t\t\tr = -1;\n+\t\tr = (r || fcntl(report_pipe[1], F_SETFD, flags | FD_CLOEXEC));\n+\t\tif (r) {\n+\t\t\twarning(\"Can't FD_CLOEXEC pipe for exec status report: %s\",\n+\t\t\t\tstrerror(errno));\n+\t\t\tforce_close(report_pipe[1]);\n+\t\t\treport_pipe[1] = -1;\n+\t\t}\n+\n+\t\t/*\n+\t\t * Redirect the channel to write syscall error messages to\n+\t\t * before redirecting the process's stderr so that all error\n+\t\t * messages in subsequent call paths use the parent's stderr.\n+\t\t */\n+\t\tif (cmd->no_stderr || need_err) {\n+\t\t\tchild_err = dup(2);\n+\t\t\tif (child_err < 0 && errno != EBADF)\n+\t\t\t\t/* EBADF => No stderr, nowhere to go. */\n+\t\t\t\twarning(\"Can't dup fd for child exec errors: %s\",\n+\t\t\t\t\tstrerror(errno));\n+\t\t\tif (child_err < 0)\n+\t\t\t\tgoto no_error_chan;\n+\n+\t\t\tflags = fcntl(child_err, F_GETFL);\n+\t\t\tif (flags < 0)\n+\t\t\t\tflags = 0;\n+\t\t\tif (fcntl(child_err, F_SETFL, flags | FD_CLOEXEC) < 0) {\n+\t\t\t\twarning(\"Can't FD_CLOEXEC fd for child exec errors: %s\",\n+\t\t\t\t\tstrerror(errno));\n+\t\t\t\tforce_close(report_pipe[1]);\n+\t\t\t\tchild_err = -1;\n+\t\t\t}\n+\t\t} else\n+\t\t\tchild_err = 2;\n+no_error_chan:\n+\n \t\tif (cmd->no_stdin)\n \t\t\tdup_devnull(0);\n \t\telse if (need_in) {\n@@ -152,9 +271,15 @@ fail_pipe:\n \t\t\tclose(cmd->out);\n \t\t}\n \n-\t\tif (cmd->dir && chdir(cmd->dir))\n-\t\t\tdie_errno(\"exec '%s': cd to '%s' failed\", cmd->argv[0],\n-\t\t\t    cmd->dir);\n+\t\tif (cmd->dir && chdir(cmd->dir)) {\n+\t\t\tfailed_errno = errno;\n+\t\t\terror_child(\"exec '%s': cd to '%s' failed: %s\",\n+\t\t\t\tcmd->argv[0], cmd->dir, strerror(errno));\n+\t\t\ttrace_printf(\"trace: exec '%s': cd to '%s' failed: %s\\n\",\n+\t\t\t\tcmd->argv[0], cmd->dir, strerror(errno));\n+\t\t\terrored = 1;\n+\t\t\tgoto throw_fault;\n+\t\t}\n \t\tif (cmd->env) {\n \t\t\tfor (; *cmd->env; cmd->env++) {\n \t\t\t\tif (strchr(*cmd->env, '='))\n@@ -163,8 +288,23 @@ fail_pipe:\n \t\t\t\t\tunsetenv(*cmd->env);\n \t\t\t}\n \t\t}\n-\t\tif (cmd->preexec_cb)\n+\t\tif (cmd->preexec_cb) {\n+\t\t\t/*\n+\t\t\t * We don't know what pre-exec callbacks do, and they\n+\t\t\t * may do something that causes deadlock with exec\n+\t\t\t * reporting. The sole user of this hook seems to\n+\t\t\t * be pager, and it is run through shell, so one\n+\t\t\t * wouldn't get useful error from exec reporting\n+\t\t\t * and would get useful error from shell anyway. So\n+\t\t\t * just disable exec reporting for such comamnds.\n+\t\t\t * (unless explicitly specified otherwise).\n+\t\t\t */\n+\t\t\tif (!cmd->report_preexec_cb) {\n+\t\t\t\tforce_close(report_pipe[1]);\n+\t\t\t\treport_pipe[1] = -1;\n+\t\t\t}\n \t\t\tcmd->preexec_cb();\n+\t\t}\n \t\tif (cmd->git_cmd) {\n \t\t\texecv_git_cmd(cmd->argv);\n \t\t} else if (cmd->use_shell) {\n@@ -172,13 +312,36 @@ fail_pipe:\n \t\t} else {\n \t\t\texecvp(cmd->argv[0], (char *const*) cmd->argv);\n \t\t}\n+\t\tfailed_errno = errno;\n+\n \t\ttrace_printf(\"trace: exec '%s' failed: %s\\n\", cmd->argv[0],\n \t\t\t\tstrerror(errno));\n-\t\texit(127);\n-\t}\n-\tif (cmd->pid < 0)\n+\n+\t\tif (!cmd->silent_exec_failure || errno != ENOENT)\n+\t\t\terror_child(\"cannot exec '%s': %s\",\n+\t\t\t\tcmd->argv[0], strerror(failed_errno));\n+throw_fault:\n+\t\tr = 0;\n+write_again:\n+\t\tif (report_pipe[1] >= 0)\n+\t\t\tr = write(report_pipe[1], &failed_errno,\n+\t\t\t\tsizeof(failed_errno));\n+\t\tif (r < 0 && (errno == EAGAIN || errno == EINTR ||\n+\t\t\terrno == EWOULDBLOCK))\n+\t\t\tgoto write_again;\n+\n+\t\t/*\n+\t\t * Diffrentiate between program doesn't exist and other\n+\t\t * errors.\n+\t\t */\n+\t\tif (errored || errno != ENOENT)\n+\t\t\texit(128);\n+\t\telse\n+\t\t\texit(127);\n+\t} else if (cmd->pid < 0)\n \t\terror(\"cannot fork() for %s: %s\", cmd->argv[0],\n \t\t\tstrerror(failed_errno = errno));\n+}\n #else\n {\n \tint s0 = -1, s1 = -1, s2 = -1;\t/* backups of stdin, stdout, stderr */\n@@ -233,6 +396,8 @@ fail_pipe:\n \tfailed_errno = errno;\n \tif (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))\n \t\terror(\"cannot spawn %s: %s\", cmd->argv[0], strerror(errno));\n+\tif (cmd->pid < 0 && errno == ENOENT)\n+\t\tfunc_ret = -2;\n \n \tif (cmd->env)\n \t\tfree_environ(env);\n@@ -261,7 +426,10 @@ fail_pipe:\n \t\tif (need_err)\n \t\t\tclose_pair(fderr);\n \t\terrno = failed_errno;\n-\t\treturn -1;\n+\t\tif (cmd->extended_error_code && func_ret)\n+\t\t\treturn func_ret;\n+\t\telse\n+\t\t\treturn -1;\n \t}\n \n \tif (need_in)\n@@ -280,7 +448,8 @@ fail_pipe:\n \treturn 0;\n }\n \n-static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n+static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure,\n+\tint extended_err)\n {\n \tint status, code = -1;\n \tpid_t waiting;\n@@ -309,7 +478,7 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n \t\t * Convert special exit code when execvp failed.\n \t\t */\n \t\tif (code == 127) {\n-\t\t\tcode = -1;\n+\t\t\tcode = extended_err ? -2 : -1;\n \t\t\tfailed_errno = ENOENT;\n \t\t\tif (!silent_exec_failure)\n \t\t\t\terror(\"cannot run %s: %s\", argv0,\n@@ -324,7 +493,8 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n \n int finish_command(struct child_process *cmd)\n {\n-\treturn wait_or_whine(cmd->pid, cmd->argv[0], cmd->silent_exec_failure);\n+\treturn wait_or_whine(cmd->pid, cmd->argv[0], cmd->silent_exec_failure,\n+\t\tcmd->extended_error_code);\n }\n \n int run_command(struct child_process *cmd)\n@@ -410,7 +580,7 @@ int start_async(struct async *async)\n int finish_async(struct async *async)\n {\n #ifndef WIN32\n-\tint ret = wait_or_whine(async->pid, \"child process\", 0);\n+\tint ret = wait_or_whine(async->pid, \"child process\", 0, 0);\n #else\n \tDWORD ret = 0;\n \tif (WaitForSingleObject(async->tid, INFINITE) != WAIT_OBJECT_0)\ndiff --git a/run-command.h b/run-command.h\nindex 967ba8c..f6b4caf 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -34,6 +34,8 @@ struct child_process {\n \tunsigned silent_exec_failure:1;\n \tunsigned stdout_to_stderr:1;\n \tunsigned use_shell:1;\n+\tunsigned extended_error_code:1;\n+\tunsigned report_preexec_cb:1;\n \tvoid (*preexec_cb)(void);\n };\n \ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nnew file mode 100755\nindex 0000000..0c217b1\n--- /dev/null\n+++ b/t/t0061-run-command.sh\n@@ -0,0 +1,20 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2009 Ilari Liusvaara\n+#\n+\n+test_description='Test run command'\n+\n+. ./test-lib.sh\n+\n+test_expect_success \"reporting ENOENT (non-extended)\" \\\n+\"test-run-command 2\"\n+\n+test_expect_success \"reporting ENOENT (extended)\" \\\n+\"test-run-command 1\"\n+\n+test_expect_success \"reporting chdir failed (extended)\" \\\n+\"test-run-command 3\"\n+\n+\n+test_done\ndiff --git a/test-run-command.c b/test-run-command.c\nnew file mode 100644\nindex 0000000..db664f3\n--- /dev/null\n+++ b/test-run-command.c\n@@ -0,0 +1,74 @@\n+/*\n+ * test-run-command.c: test run command API.\n+ *\n+ * (C) 2009 Ilari Liusvaara <ilari.liusvaara@elisanet.fi>\n+ *\n+ * This code is free software; you can redistribute it and/or modify\n+ * it under the terms of the GNU General Public License version 2 as\n+ * published by the Free Software Foundation.\n+ */\n+\n+#include \"git-compat-util.h\"\n+#include \"run-command.h\"\n+#include <string.h>\n+#include <errno.h>\n+\n+int main(int argc, char **argv)\n+{\n+\tchar* procs[2];\n+\tstruct child_process proc;\n+\tchar test = argv[1][0];\n+\tmemset(&proc, 0, sizeof(proc));\n+\n+\tif(argc < 2)\n+\t\treturn 1;\n+\n+\n+\tswitch(test) {\n+\tcase '1':\n+\t\tprocs[0] = \"does-not-exist-62896869286\";\n+\t\tprocs[1] = NULL;\n+\t\tproc.argv = (const char **)procs;\n+\t\tproc.extended_error_code = 1;\n+\n+\t\tif (start_command(&proc) != -2) {\n+\t\t\tfprintf(stderr, \"FAIL A\\n\");\n+\t\t\treturn 1;\n+\t\t}\n+\t\tif (test == '1' && errno == ENOENT)\n+\t\t\treturn 0;\n+\tcase '2':\n+\t\tprocs[0] = \"does-not-exist-62896869286\";\n+\t\tprocs[1] = NULL;\n+\t\tproc.argv = (const char **)procs;\n+\t\tproc.extended_error_code = 0;\n+\n+\t\tif (start_command(&proc) != -1) {\n+\t\t\tfprintf(stderr, \"FAIL A\\n\");\n+\t\t\treturn 1;\n+\t\t}\n+\t\tif (errno == ENOENT)\n+\t\t\treturn 0;\n+\tcase '3':\n+#ifdef WIN32\n+\t\t/* This can't work on Win32, so just skip it. */\n+\t\treturn 0;\n+#endif\n+\t\tprocs[0] = \"does-not-exist-62896869286\";\n+\t\tprocs[1] = NULL;\n+\t\tproc.argv = (const char **)procs;\n+\t\tproc.dir = \"does-not-exist-52632632733\";\n+\t\tproc.extended_error_code = 1;\n+\n+\t\tif (start_command(&proc) != -1) {\n+\t\t\tfprintf(stderr, \"FAIL A\\n\");\n+\t\t\treturn 1;\n+\t\t}\n+\t\tif (errno == ENOENT)\n+\t\t\treturn 0;\n+\t}\n+\n+\tfprintf(stderr, \"FAIL B\\n\");\n+\n+\treturn 1;\n+}\n-- \n1.6.6.3.gaa2e1\n"},{"id":"131182","messageId":"1263044757-12560-3-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"22144","inReplyTo":"1263044757-12560-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"[RFC PATCH v2 2/2] Improve transport helper exec failure reporting","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2010-01-09T13:45:57Z","receivedAt":"2010-01-09T13:45:57Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"Previously transport-helper exec failure error reporting was pretty\nmuch useless as it didn't report errors from execve, only from pipe\nand fork. Now that run-command passes errno from exec, use the\nimproved support to actually print useful errors if execution fails.\n\nSigned-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>\n---\n transport-helper.c |   27 +++++++++++++++++++++++----\n 1 files changed, 23 insertions(+), 4 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 5147296..72be965 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -102,6 +102,7 @@ static struct child_process *get_helper(struct transport *transport)\n \tint refspec_nr = 0;\n \tint refspec_alloc = 0;\n \tint duped;\n+\tint code;\n \n \tif (data->helper)\n \t\treturn data->helper;\n@@ -111,13 +112,31 @@ static struct child_process *get_helper(struct transport *transport)\n \thelper->out = -1;\n \thelper->err = 0;\n \thelper->argv = xcalloc(4, sizeof(*helper->argv));\n-\tstrbuf_addf(&buf, \"remote-%s\", data->name);\n+\tstrbuf_addf(&buf, \"git-remote-%s\", data->name);\n \thelper->argv[0] = strbuf_detach(&buf, NULL);\n \thelper->argv[1] = transport->remote->name;\n \thelper->argv[2] = remove_ext_force(transport->url);\n-\thelper->git_cmd = 1;\n-\tif (start_command(helper))\n-\t\tdie(\"Unable to run helper: git %s\", helper->argv[0]);\n+\thelper->argv[2] = transport->url;\n+\thelper->git_cmd = 0;\n+\thelper->silent_exec_failure = 1;\n+\thelper->extended_error_code = 1;\n+\t/*\n+\t * Print errors even if start_command should have printed them in\n+\t * order to get proper fatal-level error message and include\n+\t * errnos to make inevitable incomplete problem reports more useful\n+\t * (those have nasty tendency of including only the last line that\n+\t * looks like error).\n+\t */\n+\tcode = start_command(helper);\n+\tif (code == -2)\n+\t\tdie(\"Unable to find remote helper for '%s'\",\n+\t\t\tdata->name);\n+\telse if(code == -1)\n+\t\tdie_errno(\"Error running helper for '%s'\", data->name);\n+\telse if(code != 0)\n+\t\t/* Shouldn't happen. */\n+\t\tdie_errno(\"code %i running helper for '%s'\", code, data->name);\n+\n \tdata->helper = helper;\n \tdata->no_disconnect_req = 0;\n \n-- \n1.6.6.3.gaa2e1\n"},{"id":"131227","messageId":"201001101404.22258.j6t@kdbg.org","threadId":"22144","inReplyTo":"1263044757-12560-2-git-send-email-ilari.liusvaara@elisanet.fi","subject":"[PATCH 0/4] Detect exec errors in start_command early","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-10T13:04:21Z","receivedAt":"2010-01-10T13:04:21Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Samstag, 9. Januar 2010, Ilari Liusvaara wrote:\n>  6 files changed, 282 insertions(+), 13 deletions(-)\n\nIMHO, you are going completely overboard with your patch.\n\nYou check every single possible and unlikely error condition in the new code \nthat you introde, but ignore that there are quite a number of calls in the \nexisting code whose error conditions go checked.\n\nAnd as a result, you \"only\" report errno values of some calls, and ignore that \nthere is at least one potential die() call hidden in execv_git_cmd(). IOW, \nyour code is not a catch-all and only an isolated solution.\n\nI hope I can do better.\n\nI developed this series on top of il/vcs-helper. But patches 1-3/4 are \nactually an independent topic and also suitable to be merged into \njk/run-command-use-shell to implement improved DWIM whether a shell is \nneeded.\n\nPatch 4/4 is to address your problem of weak error reporting with transport \nhelpers. I think I understand now what your problem is -- namely that \npreviously no error was reported if a transport helper program did not exist. \nThe reason for the missing error message is IMO a design weakness in the \nprotocol: It requires that the parent talks first - and by talking to an \nearly-died child process it dies from SIGPIPE. I experimented with \nsignal(SIGPIPE, SIG_IGN) to get around this, but it didn't work (the SIGPIPE \narrived anyway).\n\nAnyway, patch 4/4 goes on top of il/vcs-helper after merging the topic \nconsisting of 1-3/4.\n\nIlari Liusvaara (1):\n  Improve error message when a transport helper was not found\n\nJohannes Sixt (3):\n  start_command: report child process setup errors to the parent's\n    stderr\n  run-command: move wait_or_whine earlier\n  start_command: detect execvp failures early\n\n Makefile               |    1 +\n run-command.c          |  177 \n+++++++++++++++++++++++++++++++++++-------------\n t/t0061-run-command.sh |   14 ++++\n test-run-command.c     |   35 ++++++++++\n transport-helper.c     |   14 +++-\n 5 files changed, 191 insertions(+), 50 deletions(-)\n create mode 100755 t/t0061-run-command.sh\n create mode 100644 test-run-command.c\n"},{"id":"131228","messageId":"201001101407.52281.j6t@kdbg.org","threadId":"22144","inReplyTo":"201001101404.22258.j6t@kdbg.org","subject":"[PATCH 1/4] start_command: report child process setup errors to the parent's stderr","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-10T13:07:52Z","receivedAt":"2010-01-10T13:07:52Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"When the child process's environment is set up in start_command(), error\nmessages were written to wherever the parent redirected the child's stderr\nchannel. However, even if the parent redirected the child's stderr, errors\nduring this setup process, including the exec itself, are usually an\nindication of a problem in the parent's environment. Therefore, the error\nmessages should go to the parent's stderr.\n\nRedirection of the child's error messages is usually only used to redirect\nhook error messages during client-server exchanges. In these cases, hook\nsetup errors could be regarded as information leak.\n\nThis patch makes a copy of stderr if necessary and uses a special\ndie routine that is used for all die() calls in the child that sends the\nerrors messages to the parent's stderr.\n\nThe trace call that reported a failed execvp is removed (because it writes\nto stderr) and replaced by die_errno() with special treatment of ENOENT.\nThe improvement in the error message can be seen with this sequence:\n\n   mkdir .git/hooks/pre-commit\n   git commit\n\nPreviously, the error message was\n\n   error: cannot run .git/hooks/pre-commit: No such file or directory\n\nand now it is\n\n   fatal: cannot exec '.git/hooks/pre-commit': Permission denied\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n This should address your concern that \"errors go to who-knows-where\",\n that I meanwhile share with you.\n\n run-command.c |   46 +++++++++++++++++++++++++++++++++++++++++++---\n 1 files changed, 43 insertions(+), 3 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex cf2d8f7..02c7bfb 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -15,6 +15,30 @@ static inline void dup_devnull(int to)\n \tclose(fd);\n }\n \n+#ifndef WIN32\n+static int child_err = 2;\n+\n+static NORETURN void die_child(const char *err, va_list params)\n+{\n+\tchar msg[4096];\n+\tint len = vsnprintf(msg, sizeof(msg), err, params);\n+\tif (len > sizeof(msg))\n+\t\tlen = sizeof(msg);\n+\n+\twrite(child_err, \"fatal: \", 7);\n+\twrite(child_err, msg, len);\n+\twrite(child_err, \"\\n\", 1);\n+\texit(128);\n+}\n+\n+static inline void set_cloexec(int fd)\n+{\n+\tint flags = fcntl(fd, F_GETFD);\n+\tif (flags >= 0)\n+\t\tfcntl(fd, F_SETFD, flags | FD_CLOEXEC);\n+}\n+#endif\n+\n int start_command(struct child_process *cmd)\n {\n \tint need_in, need_out, need_err;\n@@ -79,6 +103,17 @@ fail_pipe:\n \tfflush(NULL);\n \tcmd->pid = fork();\n \tif (!cmd->pid) {\n+\t\t/*\n+\t\t * Redirect the channel to write syscall error messages to\n+\t\t * before redirecting the process's stderr so that all die()\n+\t\t * in subsequent call paths use the parent's stderr.\n+\t\t */\n+\t\tif (cmd->no_stderr || need_err) {\n+\t\t\tchild_err = dup(2);\n+\t\t\tset_cloexec(child_err);\n+\t\t}\n+\t\tset_die_routine(die_child);\n+\n \t\tif (cmd->no_stdin)\n \t\t\tdup_devnull(0);\n \t\telse if (need_in) {\n@@ -126,9 +161,14 @@ fail_pipe:\n \t\t} else {\n \t\t\texecvp(cmd->argv[0], (char *const*) cmd->argv);\n \t\t}\n-\t\ttrace_printf(\"trace: exec '%s' failed: %s\\n\", cmd->argv[0],\n-\t\t\t\tstrerror(errno));\n-\t\texit(127);\n+\t\t/*\n+\t\t * Do not check for cmd->silent_exec_failure; the parent\n+\t\t * process will check it when it sees this exit code.\n+\t\t */\n+\t\tif (errno == ENOENT)\n+\t\t\texit(127);\n+\t\telse\n+\t\t\tdie_errno(\"cannot exec '%s'\", cmd->argv[0]);\n \t}\n \tif (cmd->pid < 0)\n \t\terror(\"cannot fork() for %s: %s\", cmd->argv[0],\n-- \n1.6.6.115.gd1ab3\n"},{"id":"131229","messageId":"201001101408.45603.j6t@kdbg.org","threadId":"22144","inReplyTo":"201001101404.22258.j6t@kdbg.org","subject":"[PATCH 2/4] run-command: move wait_or_whine earlier","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-10T13:08:45Z","receivedAt":"2010-01-10T13:08:45Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"We want to reuse it from start_command.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n run-command.c |   84 ++++++++++++++++++++++++++++----------------------------\n 1 files changed, 42 insertions(+), 42 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 02c7bfb..dccac37 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -39,6 +39,48 @@ static inline void set_cloexec(int fd)\n }\n #endif\n \n+static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n+{\n+\tint status, code = -1;\n+\tpid_t waiting;\n+\tint failed_errno = 0;\n+\n+\twhile ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n+\t\t;\t/* nothing */\n+\n+\tif (waiting < 0) {\n+\t\tfailed_errno = errno;\n+\t\terror(\"waitpid for %s failed: %s\", argv0, strerror(errno));\n+\t} else if (waiting != pid) {\n+\t\terror(\"waitpid is confused (%s)\", argv0);\n+\t} else if (WIFSIGNALED(status)) {\n+\t\tcode = WTERMSIG(status);\n+\t\terror(\"%s died of signal %d\", argv0, code);\n+\t\t/*\n+\t\t * This return value is chosen so that code & 0xff\n+\t\t * mimics the exit code that a POSIX shell would report for\n+\t\t * a program that died from this signal.\n+\t\t */\n+\t\tcode -= 128;\n+\t} else if (WIFEXITED(status)) {\n+\t\tcode = WEXITSTATUS(status);\n+\t\t/*\n+\t\t * Convert special exit code when execvp failed.\n+\t\t */\n+\t\tif (code == 127) {\n+\t\t\tcode = -1;\n+\t\t\tfailed_errno = ENOENT;\n+\t\t\tif (!silent_exec_failure)\n+\t\t\t\terror(\"cannot run %s: %s\", argv0,\n+\t\t\t\t\tstrerror(ENOENT));\n+\t\t}\n+\t} else {\n+\t\terror(\"waitpid is confused (%s)\", argv0);\n+\t}\n+\terrno = failed_errno;\n+\treturn code;\n+}\n+\n int start_command(struct child_process *cmd)\n {\n \tint need_in, need_out, need_err;\n@@ -272,48 +314,6 @@ fail_pipe:\n \treturn 0;\n }\n \n-static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n-{\n-\tint status, code = -1;\n-\tpid_t waiting;\n-\tint failed_errno = 0;\n-\n-\twhile ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)\n-\t\t;\t/* nothing */\n-\n-\tif (waiting < 0) {\n-\t\tfailed_errno = errno;\n-\t\terror(\"waitpid for %s failed: %s\", argv0, strerror(errno));\n-\t} else if (waiting != pid) {\n-\t\terror(\"waitpid is confused (%s)\", argv0);\n-\t} else if (WIFSIGNALED(status)) {\n-\t\tcode = WTERMSIG(status);\n-\t\terror(\"%s died of signal %d\", argv0, code);\n-\t\t/*\n-\t\t * This return value is chosen so that code & 0xff\n-\t\t * mimics the exit code that a POSIX shell would report for\n-\t\t * a program that died from this signal.\n-\t\t */\n-\t\tcode -= 128;\n-\t} else if (WIFEXITED(status)) {\n-\t\tcode = WEXITSTATUS(status);\n-\t\t/*\n-\t\t * Convert special exit code when execvp failed.\n-\t\t */\n-\t\tif (code == 127) {\n-\t\t\tcode = -1;\n-\t\t\tfailed_errno = ENOENT;\n-\t\t\tif (!silent_exec_failure)\n-\t\t\t\terror(\"cannot run %s: %s\", argv0,\n-\t\t\t\t\tstrerror(ENOENT));\n-\t\t}\n-\t} else {\n-\t\terror(\"waitpid is confused (%s)\", argv0);\n-\t}\n-\terrno = failed_errno;\n-\treturn code;\n-}\n-\n int finish_command(struct child_process *cmd)\n {\n \treturn wait_or_whine(cmd->pid, cmd->argv[0], cmd->silent_exec_failure);\n-- \n1.6.6.115.gd1ab3\n"},{"id":"131230","messageId":"201001101411.22418.j6t@kdbg.org","threadId":"22144","inReplyTo":"201001101404.22258.j6t@kdbg.org","subject":"[PATCH 3/4] start_command: detect execvp failures early","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-10T13:11:22Z","receivedAt":"2010-01-10T13:11:22Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Previously, failures during execvp could be detected only by\nfinish_command. However, in some situations it is beneficial for the\nparent process to know earlier that the child process will not run.\n\nThe idea to use a pipe to signal failures to the parent process and\nthe test case were lifted from patches by Ilari Liusvaara.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n Makefile               |    1 +\n run-command.c          |   47 ++++++++++++++++++++++++++++++++++++++++++++++-\n t/t0061-run-command.sh |   14 ++++++++++++++\n test-run-command.c     |   35 +++++++++++++++++++++++++++++++++++\n 4 files changed, 96 insertions(+), 1 deletions(-)\n create mode 100755 t/t0061-run-command.sh\n create mode 100644 test-run-command.c\n\ndiff --git a/Makefile b/Makefile\nindex 87fc7ff..22c1546 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1785,6 +1785,7 @@ TEST_PROGRAMS += test-genrandom$X\n TEST_PROGRAMS += test-match-trees$X\n TEST_PROGRAMS += test-parse-options$X\n TEST_PROGRAMS += test-path-utils$X\n+TEST_PROGRAMS += test-run-command$X\n TEST_PROGRAMS += test-sha1$X\n TEST_PROGRAMS += test-sigchain$X\n \ndiff --git a/run-command.c b/run-command.c\nindex dccac37..473b7f8 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -17,6 +17,12 @@ static inline void dup_devnull(int to)\n \n #ifndef WIN32\n static int child_err = 2;\n+static int child_notifier = -1;\n+\n+static void notify_parent()\n+{\n+\twrite(child_notifier, \"\", 1);\n+}\n \n static NORETURN void die_child(const char *err, va_list params)\n {\n@@ -142,6 +148,11 @@ fail_pipe:\n \ttrace_argv_printf(cmd->argv, \"trace: run_command:\");\n \n #ifndef WIN32\n+{\n+\tint notify_pipe[2];\n+\tif (pipe(notify_pipe))\n+\t\tnotify_pipe[0] = notify_pipe[1] = -1;\n+\n \tfflush(NULL);\n \tcmd->pid = fork();\n \tif (!cmd->pid) {\n@@ -156,6 +167,11 @@ fail_pipe:\n \t\t}\n \t\tset_die_routine(die_child);\n \n+\t\tclose(notify_pipe[0]);\n+\t\tset_cloexec(notify_pipe[1]);\n+\t\tchild_notifier = notify_pipe[1];\n+\t\tatexit(notify_parent);\n+\n \t\tif (cmd->no_stdin)\n \t\t\tdup_devnull(0);\n \t\telse if (need_in) {\n@@ -196,8 +212,16 @@ fail_pipe:\n \t\t\t\t\tunsetenv(*cmd->env);\n \t\t\t}\n \t\t}\n-\t\tif (cmd->preexec_cb)\n+\t\tif (cmd->preexec_cb) {\n+\t\t\t/*\n+\t\t\t * We cannot predict what the pre-exec callback does.\n+\t\t\t * Forgo parent notification.\n+\t\t\t */\n+\t\t\tclose(child_notifier);\n+\t\t\tchild_notifier = -1;\n+\n \t\t\tcmd->preexec_cb();\n+\t\t}\n \t\tif (cmd->git_cmd) {\n \t\t\texecv_git_cmd(cmd->argv);\n \t\t} else {\n@@ -215,6 +239,27 @@ fail_pipe:\n \tif (cmd->pid < 0)\n \t\terror(\"cannot fork() for %s: %s\", cmd->argv[0],\n \t\t\tstrerror(failed_errno = errno));\n+\n+\t/*\n+\t * Wait for child's execvp. If the execvp succeeds (or if fork()\n+\t * failed), EOF is seen immediately by the parent. Otherwise, the\n+\t * child process sends a single byte.\n+\t * Note that use of this infrastructure is completely advisory,\n+\t * therefore, we keep error checks minimal.\n+\t */\n+\tclose(notify_pipe[1]);\n+\tif (read(notify_pipe[0], &notify_pipe[1], 1) == 1) {\n+\t\t/*\n+\t\t * At this point we know that fork() succeeded, but execvp()\n+\t\t * failed. Errors have been reported to our stderr.\n+\t\t */\n+\t\twait_or_whine(cmd->pid, cmd->argv[0],\n+\t\t\t      cmd->silent_exec_failure);\n+\t\tfailed_errno = errno;\n+\t\tcmd->pid = -1;\n+\t}\n+\tclose(notify_pipe[0]);\n+}\n #else\n {\n \tint s0 = -1, s1 = -1, s2 = -1;\t/* backups of stdin, stdout, stderr */\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nnew file mode 100755\nindex 0000000..10b26e4\n--- /dev/null\n+++ b/t/t0061-run-command.sh\n@@ -0,0 +1,14 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2009 Ilari Liusvaara\n+#\n+\n+test_description='Test run command'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'start_command reports ENOENT' '\n+\ttest-run-command start-command-ENOENT ./does-not-exist\n+'\n+\n+test_done\ndiff --git a/test-run-command.c b/test-run-command.c\nnew file mode 100644\nindex 0000000..a1d5306\n--- /dev/null\n+++ b/test-run-command.c\n@@ -0,0 +1,35 @@\n+/*\n+ * test-run-command.c: test run command API.\n+ *\n+ * (C) 2009 Ilari Liusvaara <ilari.liusvaara@elisanet.fi>\n+ *\n+ * This code is free software; you can redistribute it and/or modify\n+ * it under the terms of the GNU General Public License version 2 as\n+ * published by the Free Software Foundation.\n+ */\n+\n+#include \"git-compat-util.h\"\n+#include \"run-command.h\"\n+#include <string.h>\n+#include <errno.h>\n+\n+int main(int argc, char **argv)\n+{\n+\tstruct child_process proc;\n+\n+\tmemset(&proc, 0, sizeof(proc));\n+\n+\tif(argc < 3)\n+\t\treturn 1;\n+\tproc.argv = (const char **)argv+2;\n+\n+\tif (!strcmp(argv[1], \"start-command-ENOENT\")) {\n+\t\tif (start_command(&proc) < 0 && errno == ENOENT)\n+\t\t\treturn 0;\n+\t\tfprintf(stderr, \"FAIL %s\\n\", argv[1]);\n+\t\treturn 1;\n+\t}\n+\n+\tfprintf(stderr, \"check usage\\n\");\n+\treturn 1;\n+}\n-- \n1.6.6.115.gd1ab3\n"},{"id":"131231","messageId":"201001101418.39321.j6t@kdbg.org","threadId":"22144","inReplyTo":"201001101404.22258.j6t@kdbg.org","subject":"[PATCH 4/4] Improve error message when a transport helper was not found","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-10T13:18:39Z","receivedAt":"2010-01-10T13:18:39Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"From: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>\n\nPerviously, the error message was:\n\n    git: 'remote-foo' is not a git-command. See 'git --help'.\n\nBy not treating the transport helper as a git command, a more suitable\nerror is reported:\n\n    fatal: Unable to find remote helper for 'foo'\n\nSigned-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n While I agree mostly with the implementation of your patch, the justification\n was completely off, and I think it was because you did not notice that the\n parent process died from SIGPIPE. When patches 1-3/4 are applied, the\n parent does not die from SIGPIPE anymore, and the issue is now only minor\n (an unsuitable error message instead of no error message). Therefore,\n I rewrote the commit message to tone it down.\n\n -- Hannes\n\n transport-helper.c |   14 ++++++++++----\n 1 files changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 6ece0d9..d1bc3af 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -102,6 +102,7 @@ static struct child_process *get_helper(struct transport *transport)\n \tint refspec_nr = 0;\n \tint refspec_alloc = 0;\n \tint duped;\n+\tint code;\n \n \tif (data->helper)\n \t\treturn data->helper;\n@@ -111,13 +112,18 @@ static struct child_process *get_helper(struct transport *transport)\n \thelper->out = -1;\n \thelper->err = 0;\n \thelper->argv = xcalloc(4, sizeof(*helper->argv));\n-\tstrbuf_addf(&buf, \"remote-%s\", data->name);\n+\tstrbuf_addf(&buf, \"git-remote-%s\", data->name);\n \thelper->argv[0] = strbuf_detach(&buf, NULL);\n \thelper->argv[1] = transport->remote->name;\n \thelper->argv[2] = remove_ext_force(transport->url);\n-\thelper->git_cmd = 1;\n-\tif (start_command(helper))\n-\t\tdie(\"Unable to run helper: git %s\", helper->argv[0]);\n+\thelper->git_cmd = 0;\n+\thelper->silent_exec_failure = 1;\n+\tcode = start_command(helper);\n+\tif (code < 0 && errno == ENOENT)\n+\t\tdie(\"Unable to find remote helper for '%s'\", data->name);\n+\telse\n+\t\texit(code);\n+\n \tdata->helper = helper;\n \tdata->no_disconnect_req = 0;\n \n-- \n1.6.6.115.gd1ab3\n"},{"id":"131678","messageId":"7viqb49xwb.fsf@alter.siamese.dyndns.org","threadId":"22144","inReplyTo":"201001101411.22418.j6t@kdbg.org","subject":"Re: [PATCH 3/4] start_command: detect execvp failures early","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-14T21:31:16Z","receivedAt":"2010-01-14T21:31:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> Previously, failures during execvp could be detected only by\n> finish_command. However, in some situations it is beneficial for the\n> parent process to know earlier that the child process will not run.\n>\n> The idea to use a pipe to signal failures to the parent process and\n> the test case were lifted from patches by Ilari Liusvaara.\n\nI wonder if we can do this without pipe, perhaps using \"vfork, exec, then\nupdate a variable\"....\n"},{"id":"131681","messageId":"201001142253.19595.j6t@kdbg.org","threadId":"22144","inReplyTo":"7viqb49xwb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/4] start_command: detect execvp failures early","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-14T21:53:19Z","receivedAt":"2010-01-14T21:53:19Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 14. Januar 2010, Junio C Hamano wrote:\n> Johannes Sixt <j6t@kdbg.org> writes:\n> > Previously, failures during execvp could be detected only by\n> > finish_command. However, in some situations it is beneficial for the\n> > parent process to know earlier that the child process will not run.\n> >\n> > The idea to use a pipe to signal failures to the parent process and\n> > the test case were lifted from patches by Ilari Liusvaara.\n>\n> I wonder if we can do this without pipe, perhaps using \"vfork, exec, then\n> update a variable\"....\n\nExcept that some systems implement vfork(2) in terms of fork(2), I heard. \nMoreover, I think that we do way too many things before the exec; isn't that \ndangerous?\n\n-- Hannes\n"},{"id":"131683","messageId":"7vska88g4k.fsf@alter.siamese.dyndns.org","threadId":"22144","inReplyTo":"201001142253.19595.j6t@kdbg.org","subject":"Re: [PATCH 3/4] start_command: detect execvp failures early","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-14T22:40:27Z","receivedAt":"2010-01-14T22:40:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> On Donnerstag, 14. Januar 2010, Junio C Hamano wrote:\n>> Johannes Sixt <j6t@kdbg.org> writes:\n>> > Previously, failures during execvp could be detected only by\n>> > finish_command. However, in some situations it is beneficial for the\n>> > parent process to know earlier that the child process will not run.\n>> >\n>> > The idea to use a pipe to signal failures to the parent process and\n>> > the test case were lifted from patches by Ilari Liusvaara.\n>>\n>> I wonder if we can do this without pipe, perhaps using \"vfork, exec, then\n>> update a variable\"....\n>\n> Except that some systems implement vfork(2) in terms of fork(2), I heard. \n\nAs long as they implement it correctly, we don't care if it is implemented\nin terms of fork or knife or chopstick.  The only thing we care about in\nthis crazy-idea was the shared address space the child can write into\nuntil it exec or _exit while the parent is in a frozen state.\n\n> Moreover, I think that we do way too many things before the exec; isn't that \n> dangerous?\n\nYeah, we do seem to do quite a lot more than I thought.  Scratch that\ncrazy idea, then.\n"}]}