{"thread":{"id":"22072","subject":"[updated patch v3 0/2] Imporve remote helpers exec failure reporting","startedAt":"2009-12-31T18:26:41Z","lastAt":"2010-01-05T05:11:35Z","messageCount":6,"participants":["Ilari Liusvaara","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"130618","messageId":"1262284003-1417-1-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"22072","inReplyTo":null,"subject":"[updated patch v3 0/2] Imporve remote helpers exec failure reporting","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-31T18:26:41Z","receivedAt":"2009-12-31T18:26:41Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"Changes from previous rounds are:\n\n- Don't loop on unknown errors from close.\n- Don't loop on unknown errors from waitpid.\n- Set silent_exec_failure on remote helper exec.\n- Fix the test to actually test what it is supposed to.\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          |   97 ++++++++++++++++++++++++++++++++++++++++++++++--\n t/t0061-run-command.sh |   13 ++++++\n test-run-command.c     |   35 +++++++++++++++++\n transport-helper.c     |   14 +++++--\n 5 files changed, 152 insertions(+), 8 deletions(-)\n create mode 100755 t/t0061-run-command.sh\n create mode 100644 test-run-command.c\n"},{"id":"130619","messageId":"1262284003-1417-2-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"22072","inReplyTo":"1262284003-1417-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"[updated patch v3 1/2] Report exec errors from run-command","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-31T18:26:42Z","receivedAt":"2009-12-31T18:26:42Z","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          |   97 ++++++++++++++++++++++++++++++++++++++++++++++--\n t/t0061-run-command.sh |   13 ++++++\n test-run-command.c     |   35 +++++++++++++++++\n 4 files changed, 142 insertions(+), 4 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..1086c6d 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -15,6 +15,18 @@ 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 * The close is deemed success or failed in non-transient way if\n+\t * close() suceeds, returns EBADF or error other than EINTR or\n+\t * EAGAIN.\n+\t */\n+\twhile (close(fd) < 0 && errno != EBADF)\n+\t\tif(errno != EINTR && errno != EAGAIN)\n+\t\t\tbreak;\n+}\n+\n int start_command(struct child_process *cmd)\n {\n \tint need_in, need_out, need_err;\n@@ -76,9 +88,62 @@ 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+\t\t\tif(waitpid(cmd->pid, &ret, 0) < 0 && errno == EINTR)\n+\t\t\t\t/* Nothing. */ ;\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@@ -119,20 +184,44 @@ 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 */\n+\t\t\tforce_close(report_pipe[1]);\n+\t\t\treport_pipe[1] = -1;\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 \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..4716033\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][0] == '1')\n+\t\tprocs[0] = \"does-not-exist-62896869286\";\n+\tprocs[1] = NULL;\n+\tproc.argv = (const char **)procs;\n+\n+\tif (!start_command(&proc))\n+\t\treturn 1;\n+\tif (argv[1][0] == '1' && errno == ENOENT)\n+\t\treturn 0;\n+\treturn 1;\n+}\n-- \n1.6.6.3.gaa2e1\n"},{"id":"130620","messageId":"1262284003-1417-3-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"22072","inReplyTo":"1262284003-1417-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"[updated patch v3 2/2] Improve transport helper exec failure reporting","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-31T18:26:43Z","receivedAt":"2009-12-31T18:26:43Z","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..73da339 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+\thelper->silent_exec_failure = 1;\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_errno(\"Unable to run helper %s\", helper->argv[0]);\n+\t}\n \tdata->helper = helper;\n \n \twrite_str_in_full(helper->in, \"capabilities\\n\");\n-- \n1.6.6.3.gaa2e1\n"},{"id":"130622","messageId":"4B3CF568.1050705@kdbg.org","threadId":"22072","inReplyTo":"1262284003-1417-2-git-send-email-ilari.liusvaara@elisanet.fi","subject":"Re: [updated patch v3 1/2] Report exec errors from run-command","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-12-31T19:03:04Z","receivedAt":"2009-12-31T19:03:04Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Ilari Liusvaara schrieb:\n> +static inline void force_close(int fd)\n> +{\n> +\t/*\n> +\t * The close is deemed success or failed in non-transient way if\n> +\t * close() suceeds, returns EBADF or error other than EINTR or\n> +\t * EAGAIN.\n> +\t */\n> +\twhile (close(fd) < 0 && errno != EBADF)\n> +\t\tif(errno != EINTR && errno != EAGAIN)\n> +\t\t\tbreak;\n\nYou are constantly ignoring proposals to iterate only on EINTR and EAGAIN, \nbut do not make an argument why you do otherwise. Did I miss something?\n\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> +\t\t\tif(waitpid(cmd->pid, &ret, 0) < 0 && errno == EINTR)\n> +\t\t\t\t/* Nothing. */ ;\n> +\t\t\tcmd->pid = -1;\n\nAs per Documentation/technical/api-run-command.txt, you should write an \nerror here, except if (failed_errno==ENOENT && cmd->silent_exec_failure!=0).\n\n> +test_expect_success \"reporting ENOENT\" \\\n> +\"test-run-command 1\"\n\nI wonder what this parameter \"1\" is good for...\n\n-- Hannes\n"},{"id":"130623","messageId":"4B3CF60A.4050100@kdbg.org","threadId":"22072","inReplyTo":"1262284003-1417-3-git-send-email-ilari.liusvaara@elisanet.fi","subject":"Re: [updated patch v3 2/2] Improve transport helper exec failure reporting","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-12-31T19:05:46Z","receivedAt":"2009-12-31T19:05:46Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Ilari Liusvaara schrieb:\n> +\thelper->silent_exec_failure = 1;\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_errno(\"Unable to run helper %s\", helper->argv[0]);\n\nWhen you fix your implementation of start_command() to follow \nDocumentation/technical/api-run-command.txt, the error message in the else \nbranch is not needed anymore (and then you can ask yourself whether you \nreally want to issue your own error message in the case of ENOENT).\n\n-- Hannes\n"},{"id":"130813","messageId":"7vaawt87aw.fsf@alter.siamese.dyndns.org","threadId":"22072","inReplyTo":"4B3CF568.1050705@kdbg.org","subject":"Re: [updated patch v3 1/2] Report exec errors from run-command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-05T05:11:35Z","receivedAt":"2010-01-05T05:11:35Z","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> Ilari Liusvaara schrieb:\n>> +static inline void force_close(int fd)\n>> +{\n>> +\t/*\n>> +\t * The close is deemed success or failed in non-transient way if\n>> +\t * close() suceeds, returns EBADF or error other than EINTR or\n>> +\t * EAGAIN.\n>> +\t */\n>> +\twhile (close(fd) < 0 && errno != EBADF)\n>> +\t\tif(errno != EINTR && errno != EAGAIN)\n>> +\t\t\tbreak;\n>\n> You are constantly ignoring proposals to iterate only on EINTR and\n> EAGAIN, but do not make an argument why you do otherwise. Did I miss\n> something?\n\nMeaning something like this?\n\n\tstatic inline void force_close(int fd)\n\t{\n\t\t/*\n\t\t * Retry failed close() on EINTR or EAGAIN\n\t\t */\n\t\twhile (close(fd) < 0 && (errno == EINTR || errno == EAGAIN))\n\t\t\t; /* try again */\n\t}\n\nwhich should be equivalent as long as EBADF is different from EINTR and\nEAGAIN, I think.\n\n>> +\tif (cmd->pid > 0) {\n>> +\t\tint r = 0, ret;\n>> +\t\tforce_close(report_pipe[1]);\n>> +read_again:\n>> ...\n>> +\t\t\tif(waitpid(cmd->pid, &ret, 0) < 0 && errno == EINTR)\n>> +\t\t\t\t/* Nothing. */ ;\n>> +\t\t\tcmd->pid = -1;\n>\n> As per Documentation/technical/api-run-command.txt, you should write\n> an error here, except if (failed_errno==ENOENT &&\n> cmd->silent_exec_failure!=0).\n\nI was planning to replace the earlier series that was dropped from pu with\nthis iteration, but I guess I'll wait for another round before doing so.\n\nThanks for reviewing.\n\n>> +test_expect_success \"reporting ENOENT\" \\\n>> +\"test-run-command 1\"\n>\n> I wonder what this parameter \"1\" is good for...\n\nI guessed that this is for adding more tests to test-run-command in the\nfuture and not having to change this test, in which case I think it is a\nsensible thing to do.\n"}]}