{"thread":{"id":"22069","subject":"[updated patch v2 0/2] Improve remote helper exec failure reporting","startedAt":"2009-12-31T10:48:06Z","lastAt":"2009-12-31T16:22:02Z","messageCount":4,"participants":["Ilari Liusvaara","Johannes Sixt"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"130598","messageId":"1262256488-22985-1-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"22069","inReplyTo":null,"subject":"[updated patch v2 0/2] Improve remote helper exec failure reporting","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-31T10:48:06Z","receivedAt":"2009-12-31T10:48:06Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"Changes since updated patch (second round):\n- Work around deadlock with pager.\n- Fix exec reporting if read end of report pipe winds up to be fd #0.\n- Limit number of retries in force_close for unexpected errors.\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          |  103 ++++++++++++++++++++++++++++++++++++++++++++++--\n t/t0061-run-command.sh |   13 ++++++\n test-run-command.c     |   35 ++++++++++++++++\n transport-helper.c     |   14 +++++--\n 5 files changed, 158 insertions(+), 8 deletions(-)\n create mode 100755 t/t0061-run-command.sh\n create mode 100644 test-run-command.c\n"},{"id":"130600","messageId":"1262256488-22985-2-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"22069","inReplyTo":"1262256488-22985-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"[updated patch v2 1/2] Report exec errors from run-command","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-31T10:48:07Z","receivedAt":"2009-12-31T10:48:07Z","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          |  103 ++++++++++++++++++++++++++++++++++++++++++++++--\n t/t0061-run-command.sh |   13 ++++++\n test-run-command.c     |   35 ++++++++++++++++\n 4 files changed, 148 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..1c5b103 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -15,6 +15,22 @@ static inline void dup_devnull(int to)\n \tclose(fd);\n }\n \n+static inline void force_close(int fd)\n+{\n+\tint err = 0;\n+\t/*\n+\t * Retry EINTRs undefinitely, exit on EBADF immediately, other\n+\t * errors retry only up to three times (even if pipe close\n+\t * shouldn't cause other errors, but you never know with\n+\t * what broken systems may return on closed file descriptor).\n+\t * consequences of failure to close pipe here may include\n+\t * deadlocking.\n+\t */\n+\twhile (close(fd) < 0 && errno != EBADF && err < 3)\n+\t\tif(errno != EINTR)\n+\t\t\terr++;\n+}\n+\n int start_command(struct child_process *cmd)\n {\n \tint need_in, need_out, need_err;\n@@ -76,9 +92,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@@ -119,20 +190,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..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":"130599","messageId":"1262256488-22985-3-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"22069","inReplyTo":"1262256488-22985-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"[updated patch v2 2/2] Improve transport helper exec failure reporting","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-31T10:48:08Z","receivedAt":"2009-12-31T10:48:08Z","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":"130613","messageId":"4B3CCFAA.7060702@kdbg.org","threadId":"22069","inReplyTo":"1262256488-22985-2-git-send-email-ilari.liusvaara@elisanet.fi","subject":"Re: [updated patch v2 1/2] Report exec errors from run-command","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-12-31T16:22:02Z","receivedAt":"2009-12-31T16:22:02Z","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> +\tint err = 0;\n> +\t/*\n> +\t * Retry EINTRs undefinitely, exit on EBADF immediately, other\n> +\t * errors retry only up to three times (even if pipe close\n> +\t * shouldn't cause other errors, but you never know with\n> +\t * what broken systems may return on closed file descriptor).\n> +\t * consequences of failure to close pipe here may include\n> +\t * deadlocking.\n> +\t */\n> +\twhile (close(fd) < 0 && errno != EBADF && err < 3)\n> +\t\tif(errno != EINTR)\n> +\t\t\terr++;\n\nWhat's the point to iterate on all errors except EBADF? If the close() \nfails once, it will fail again.\n\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\nYou really should iterate only on well-known errors. What's wrong with\n\n\t\twhile (waitpid(pid, &status, 0) < 0 && errno == EINTR)\n\t\t\t;\t/* nothing */\n\nsimilar to wait_or_whine()'s call to waitpid() and to avoid goto.\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\nThis test is not specific enough: It would pass even without your change \nto start_command(), because finish_command() detects the ENOENT case. You \nreally want to test that you see ENOENT after start_command() (i.e., \nbefore finish_command()).\n\n-- Hannes\n"}]}