{"thread":{"id":"22023","subject":"[RFC PATCH 2/2] Improve transport helper exec failure reporting","startedAt":"2009-12-24T17:49:29Z","lastAt":"2009-12-25T17:15:05Z","messageCount":11,"participants":["Ilari Liusvaara","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"130315","messageId":"1261676971-3285-1-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"22023","inReplyTo":null,"subject":"[RFC PATCH 0/2] Report remote helper exec failures","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-24T17:49:29Z","receivedAt":"2009-12-24T17:49:29Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"Actually give useful error messages if executing git remote helper fails\nfor some reason.\n\nThe previous error message was:\n\ngit: 'remote-fail' is not a git-command. See 'git --help'\n\nThis changes the error message to:\n\n'Unable to find remote helper for \"nonexistent\"'\n\nor\n\n'Unable to run helper HelperThatGetsEACCESS: Permission denied' \n(or whatever the errno is).\n\n\nPatch series is based on v1.6.6.\n\nOne of the changes is adjacent to changes in external helper dispatch\nsupport change and gets merge conflict. Here's how I resolved that\nconflict for testing:\n\n helper->argv[2] = remove_ext_force(transport->url);\n helper->git_cmd = 0;\n if (start_command(helper)) {\n if (errno == ENOENT)\n die(\"Unable to find remote helper for \\\"%s\\\"\",\n data->name);\n else\n die(\"Unable to run helper %s: %s\", helper->argv[0],\n strerror(errno));\n }\n\nThe first line in that comes from dispatch support and the rest come from\nexec failure reporting.\n\n\nIlari Liusvaara (2):\n Report exec errors from run-command\n Improve transport helper exec failure reporting\n\n run-command.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++--\n transport-helper.c | 14 +++++++---\n 2 files changed, 79 insertions(+), 7 deletions(-)\n"},{"id":"130316","messageId":"1261676971-3285-2-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"22023","inReplyTo":"1261676971-3285-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"[RFC PATCH 1/2] Report exec errors from run-command","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-24T17:49:30Z","receivedAt":"2009-12-24T17:49:30Z","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 \n---\n run-command.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 files changed, 69 insertions(+), 3 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex cf2d8f7..d334d0f 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -76,9 +76,60 @@ 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\twarning(\"Can't open pipe for exec status report: %s\\n\",\n+\t\t\tstrerror(errno));\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\twhile(close(report_pipe[1]) < 0 && errno != EBADF);\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\telse {\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\twhile(close(report_pipe[0]) < 0 && errno != EBADF);\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\twhile(close(report_pipe[1]) < 0 && errno != EBADF);\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 +177,28 @@ 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+\t\telse if (r < 0)\n+\t\t\twarning(\"Can't write exec status report: %s\\n\",\n+\t\t\t\tstrerror(errno));\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 */\n-- \n1.6.6.3.gaa2e1\n"},{"id":"130314","messageId":"1261676971-3285-3-git-send-email-ilari.liusvaara@elisanet.fi","threadId":"22023","inReplyTo":"1261676971-3285-1-git-send-email-ilari.liusvaara@elisanet.fi","subject":"[RFC PATCH 2/2] Improve transport helper exec failure reporting","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-24T17:49:31Z","receivedAt":"2009-12-24T17:49:31Z","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 \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":"130323","messageId":"7vr5qjecbb.fsf@alter.siamese.dyndns.org","threadId":"22023","inReplyTo":"1261676971-3285-2-git-send-email-ilari.liusvaara@elisanet.fi","subject":"Re: [RFC PATCH 1/2] Report exec errors from run-command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-25T07:35:04Z","receivedAt":"2009-12-25T07:35:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ilari Liusvaara writes:\n\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\nThanks; I think overall this is a good idea, even though I have no clue\nif WIN32 side wants a similar support.\n\n - At first reading, the \"while (close(fd) < 0 && errno != EBADF);\"\n pattern was a bit of eyesore. It might be worth factoring that out to\n a small static helper function that a smart compiler would\n automatically inline (or mark it as a static inline).\n\n - Is it guaranteed that a failed pipe(2) never touches its int fildes[2]\n parameter, or the values are undefined when it fails? The approach\n would save one extra variable, compared to an alternative approach to\n keep an explicit variable to record a pipe failure, but It feels iffy\n that the code relies on them being left as their initial -1 values.\n\n - Should we worry about partial write as well (you seem to warn when you\n get a partial read)? Is it worth using xread() and friends found in\n wrapper.c instead of goto read/write_again?\n\n - Shouldn't any of these warning() be die() instead?\n\n - The two extra file descriptors this patch uses are allocated after all\n the existing pipe flipping is done, and all the dup's done in the child\n process are to use dup2() to set up the known file descriptors at low\n numbers, so I don't think we have to worry about this patch changing\n the behaviour of the process pair by changing the assignment of file\n descriptors (we had a bug or two in the past that made subprocess\n misbehave under some funky condition, e.g. run with fd#0 closed).\n"},{"id":"130324","messageId":"7vljgrebv2.fsf@alter.siamese.dyndns.org","threadId":"22023","inReplyTo":"1261676971-3285-3-git-send-email-ilari.liusvaara@elisanet.fi","subject":"Re: [RFC PATCH 2/2] Improve transport helper exec failure reporting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-25T07:44:49Z","receivedAt":"2009-12-25T07:44:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ilari Liusvaara writes:\n\n> Previously transport-helper exec failure error reporting was pretty\n> much useless as it didn't report errors from execve, only from pipe\n> and fork. Now that run-command passes errno from exec, use the\n> improved support to actually print useful errors if execution fails.\n>\n> Signed-off-by: Ilari Liusvaara \n\nLooks pretty straightforward, except that I have this nagging feeling that\nwe should *not* be married to the idea of \"'proc->git_cmd = 1' is merely a\nway to save you from typing 'git-' prefix in start_command(proc)\".\n\n> diff --git a/transport-helper.c b/transport-helper.c\n> index 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\nFor example, we might later want to use different $PATH only when running\na git subcommand, and telling run_command() explicitly that we are running\na git thing would help if you don't add \"git-\" to the command line and\ndrop \"proc->git_cmd = 1\" in the caller like your patch does.\n\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> -- \n> 1.6.6.3.gaa2e1\n"},{"id":"130325","messageId":"7vhbrfebrh.fsf@alter.siamese.dyndns.org","threadId":"22023","inReplyTo":"7vr5qjecbb.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC PATCH 1/2] Report exec errors from run-command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-25T07:46:58Z","receivedAt":"2009-12-25T07:46:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Ilari Liusvaara writes:\n>\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>\n> Thanks; I think overall this is a good idea, even though I have no clue\n> if WIN32 side wants a similar support.\n\nBy the way, at least it should be pretty straightforward to add a test or\ntwo for [1/2] if not [2/2].\n"},{"id":"130326","messageId":"7vd423e9as.fsf@alter.siamese.dyndns.org","threadId":"22023","inReplyTo":"7vr5qjecbb.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC PATCH 1/2] Report exec errors from run-command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-25T08:40:11Z","receivedAt":"2009-12-25T08:40:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> - At first reading, the \"while (close(fd) < 0 && errno != EBADF);\"\n> pattern was a bit of eyesore. It might be worth factoring that out to\n> a small static helper function that a smart compiler would\n> automatically inline (or mark it as a static inline).\n\nThis also is a minor style thing, but we prefer your\n\n>>+\t\twhile(close(report_pipe[1]) < 0 && errno != EBADF);\n\nformatted like this:\n\n\twhile (foobar)\n \t; /* noop */\n\nto\n\n (1) have SP after syntactic keyword like while/if/switch to differentiate\n from function calls; and \n\n (2) make the no-op stand out for a bit more visibility.\n\n \n"},{"id":"130327","messageId":"20091225093220.GA8319@Knoppix","threadId":"22023","inReplyTo":"7vljgrebv2.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC PATCH 2/2] Improve transport helper exec failure reporting","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-25T09:32:20Z","receivedAt":"2009-12-25T09:32:20Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Thu, Dec 24, 2009 at 11:44:49PM -0800, Junio C Hamano wrote:\n> Ilari Liusvaara writes:\n> \n> Looks pretty straightforward, except that I have this nagging feeling that\n> we should *not* be married to the idea of \"'proc->git_cmd = 1' is merely a\n> way to save you from typing 'git-' prefix in start_command(proc)\".\n\nThat is already broken. If nothing previous broke it, then 1/2 of this\nseries did.\n\nThe immediate executable to run for 'git-foo' && git_cmd = 0: 'git-foo'.\nThe immediate executable to run for 'foo' && git_cmd = 1: 'git'(!).\n\nAnd one wants exec status for 'git-remote-foo', NOT for 'git'. Thus,\ngit_cmd must be 0 (at least without additional flags or flag values).\n \n> For example, we might later want to use different $PATH only when running\n> a git subcommand, and telling run_command() explicitly that we are running\n> a git thing would help if you don't add \"git-\" to the command line and\n> drop \"proc->git_cmd = 1\" in the caller like your patch does.\n\nWell, that would require new flag (or git_cmd field value) to mean do direct\nexec with gitexecdir in $PATH. Otherwise, you would either break this piece\nof code, or it would be already broken (depending on value of git_cmd).\n\n-Ilari\n"},{"id":"130328","messageId":"20091225095147.GB8319@Knoppix","threadId":"22023","inReplyTo":"7vr5qjecbb.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC PATCH 1/2] Report exec errors from run-command","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-25T09:51:48Z","receivedAt":"2009-12-25T09:51:48Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Thu, Dec 24, 2009 at 11:35:04PM -0800, Junio C Hamano wrote:\n> Ilari Liusvaara writes:\n> \n> Thanks; I think overall this is a good idea, even though I have no clue\n> if WIN32 side wants a similar support.\n\nIt would. But I have no clue about WIN32. But there are other people\non this list who have. :-)\n \n> - At first reading, the \"while (close(fd) < 0 && errno != EBADF);\"\n> pattern was a bit of eyesore. It might be worth factoring that out to\n> a small static helper function that a smart compiler would\n> automatically inline (or mark it as a static inline).\n\nDone (with bit of reformatting to add space, line break and comment.\n\n> - Is it guaranteed that a failed pipe(2) never touches its int fildes[2]\n> parameter, or the values are undefined when it fails? The approach\n> would save one extra variable, compared to an alternative approach to\n> keep an explicit variable to record a pipe failure, but It feels iffy\n> that the code relies on them being left as their initial -1 values.\n\nI added explicit set to -1 on failure case (I think failed pipe doesn't touch\nthose, but you never know about what some oddball OS is going to do).\n\n> - Should we worry about partial write as well (you seem to warn when you\n> get a partial read)? Is it worth using xread() and friends found in\n> wrapper.c instead of goto read/write_again?\n\nThat's hairy code. One really can't print any errors in write path, as those\nerrors would go to who knows where due to redirections (I took the error\nwarning out).\n\nThat partial read warning is more for detecting 'can't happen' situations\nsince pipe should be large enough to atomically transfer integer.\n\n> - Shouldn't any of these warning() be die() instead?\n\nIf error reporting failures are fatal, all of them.\n\n-Ilari\n"},{"id":"130333","messageId":"200912251539.35201.j6t@kdbg.org","threadId":"22023","inReplyTo":"1261676971-3285-2-git-send-email-ilari.liusvaara@elisanet.fi","subject":"Re: [RFC PATCH 1/2] Report exec errors from run-command","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-12-25T14:39:34Z","receivedAt":"2009-12-25T14:39:34Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Donnerstag, 24. Dezember 2009, Ilari Liusvaara 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\nThe only really *important* errno of a failed exec is ENOENT. For this case, \nwouldn't it be easier to do the PATH lookup manually in the parent (before \nthe fork()), and use execv() in the forked child rather than execvp()?\n\nThere is already a path lookup function in compat/mingw.c; it could certainly \nneed some improvement, but it is a starter.\n\nThat said, we don't need the stunt that you implemented on WIN32, because by \nthe time mingw_spawnvpe() returns, we have a running child process.\n\n-- Hannes\n"},{"id":"130339","messageId":"20091225171505.GA11830@Knoppix","threadId":"22023","inReplyTo":"200912251539.35201.j6t@kdbg.org","subject":"Re: [RFC PATCH 1/2] Report exec errors from run-command","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2009-12-25T17:15:05Z","receivedAt":"2009-12-25T17:15:05Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Fri, Dec 25, 2009 at 03:39:34PM +0100, Johannes Sixt wrote:\n> \n> The only really *important* errno of a failed exec is ENOENT. For this case, \n> wouldn't it be easier to do the PATH lookup manually in the parent (before \n> the fork()), and use execv() in the forked child rather than execvp()?\n\nIn fact there is API for getting all valid commands on $PATH for given command\nprefix. That would take care of ENOENT and most of EACCESS.\n\nBut OTOH, its nice to be able to report any error.\n\n-Ilari\n"}]}