{"thread":{"id":"22061","subject":"[Updated PATCH 0/2] Improve remote helpers exec error reporting","startedAt":"2009-12-30T10:52:16Z","lastAt":"2010-01-01T00:34:06Z","messageCount":13,"participants":["Ilari Liusvaara","Erik Faye-Lund","Tarmigan","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"130535","messageId":"1262170338-11574-1-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"22061","inReplyTo":null,"subject":"[Updated PATCH 0/2] Improve remote helpers exec error reporting","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-30T10:52:16Z","receivedAt":"2009-12-30T10:52:16Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"This reroll fixes the following from previous round:\n- Split loop-trying-to-close to its own inline function.\n- Don't rely on pipe(2) preserving fd array in case of failure.\n- Don't try to use partially received error codes.\n- Don't send error about partial write as it would go to who knows where.\n- Add a testcase (ENOENT is detected correctly).\n\nIlari Liusvaara (2):\n  Report exec errors from run-command\n  Improve transport helper exec failure reporting\n\n Makefile               |    1 +\n run-command.c          |   79 ++++++++++++++++++++++++++++++++++++++++++++++--\n t/t0061-run-command.sh |   13 ++++++++\n test-run-command.c     |   35 +++++++++++++++++++++\n transport-helper.c     |   14 ++++++--\n 5 files changed, 135 insertions(+), 7 deletions(-)\n create mode 100755 t/t0061-run-command.sh\n create mode 100644 test-run-command.c\n"},{"id":"130536","messageId":"1262170338-11574-2-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"22061","inReplyTo":"1262170338-11574-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"[Updated PATCH 1/2] Report exec errors from run-command","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-30T10:52:17Z","receivedAt":"2009-12-30T10:52:17Z","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.\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: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>\n---\n Makefile               |    1 +\n run-command.c          |   79 ++++++++++++++++++++++++++++++++++++++++++++++--\n t/t0061-run-command.sh |   13 ++++++++\n test-run-command.c     |   35 +++++++++++++++++++++\n 4 files changed, 125 insertions(+), 3 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 4a1e5bc..452ad50 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1725,6 +1725,7 @@ TEST_PROGRAMS += test-parse-options$X\n TEST_PROGRAMS += test-path-utils$X\n TEST_PROGRAMS += test-sha1$X\n TEST_PROGRAMS += test-sigchain$X\n+TEST_PROGRAMS += test-run-command$X\n \n all:: $(TEST_PROGRAMS)\n \ndiff --git a/run-command.c b/run-command.c\nindex cf2d8f7..34e5af4 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -15,6 +15,12 @@ static inline void dup_devnull(int to)\n \tclose(fd);\n }\n \n+static inline void force_close(int fd)\n+{\n+\twhile (close(fd) < 0 && errno != EBADF)\n+\t\t; /* No-op */\n+}\n+\n int start_command(struct child_process *cmd)\n {\n \tint need_in, need_out, need_err;\n@@ -76,9 +82,64 @@ 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;\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\terrno = EBADMSG;\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+wait_again:\n+\t\t\tr = waitpid(cmd->pid, &ret, 0);\n+\t\t\tif (r < 0 && errno != ECHILD)\n+\t\t\t\tgoto wait_again;\n+\t\t\tcmd->pid = -1;\n+\t\t}\n+\t} else if (!cmd->pid) {\n+\t\tint r = 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\tflags |= FD_CLOEXEC;\n+\t\tr = (r || fcntl(report_pipe[1], F_SETFD, flags));\n+\t\tif (r) {\n+\t\t\twarning(\"Can't FD_CLOEXEC pipe for exec status \"\n+\t\t\t\t\"report: %s\\n\", strerror(errno));\n+\t\t\tforce_close(report_pipe[1]);\n+\t\t\treport_pipe[1] = -1;\n+\t\t}\n+\n \t\tif (cmd->no_stdin)\n \t\t\tdup_devnull(0);\n \t\telse if (need_in) {\n@@ -126,13 +187,25 @@ 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+\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\texit(127);\n-\t}\n-\tif (cmd->pid < 0)\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 */\ndiff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh\nnew file mode 100755\nindex 0000000..1d9e82a\n--- /dev/null\n+++ b/t/t0061-run-command.sh\n@@ -0,0 +1,13 @@\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\" \\\n+\"test-run-command 1\"\n+\n+test_done\ndiff --git a/test-run-command.c b/test-run-command.c\nnew file mode 100644\nindex 0000000..6297785\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+\tchar* procs[2];\n+\tstruct child_process proc;\n+\tmemset(&proc, 0, sizeof(proc));\n+\n+\tif(argc < 2)\n+\t\treturn 1;\n+\n+\tif (argv[1][1] == '1')\n+\t\tprocs[0] = \"does-not-exist-62896869286\";\n+\tprocs[1] = NULL;\n+\tproc.argv = (const char **)procs;\n+\n+\tif (!run_command(&proc))\n+\t\treturn 1;\n+\tif (errno != ENOENT)\n+\t\treturn 1;\n+\treturn 0;\n+}\n-- \n1.6.6.3.gaa2e1\n"},{"id":"130537","messageId":"1262170338-11574-3-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"22061","inReplyTo":"1262170338-11574-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"[Updated PATCH 2/2] Improve transport helper exec failure reporting","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-30T10:52:18Z","receivedAt":"2009-12-30T10:52:18Z","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 |   14 ++++++++++----\n 1 files changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 5078c71..0965c9b 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -31,13 +31,19 @@ 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] = 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+\tif (start_command(helper)) {\n+\t\tif (errno == ENOENT)\n+\t\t\tdie(\"Unable to find remote helper for \\\"%s\\\"\",\n+\t\t\t\tdata->name);\n+\t\telse\n+\t\t\tdie(\"Unable to run helper %s: %s\", helper->argv[0],\n+\t\t\t\tstrerror(errno));\n+\t}\n \tdata->helper = helper;\n \n \twrite_str_in_full(helper->in, \"capabilities\\n\");\n-- \n1.6.6.3.gaa2e1\n"},{"id":"130554","messageId":"40aa078e0912300547r407efb74xb821345a54a87277@mail.gmail.com","threadId":"22061","inReplyTo":"1262170338-11574-2-git-send-email-ilari.liusvaara@elisanet.fi","subject":"Re: [Updated PATCH 1/2] Report exec errors from run-command","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2009-12-30T13:47:11Z","receivedAt":"2009-12-30T13:47:11Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Dec 30, 2009 at 11:52 AM, Ilari Liusvaara\n<ilari.liusvaara@elisanet.fi> wrote:\n> +static inline void force_close(int fd)\n> +{\n> +       while (close(fd) < 0 && errno != EBADF)\n> +               ; /* No-op */\n> +}\n> +\n\nAccording to http://linux.die.net/man/2/close, close can set errno to\nEBADF, EINTR, or EIO. Currently, you're retrying on EINTR and EIO.\nWhen we get EIO, are you sure it makes sense to retry? I'd imagine\nthat error would most likely just repeat itself, leading to an\ninfinite loop. How about \"while (close(fd) < 0 && errno == EINTR)\"\ninstead? I've seen other functions (like xread in wrapper.c) only\nretry on those errors that it expects. In xreads case, it's not\nretrying on EIO.\n\nPerhaps it's OK still, since force_close() is only used on pipes. I\ndon't know if closing a pipe can generate EIO or not.\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"130589","messageId":"905315640912302126n1848c99cre0f9caa644041fad@mail.gmail.com","threadId":"22061","inReplyTo":"1262170338-11574-2-git-send-email-ilari.liusvaara@elisanet.fi","subject":"Re: [Updated PATCH 1/2] Report exec errors from run-command","fromName":"Tarmigan","fromEmail":"tarmigan+git@gmail.com","sentAt":"2009-12-31T05:26:48Z","receivedAt":"2009-12-31T05:26:48Z","isPatch":true,"sender":{"key":"tarmigan+git@gmail.com","avatar":null},"body":"On Wed, Dec 30, 2009 at 5:52 AM, Ilari Liusvaara\n<ilari.liusvaara@elisanet.fi> wrote:\n> Previously run-command was unable to report errors happening in exec\n> call. Change it to pass errno from failed exec to errno value at\n> return.\n>\n> The errno value passing can be done by opening close-on-exec pipe and\n> piping the error code through in case of failure. In case of success,\n> close-on-exec closes the pipe on successful exec and parent process\n> gets end of file on read.\n\nI was testing pu and 'git diff' and 'git log' would hang forever.\n\nBisecting pointed to v1 of this patch.  But seeing that v2 was out, I\ntried v2 of the patch, but the issue remains.\n\nTried on OSX and linux with the same results.\n\nHere's a gdb backtrace on OSX at the point where I interrupted it:\n(gdb) bt\n#0  0x9923dbfe in read$UNIX2003 ()\n#1  0x000b399b in start_command (cmd=0x10b300) at run-command.c:110\n#2  0x00099452 in setup_pager () at pager.c:94\n#3  0x0002196b in cmd_diff (argc=1, argv=0xbffff42c, prefix=0x0) at\nbuiltin-diff.c:316\n#4  0x00002a2a in run_builtin [inlined] () at git.c:257\n#5  0x00002a2a in handle_internal_command (argc=1, argv=0xbffff42c) at git.c:401\n#6  0x00002cab in main (argc=1, argv=0xbffff42c) at git.c:443\n\nand on linux:\n(gdb) bt\n#0  0x0000003530a0d590 in __read_nocancel () from /lib64/libpthread.so.0\n#1  0x0000000000494858 in start_command (cmd=0x71ed60) at run-command.c:93\n#2  0x000000000047daf8 in setup_pager () at pager.c:94\n#3  0x000000000041d83f in cmd_diff (argc=1, argv=0x7fff256f9fe0, prefix=0x0)\n    at builtin-diff.c:316\n#4  0x0000000000403ced in handle_internal_command (argc=1, argv=0x7fff256f9fe0)\n    at git.c:257\n#5  0x0000000000403f26 in main (argc=1, argv=0x7fff256f9fe0) at git.c:445\n\nThanks,\nTarmigan\n"},{"id":"130601","messageId":"20091231104835.GA18848@Knoppix","threadId":"22061","inReplyTo":"905315640912302126n1848c99cre0f9caa644041fad@mail.gmail.com","subject":"Re: [Updated PATCH 1/2] Report exec errors from run-command","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-31T10:48:36Z","receivedAt":"2009-12-31T10:48:36Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Thu, Dec 31, 2009 at 12:26:48AM -0500, Tarmigan wrote:\n> On Wed, Dec 30, 2009 at 5:52 AM, Ilari Liusvaara\n> <ilari.liusvaara@elisanet.fi> wrote:\n> \n> I was testing pu and 'git diff' and 'git log' would hang forever.\n \nV3 just sent to list. Should fix this issue.\n\n-Ilari\n"},{"id":"130610","messageId":"905315640912310644r7e94699eo876361fb6139f0e7@mail.gmail.com","threadId":"22061","inReplyTo":"20091231104835.GA18848@Knoppix","subject":"Re: [Updated PATCH 1/2] Report exec errors from run-command","fromName":"Tarmigan","fromEmail":"tarmigan+git@gmail.com","sentAt":"2009-12-31T14:44:33Z","receivedAt":"2009-12-31T14:44:33Z","isPatch":true,"sender":{"key":"tarmigan+git@gmail.com","avatar":null},"body":"On Thu, Dec 31, 2009 at 5:48 AM, Ilari Liusvaara\n<ilari.liusvaara@elisanet.fi> wrote:\n> On Thu, Dec 31, 2009 at 12:26:48AM -0500, Tarmigan wrote:\n>> On Wed, Dec 30, 2009 at 5:52 AM, Ilari Liusvaara\n>> <ilari.liusvaara@elisanet.fi> wrote:\n>>\n>> I was testing pu and 'git diff' and 'git log' would hang forever.\n>\n> V3 just sent to list. Should fix this issue.\n\nYep, that fixed it.\n\nThanks,\nTarmigan\n"},{"id":"130612","messageId":"4B3CC6E5.7090404@kdbg.org","threadId":"22061","inReplyTo":"1262170338-11574-3-git-send-email-ilari.liusvaara@elisanet.fi","subject":"Re: [Updated PATCH 2/2] Improve transport helper exec failure reporting","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-12-31T15:44:37Z","receivedAt":"2009-12-31T15:44:37Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Ilari Liusvaara schrieb:\n> @@ -31,13 +31,19 @@ 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] = 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> +\tif (start_command(helper)) {\n> +\t\tif (errno == ENOENT)\n> +\t\t\tdie(\"Unable to find remote helper for \\\"%s\\\"\",\n> +\t\t\t\tdata->name);\n\nYou should set helper->silent_exec_failure = 1 when you give your own \nerror message for the ENOENT case.\n\nBTW, which error message do you see without your change in this case? You \nonly say \"pretty much useless\", but do not give an example.\n\n> +\t\telse\n> +\t\t\tdie(\"Unable to run helper %s: %s\", helper->argv[0],\n> +\t\t\t\tstrerror(errno));\n\nYou shouldn't write an error message here because start_command has \nalready reported the error.\n\n-- Hannes\n"},{"id":"130615","messageId":"20091231165904.GA24243@Knoppix","threadId":"22061","inReplyTo":"4B3CC6E5.7090404@kdbg.org","subject":"Re: [Updated PATCH 2/2] Improve transport helper exec failure reporting","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-31T16:59:04Z","receivedAt":"2009-12-31T16:59:04Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Thu, Dec 31, 2009 at 04:44:37PM +0100, Johannes Sixt wrote:\n> Ilari Liusvaara schrieb:\n> >@@ -31,13 +31,19 @@ static struct child_process *get_helper(struct transport *transport)\n> \n> You should set helper->silent_exec_failure = 1 when you give your\n> own error message for the ENOENT case.\n\nAh yeah, might matter for Win32.\n\n> BTW, which error message do you see without your change in this\n> case? You only say \"pretty much useless\", but do not give an\n> example.\n\ngit: 'remote-foo' is not a git-command. See 'git --help'.\n\nAnd as first line of output, such thing is utterly confusing.\n\n> >+\t\telse\n> >+\t\t\tdie(\"Unable to run helper %s: %s\", helper->argv[0],\n> >+\t\t\t\tstrerror(errno));\n> \n> You shouldn't write an error message here because start_command has\n> already reported the error.\n\nIts not printed on Unix.\n\n-Ilari\n"},{"id":"130616","messageId":"4B3CE3D2.5010502@kdbg.org","threadId":"22061","inReplyTo":"20091231165904.GA24243@Knoppix","subject":"Re: [Updated PATCH 2/2] Improve transport helper exec failure reporting","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-12-31T17:48:02Z","receivedAt":"2009-12-31T17:48:02Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Ilari Liusvaara schrieb:\n> On Thu, Dec 31, 2009 at 04:44:37PM +0100, Johannes Sixt wrote:\n>> Ilari Liusvaara schrieb:\n>>> @@ -31,13 +31,19 @@ static struct child_process *get_helper(struct transport *transport)\n>> You should set helper->silent_exec_failure = 1 when you give your\n>> own error message for the ENOENT case.\n> \n> Ah yeah, might matter for Win32.\n\nActually, no. I forgot to mention that your modified start_command should \ntreat ENOENT differently by looking at cmd->silent_exec_failure. But see \nbelow.\n\n>> BTW, which error message do you see without your change in this\n>> case? You only say \"pretty much useless\", but do not give an\n>> example.\n> \n> git: 'remote-foo' is not a git-command. See 'git --help'.\n> \n> And as first line of output, such thing is utterly confusing.\n\nAnd you change this by treating the helper command not as a git command, \nbut as a normal command that happens to start with 'git-'. Whether this \ninterpretation is suitable for the transport layer, I do not want to \ndecide and I will certainly not object. :-)\n\nAn alternative solution would be to forward the silent_exec_failure flag \nto exec_git_cmd() to unify the treatment of the error condition with the \nnon-git-command error path.\n\n>>> +\t\telse\n>>> +\t\t\tdie(\"Unable to run helper %s: %s\", helper->argv[0],\n>>> +\t\t\t\tstrerror(errno));\n>> You shouldn't write an error message here because start_command has\n>> already reported the error.\n> \n> Its not printed on Unix.\n\nI see.\n\nDocumentation/technical/api-run-command.txt documents the error behavior. \nThere are three error cases:\n\n1. system call failures\n2. exec failure due to ENOENT\n3. non-zero exit of the child and death through signal\n\nYour patch makes all exec failures into category 1, but IMO, these are \nactually category 3 (except for the ENOENT case).\n\nIn case 3, it is expected that the child process prints a suitable error \nmessage. Therefore, you should start with merely replacing the unconditional\n\n\texit(127);\nby\n\tif (errno == ENOENT)\n\t\texit(127);\n\telse\n\t\tdie_errno(\"Cannot exec %s\", cmd->argv[0]);\n\nAnd then you can think about how you support the ENOENT case better. My \nproposal for this was to do the PATH lookup manually before the fork(), \nand then the above conditional would melt down to simply:\n\n\tdie_errno(\"Cannot exec %s\", cmd->argv[0]);\n\n-- Hannes\n"},{"id":"130617","messageId":"20091231182436.GA1326@Knoppix","threadId":"22061","inReplyTo":"4B3CE3D2.5010502@kdbg.org","subject":"Re: [Updated PATCH 2/2] Improve transport helper exec failure reporting","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-31T18:24:36Z","receivedAt":"2009-12-31T18:24:36Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Thu, Dec 31, 2009 at 06:48:02PM +0100, Johannes Sixt wrote:\n> Ilari Liusvaara schrieb:\n> >On Thu, Dec 31, 2009 at 04:44:37PM +0100, Johannes Sixt wrote:\n> >>Ilari Liusvaara schrieb:\n \n> And you change this by treating the helper command not as a git\n> command, but as a normal command that happens to start with 'git-'.\n> Whether this interpretation is suitable for the transport layer, I\n> do not want to decide and I will certainly not object. :-)\n\nThe transport helpers are special: they shouldn't be built-in.\n\n> An alternative solution would be to forward the silent_exec_failure\n> flag to exec_git_cmd() to unify the treatment of the error condition\n> with the non-git-command error path.\n\nWon't work. The error in git command case would be noted in another memory\nimage. And passing that back would be nasty to say the least.\n \n> In case 3, it is expected that the child process prints a suitable\n> error message. Therefore, you should start with merely replacing the\n> unconditional\n> \n> \texit(127);\n> by\n> \tif (errno == ENOENT)\n> \t\texit(127);\n> \telse\n> \t\tdie_errno(\"Cannot exec %s\", cmd->argv[0]);\n> \n> And then you can think about how you support the ENOENT case better.\n> My proposal for this was to do the PATH lookup manually before the\n> fork(), and then the above conditional would melt down to simply:\n> \n> \tdie_errno(\"Cannot exec %s\", cmd->argv[0]);\n> \n\nThe child process can't sanely print anything. Stderr would go to\nwho knows where. Parent process should have much better idea what to\ndo with errors.\n\n\n-Ilari\n"},{"id":"130621","messageId":"4B3CF118.7080404@kdbg.org","threadId":"22061","inReplyTo":"20091231182436.GA1326@Knoppix","subject":"Re: [Updated PATCH 2/2] Improve transport helper exec failure reporting","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-12-31T18:44:40Z","receivedAt":"2009-12-31T18:44:40Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Ilari Liusvaara schrieb:\n> On Thu, Dec 31, 2009 at 06:48:02PM +0100, Johannes Sixt wrote:\n>> In case 3, it is expected that the child process prints a suitable\n>> error message. Therefore, you should start with merely replacing the\n>> unconditional\n>>\n>> \texit(127);\n>> by\n>> \tif (errno == ENOENT)\n>> \t\texit(127);\n>> \telse\n>> \t\tdie_errno(\"Cannot exec %s\", cmd->argv[0]);\n>>\n>> And then you can think about how you support the ENOENT case better.\n>> My proposal for this was to do the PATH lookup manually before the\n>> fork(), and then the above conditional would melt down to simply:\n>>\n>> \tdie_errno(\"Cannot exec %s\", cmd->argv[0]);\n>>\n> \n> The child process can't sanely print anything. Stderr would go to\n> who knows where.\n\nWrong - because:\n\n> Parent process should have much better idea what to\n> do with errors.\n\nVery correct. For this reason, the parent process assigns a stderr channel \nto the child (or does not do so to inherit its own stderr), and the child \nis expected to use it. Errors due to execvp failures are no exception, IMO \n(except ENOENT, as always).\n\n-- Hannes\n"},{"id":"130650","messageId":"201001010134.06740.j6t@kdbg.org","threadId":"22061","inReplyTo":"4B3CF118.7080404@kdbg.org","subject":"Re: [Updated PATCH 2/2] Improve transport helper exec failure reporting","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-01T00:34:06Z","receivedAt":"2010-01-01T00:34:06Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 31. Dezember 2009, Johannes Sixt wrote:\n> Ilari Liusvaara schrieb:\n> > The child process can't sanely print anything. Stderr would go to\n> > who knows where.\n>\n> Wrong - because:\n> > Parent process should have much better idea what to\n> > do with errors.\n>\n> Very correct. For this reason, the parent process assigns a stderr channel\n> to the child (or does not do so to inherit its own stderr), and the child\n> is expected to use it. Errors due to execvp failures are no exception, IMO\n> (except ENOENT, as always).\n\nActually, I changed my mind: execvp failures should go to the parent's stderr,\njust as all errors before the exec happens.\n\nHow about this patch for a starter? Take it with a grain of salt - I coded it\nafter the New Year celebration ;) I was unable to find a case quickly that\nexercises die_child().\n\n--- 8< ---\nFrom: Johannes Sixt <j6t@kdbg.org>\nDate: Fri, 1 Jan 2010 01:22:05 +0100\nSubject: [PATCH] start_command: report child process setup errors to the parent's stderr\n\nWhen 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 such that stderr goes\nto the client. In these cases, hook setup errors could be regarded as\ninformation leak.\n\nThis patch makes a copy of stderr if necessary and uses a special\ndie routine that is used for all die() calls so that the errors are sent to\nthe parent's channel.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n run-command.c |   39 ++++++++++++++++++++++++++++++++++++---\n 1 files changed, 36 insertions(+), 3 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex cf2d8f7..2480a8c 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -15,6 +15,23 @@ static inline void dup_devnull(int to)\n \tclose(fd);\n }\n \n+#ifndef WIN32\n+static int child_err;\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+#endif\n+\n int start_command(struct child_process *cmd)\n {\n \tint need_in, need_out, need_err;\n@@ -79,6 +96,21 @@ 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\tint flags;\n+\t\t\tchild_err = dup(2);\n+\t\t\tflags = fcntl(child_err, F_GETFL);\n+\t\t\tif (flags < 0)\n+\t\t\t\tflags = 0;\n+\t\t\tfcntl(child_err, F_SETFL, flags | FD_CLOEXEC);\n+\t\t\tset_die_routine(die_child);\n+\t\t}\n+\n \t\tif (cmd->no_stdin)\n \t\t\tdup_devnull(0);\n \t\telse if (need_in) {\n@@ -126,9 +158,10 @@ 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\tif (errno == ENOENT)\n+\t\t\texit(127);\n+\t\telse\n+\t\t\tdie_errno(\"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"}]}