[RFC PATCH v2 0/2] Imporove remote helper exec failure reporting

[RFC PATCH v2 0/2] Imporove remote helper exec failure reporting

[Ilari Liusvaara]

(This is newer than updated patch v3).

I decided to go on a bit further with new features, and that's why this
is back as RFC.

Diffrences from first RFC round:

- Split loop-over-close to its own function and warn but not retry on
  bizarre error codes (basically anything except EBADF and EINTR).
- Don't rely on pipe(2) to preserve the fd array on failure.
- Add some testcases.
- Fix buggy behaviour if report pipe read end winds up as fd #0.
- Set silent_exec_failure on executing remote helper.
- Make exec reporting in presence of preexec_cb controlled by flag to
  solve pager deadlock problems without preventing using preexec_cb
  with exec reporting entierely (the way pager is invoked is just
  fundamentially incompatible with exec reporting).
- Add flag to make run-command give different code for exec failing due to
  ENOENT than for other failures (including other ways to get ENOENT).
- change chdir failure to be reported as error (and pass error code back).
  BTW: Nothing seems to use that chdir feature, aside of new testcase.
- Report errors to parent's stderr.
- Don't retry waitpid on bizarre error codes (basically anything except
  EINTR).
- Make run_command actually complain about exec failing (as API docs
  say).

Ilari Liusvaara (2):
  Report exec errors from run-command
  Improve transport helper exec failure reporting

 Documentation/technical/api-run-command.txt |    2 +
 Makefile                                    |    1 +
 run-command.c                               |  196 +++++++++++++++++++++++++--
 run-command.h                               |    2 +
 t/t0061-run-command.sh                      |   20 +++
 test-run-command.c                          |   74 ++++++++++
 transport-helper.c                          |   27 +++-
 7 files changed, 305 insertions(+), 17 deletions(-)
 create mode 100755 t/t0061-run-command.sh
 create mode 100644 test-run-command.c

[RFC PATCH v2 1/2] Report exec errors from run-command

[Ilari Liusvaara]

Previously run-command was unable to report errors happening in exec
call. Change it to pass errno from failed exec to errno value at
return. Also add special mode that diffrentiates between ENOENT
from exec and ENOENT from other source.

The errno value passing can be done by opening close-on-exec pipe and
piping the error code through in case of failure. In case of success,
close-on-exec closes the pipe on successful exec and parent process
gets end of file on read.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 Documentation/technical/api-run-command.txt |    2 +
 Makefile                                    |    1 +
 run-command.c                               |  196 +++++++++++++++++++++++++--
 run-command.h                               |    2 +
 t/t0061-run-command.sh                      |   20 +++
 test-run-command.c                          |   74 ++++++++++
 6 files changed, 282 insertions(+), 13 deletions(-)
 create mode 100755 t/t0061-run-command.sh
 create mode 100644 test-run-command.c

diff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt
index b26c281..f37d8ba 100644
--- a/Documentation/technical/api-run-command.txt
+++ b/Documentation/technical/api-run-command.txt
@@ -49,6 +49,8 @@ The functions above do the following:
 
 . If the program was not found, then -1 is returned and errno is set to
   ENOENT; a diagnostic is printed only if .silent_exec_failure is 0.
+  (if extended_error_code flag is true, instead of -1, the return value
+  is -2 in this case).
 
 . Otherwise, the program is run. If it terminates regularly, its exit
   code is returned. No diagnistic is printed, even if the exit code is
diff --git a/Makefile b/Makefile
index a2fce5a..92eed68 100644
--- a/Makefile
+++ b/Makefile
@@ -1753,6 +1753,7 @@ TEST_PROGRAMS_NEED_X += test-path-utils
 TEST_PROGRAMS_NEED_X += test-sha1
 TEST_PROGRAMS_NEED_X += test-sigchain
 TEST_PROGRAMS_NEED_X += test-index-version
+TEST_PROGRAMS_NEED_X += test-run-command
 
 TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))
 
diff --git a/run-command.c b/run-command.c
index 47ced57..b1af9c4 100644
--- a/run-command.c
+++ b/run-command.c
@@ -15,6 +15,20 @@ static inline void dup_devnull(int to)
 	close(fd);
 }
 
+static inline void force_close(int fd)
+{
+	/*
+	 * Assume all errors except EINTR are persistent and
+	 * that EBADF signals already closed fd.
+	 */
+	while (close(fd) < 0 && errno != EBADF)
+		if (errno != EINTR) {
+			warning("Close returned error %s",
+				strerror(errno));
+			break;
+		}
+}
+
 static const char **prepare_shell_cmd(const char **argv)
 {
 	int argc, nargc = 0;
@@ -49,6 +63,8 @@ static const char **prepare_shell_cmd(const char **argv)
 }
 
 #ifndef WIN32
+int child_err = -1;
+
 static int execv_shell_cmd(const char **argv)
 {
 	const char **nargv = prepare_shell_cmd(argv);
@@ -57,6 +73,26 @@ static int execv_shell_cmd(const char **argv)
 	free(nargv);
 	return -1;
 }
+
+static void error_child(const char *err, ...)
+{
+	char msg[4096];
+	int len;
+	va_list args;
+	va_start(args, err);
+	len = vsnprintf(msg, sizeof(msg), err, args);
+	va_end(args);
+	if (len >= sizeof(msg))
+		len = sizeof(msg) - 1;
+
+	if (child_err < 0)
+		return;
+
+	write(child_err, "error: ", 7);
+	write(child_err, msg, len);
+	write(child_err, "\n", 1);
+}
+
 #endif
 
 int start_command(struct child_process *cmd)
@@ -64,6 +100,7 @@ int start_command(struct child_process *cmd)
 	int need_in, need_out, need_err;
 	int fdin[2], fdout[2], fderr[2];
 	int failed_errno = failed_errno;
+	int func_ret = 0;
 
 	/*
 	 * In case of errors we must keep the promise to close FDs
@@ -120,9 +157,91 @@ fail_pipe:
 	trace_argv_printf(cmd->argv, "trace: run_command:");
 
 #ifndef WIN32
+{
+	int report_pipe[2] = {-1, -1};
+
+	if (pipe(report_pipe) < 0) {
+		report_pipe[0] = -1;
+		report_pipe[1] = -1;
+		warning("Can't open pipe for exec status report: %s\n",
+			strerror(errno));
+	}
+
 	fflush(NULL);
 	cmd->pid = fork();
-	if (!cmd->pid) {
+	if (cmd->pid > 0) {
+		int r = 0, ret = 0;
+		force_close(report_pipe[1]);
+read_again:
+		if (report_pipe[0] >= 0)
+			r = read(report_pipe[0], &ret, sizeof(ret));
+		if (r < 0 && (errno == EAGAIN || errno == EINTR ||
+			errno == EWOULDBLOCK))
+			goto read_again;
+		else if (r < 0)
+			warning("Can't read exec status report: %s\n",
+				strerror(errno));
+		else if (r == 0)
+			;
+		else if (r < sizeof(ret)) {
+			warning("Received incomplete exec status report.\n");
+		} else {
+			failed_errno = ret;
+			/*
+			 * Clean up the process that did the failed execution
+			 * so no zombies remain.
+			 */
+			while (waitpid(cmd->pid, &ret, 0) < 0 && errno == EINTR)
+				/* Nothing. */ ;
+			if (WIFEXITED(ret) && WEXITSTATUS(ret) == 127)
+				/* Program was not found. */
+				func_ret = -2;
+			cmd->pid = -1;
+		}
+	} else if (!cmd->pid) {
+		int r = 0;
+		int errored = 0;
+		int flags;
+		force_close(report_pipe[0]);
+
+		flags = fcntl(report_pipe[1], F_GETFD);
+		if (flags < 0)
+			r = -1;
+		r = (r || fcntl(report_pipe[1], F_SETFD, flags | FD_CLOEXEC));
+		if (r) {
+			warning("Can't FD_CLOEXEC pipe for exec status report: %s",
+				strerror(errno));
+			force_close(report_pipe[1]);
+			report_pipe[1] = -1;
+		}
+
+		/*
+		 * Redirect the channel to write syscall error messages to
+		 * before redirecting the process's stderr so that all error
+		 * messages in subsequent call paths use the parent's stderr.
+		 */
+		if (cmd->no_stderr || need_err) {
+			child_err = dup(2);
+			if (child_err < 0 && errno != EBADF)
+				/* EBADF => No stderr, nowhere to go. */
+				warning("Can't dup fd for child exec errors: %s",
+					strerror(errno));
+			if (child_err < 0)
+				goto no_error_chan;
+
+			flags = fcntl(child_err, F_GETFL);
+			if (flags < 0)
+				flags = 0;
+			if (fcntl(child_err, F_SETFL, flags | FD_CLOEXEC) < 0) {
+				warning("Can't FD_CLOEXEC fd for child exec errors: %s",
+					strerror(errno));
+				force_close(report_pipe[1]);
+				child_err = -1;
+			}
+		} else
+			child_err = 2;
+no_error_chan:
+
 		if (cmd->no_stdin)
 			dup_devnull(0);
 		else if (need_in) {
@@ -152,9 +271,15 @@ fail_pipe:
 			close(cmd->out);
 		}
 
-		if (cmd->dir && chdir(cmd->dir))
-			die_errno("exec '%s': cd to '%s' failed", cmd->argv[0],
-			    cmd->dir);
+		if (cmd->dir && chdir(cmd->dir)) {
+			failed_errno = errno;
+			error_child("exec '%s': cd to '%s' failed: %s",
+				cmd->argv[0], cmd->dir, strerror(errno));
+			trace_printf("trace: exec '%s': cd to '%s' failed: %s\n",
+				cmd->argv[0], cmd->dir, strerror(errno));
+			errored = 1;
+			goto throw_fault;
+		}
 		if (cmd->env) {
 			for (; *cmd->env; cmd->env++) {
 				if (strchr(*cmd->env, '='))
@@ -163,8 +288,23 @@ fail_pipe:
 					unsetenv(*cmd->env);
 			}
 		}
-		if (cmd->preexec_cb)
+		if (cmd->preexec_cb) {
+			/*
+			 * We don't know what pre-exec callbacks do, and they
+			 * may do something that causes deadlock with exec
+			 * reporting. The sole user of this hook seems to
+			 * be pager, and it is run through shell, so one
+			 * wouldn't get useful error from exec reporting
+			 * and would get useful error from shell anyway. So
+			 * just disable exec reporting for such comamnds.
+			 * (unless explicitly specified otherwise).
+			 */
+			if (!cmd->report_preexec_cb) {
+				force_close(report_pipe[1]);
+				report_pipe[1] = -1;
+			}
 			cmd->preexec_cb();
+		}
 		if (cmd->git_cmd) {
 			execv_git_cmd(cmd->argv);
 		} else if (cmd->use_shell) {
@@ -172,13 +312,36 @@ fail_pipe:
 		} else {
 			execvp(cmd->argv[0], (char *const*) cmd->argv);
 		}
+		failed_errno = errno;
+
 		trace_printf("trace: exec '%s' failed: %s\n", cmd->argv[0],
 				strerror(errno));
-		exit(127);
-	}
-	if (cmd->pid < 0)
+
+		if (!cmd->silent_exec_failure || errno != ENOENT)
+			error_child("cannot exec '%s': %s",
+				cmd->argv[0], strerror(failed_errno));
+throw_fault:
+		r = 0;
+write_again:
+		if (report_pipe[1] >= 0)
+			r = write(report_pipe[1], &failed_errno,
+				sizeof(failed_errno));
+		if (r < 0 && (errno == EAGAIN || errno == EINTR ||
+			errno == EWOULDBLOCK))
+			goto write_again;
+
+		/*
+		 * Diffrentiate between program doesn't exist and other
+		 * errors.
+		 */
+		if (errored || errno != ENOENT)
+			exit(128);
+		else
+			exit(127);
+	} else if (cmd->pid < 0)
 		error("cannot fork() for %s: %s", cmd->argv[0],
 			strerror(failed_errno = errno));
+}
 #else
 {
 	int s0 = -1, s1 = -1, s2 = -1;	/* backups of stdin, stdout, stderr */
@@ -233,6 +396,8 @@ fail_pipe:
 	failed_errno = errno;
 	if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))
 		error("cannot spawn %s: %s", cmd->argv[0], strerror(errno));
+	if (cmd->pid < 0 && errno == ENOENT)
+		func_ret = -2;
 
 	if (cmd->env)
 		free_environ(env);
@@ -261,7 +426,10 @@ fail_pipe:
 		if (need_err)
 			close_pair(fderr);
 		errno = failed_errno;
-		return -1;
+		if (cmd->extended_error_code && func_ret)
+			return func_ret;
+		else
+			return -1;
 	}
 
 	if (need_in)
@@ -280,7 +448,8 @@ fail_pipe:
 	return 0;
 }
 
-static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
+static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure,
+	int extended_err)
 {
 	int status, code = -1;
 	pid_t waiting;
@@ -309,7 +478,7 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
 		 * Convert special exit code when execvp failed.
 		 */
 		if (code == 127) {
-			code = -1;
+			code = extended_err ? -2 : -1;
 			failed_errno = ENOENT;
 			if (!silent_exec_failure)
 				error("cannot run %s: %s", argv0,
@@ -324,7 +493,8 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
 
 int finish_command(struct child_process *cmd)
 {
-	return wait_or_whine(cmd->pid, cmd->argv[0], cmd->silent_exec_failure);
+	return wait_or_whine(cmd->pid, cmd->argv[0], cmd->silent_exec_failure,
+		cmd->extended_error_code);
 }
 
 int run_command(struct child_process *cmd)
@@ -410,7 +580,7 @@ int start_async(struct async *async)
 int finish_async(struct async *async)
 {
 #ifndef WIN32
-	int ret = wait_or_whine(async->pid, "child process", 0);
+	int ret = wait_or_whine(async->pid, "child process", 0, 0);
 #else
 	DWORD ret = 0;
 	if (WaitForSingleObject(async->tid, INFINITE) != WAIT_OBJECT_0)
diff --git a/run-command.h b/run-command.h
index 967ba8c..f6b4caf 100644
--- a/run-command.h
+++ b/run-command.h
@@ -34,6 +34,8 @@ struct child_process {
 	unsigned silent_exec_failure:1;
 	unsigned stdout_to_stderr:1;
 	unsigned use_shell:1;
+	unsigned extended_error_code:1;
+	unsigned report_preexec_cb:1;
 	void (*preexec_cb)(void);
 };
 
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
new file mode 100755
index 0000000..0c217b1
--- /dev/null
+++ b/t/t0061-run-command.sh
@@ -0,0 +1,20 @@
+#!/bin/sh
+#
+# Copyright (c) 2009 Ilari Liusvaara
+#
+
+test_description='Test run command'
+
+. ./test-lib.sh
+
+test_expect_success "reporting ENOENT (non-extended)" \
+"test-run-command 2"
+
+test_expect_success "reporting ENOENT (extended)" \
+"test-run-command 1"
+
+test_expect_success "reporting chdir failed (extended)" \
+"test-run-command 3"
+
+
+test_done
diff --git a/test-run-command.c b/test-run-command.c
new file mode 100644
index 0000000..db664f3
--- /dev/null
+++ b/test-run-command.c
@@ -0,0 +1,74 @@
+/*
+ * test-run-command.c: test run command API.
+ *
+ * (C) 2009 Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
+ *
+ * This code is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include "git-compat-util.h"
+#include "run-command.h"
+#include <string.h>
+#include <errno.h>
+
+int main(int argc, char **argv)
+{
+	char* procs[2];
+	struct child_process proc;
+	char test = argv[1][0];
+	memset(&proc, 0, sizeof(proc));
+
+	if(argc < 2)
+		return 1;
+
+
+	switch(test) {
+	case '1':
+		procs[0] = "does-not-exist-62896869286";
+		procs[1] = NULL;
+		proc.argv = (const char **)procs;
+		proc.extended_error_code = 1;
+
+		if (start_command(&proc) != -2) {
+			fprintf(stderr, "FAIL A\n");
+			return 1;
+		}
+		if (test == '1' && errno == ENOENT)
+			return 0;
+	case '2':
+		procs[0] = "does-not-exist-62896869286";
+		procs[1] = NULL;
+		proc.argv = (const char **)procs;
+		proc.extended_error_code = 0;
+
+		if (start_command(&proc) != -1) {
+			fprintf(stderr, "FAIL A\n");
+			return 1;
+		}
+		if (errno == ENOENT)
+			return 0;
+	case '3':
+#ifdef WIN32
+		/* This can't work on Win32, so just skip it. */
+		return 0;
+#endif
+		procs[0] = "does-not-exist-62896869286";
+		procs[1] = NULL;
+		proc.argv = (const char **)procs;
+		proc.dir = "does-not-exist-52632632733";
+		proc.extended_error_code = 1;
+
+		if (start_command(&proc) != -1) {
+			fprintf(stderr, "FAIL A\n");
+			return 1;
+		}
+		if (errno == ENOENT)
+			return 0;
+	}
+
+	fprintf(stderr, "FAIL B\n");
+
+	return 1;
+}
-- 
1.6.6.3.gaa2e1

[RFC PATCH v2 2/2] Improve transport helper exec failure reporting

[Ilari Liusvaara]

Previously transport-helper exec failure error reporting was pretty
much useless as it didn't report errors from execve, only from pipe
and fork. Now that run-command passes errno from exec, use the
improved support to actually print useful errors if execution fails.

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 transport-helper.c |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 5147296..72be965 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -102,6 +102,7 @@ static struct child_process *get_helper(struct transport *transport)
 	int refspec_nr = 0;
 	int refspec_alloc = 0;
 	int duped;
+	int code;
 
 	if (data->helper)
 		return data->helper;
@@ -111,13 +112,31 @@ static struct child_process *get_helper(struct transport *transport)
 	helper->out = -1;
 	helper->err = 0;
 	helper->argv = xcalloc(4, sizeof(*helper->argv));
-	strbuf_addf(&buf, "remote-%s", data->name);
+	strbuf_addf(&buf, "git-remote-%s", data->name);
 	helper->argv[0] = strbuf_detach(&buf, NULL);
 	helper->argv[1] = transport->remote->name;
 	helper->argv[2] = remove_ext_force(transport->url);
-	helper->git_cmd = 1;
-	if (start_command(helper))
-		die("Unable to run helper: git %s", helper->argv[0]);
+	helper->argv[2] = transport->url;
+	helper->git_cmd = 0;
+	helper->silent_exec_failure = 1;
+	helper->extended_error_code = 1;
+	/*
+	 * Print errors even if start_command should have printed them in
+	 * order to get proper fatal-level error message and include
+	 * errnos to make inevitable incomplete problem reports more useful
+	 * (those have nasty tendency of including only the last line that
+	 * looks like error).
+	 */
+	code = start_command(helper);
+	if (code == -2)
+		die("Unable to find remote helper for '%s'",
+			data->name);
+	else if(code == -1)
+		die_errno("Error running helper for '%s'", data->name);
+	else if(code != 0)
+		/* Shouldn't happen. */
+		die_errno("code %i running helper for '%s'", code, data->name);
+
 	data->helper = helper;
 	data->no_disconnect_req = 0;
 
-- 
1.6.6.3.gaa2e1

[PATCH 0/4] Detect exec errors in start_command early

[Johannes Sixt]

On Samstag, 9. Januar 2010, Ilari Liusvaara wrote:
>  6 files changed, 282 insertions(+), 13 deletions(-)

IMHO, you are going completely overboard with your patch.

You check every single possible and unlikely error condition in the new code 
that you introde, but ignore that there are quite a number of calls in the 
existing code whose error conditions go checked.

And as a result, you "only" report errno values of some calls, and ignore that 
there is at least one potential die() call hidden in execv_git_cmd(). IOW, 
your code is not a catch-all and only an isolated solution.

I hope I can do better.

I developed this series on top of il/vcs-helper. But patches 1-3/4 are 
actually an independent topic and also suitable to be merged into 
jk/run-command-use-shell to implement improved DWIM whether a shell is 
needed.

Patch 4/4 is to address your problem of weak error reporting with transport 
helpers. I think I understand now what your problem is -- namely that 
previously no error was reported if a transport helper program did not exist. 
The reason for the missing error message is IMO a design weakness in the 
protocol: It requires that the parent talks first - and by talking to an 
early-died child process it dies from SIGPIPE. I experimented with 
signal(SIGPIPE, SIG_IGN) to get around this, but it didn't work (the SIGPIPE 
arrived anyway).

Anyway, patch 4/4 goes on top of il/vcs-helper after merging the topic 
consisting of 1-3/4.

Ilari Liusvaara (1):
  Improve error message when a transport helper was not found

Johannes Sixt (3):
  start_command: report child process setup errors to the parent's
    stderr
  run-command: move wait_or_whine earlier
  start_command: detect execvp failures early

 Makefile               |    1 +
 run-command.c          |  177 
+++++++++++++++++++++++++++++++++++-------------
 t/t0061-run-command.sh |   14 ++++
 test-run-command.c     |   35 ++++++++++
 transport-helper.c     |   14 +++-
 5 files changed, 191 insertions(+), 50 deletions(-)
 create mode 100755 t/t0061-run-command.sh
 create mode 100644 test-run-command.c

[PATCH 1/4] start_command: report child process setup errors to the parent's stderr

[Johannes Sixt]

When the child process's environment is set up in start_command(), error
messages were written to wherever the parent redirected the child's stderr
channel. However, even if the parent redirected the child's stderr, errors
during this setup process, including the exec itself, are usually an
indication of a problem in the parent's environment. Therefore, the error
messages should go to the parent's stderr.

Redirection of the child's error messages is usually only used to redirect
hook error messages during client-server exchanges. In these cases, hook
setup errors could be regarded as information leak.

This patch makes a copy of stderr if necessary and uses a special
die routine that is used for all die() calls in the child that sends the
errors messages to the parent's stderr.

The trace call that reported a failed execvp is removed (because it writes
to stderr) and replaced by die_errno() with special treatment of ENOENT.
The improvement in the error message can be seen with this sequence:

   mkdir .git/hooks/pre-commit
   git commit

Previously, the error message was

   error: cannot run .git/hooks/pre-commit: No such file or directory

and now it is

   fatal: cannot exec '.git/hooks/pre-commit': Permission denied

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 This should address your concern that "errors go to who-knows-where",
 that I meanwhile share with you.

 run-command.c |   46 +++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/run-command.c b/run-command.c
index cf2d8f7..02c7bfb 100644
--- a/run-command.c
+++ b/run-command.c
@@ -15,6 +15,30 @@ static inline void dup_devnull(int to)
 	close(fd);
 }
 
+#ifndef WIN32
+static int child_err = 2;
+
+static NORETURN void die_child(const char *err, va_list params)
+{
+	char msg[4096];
+	int len = vsnprintf(msg, sizeof(msg), err, params);
+	if (len > sizeof(msg))
+		len = sizeof(msg);
+
+	write(child_err, "fatal: ", 7);
+	write(child_err, msg, len);
+	write(child_err, "\n", 1);
+	exit(128);
+}
+
+static inline void set_cloexec(int fd)
+{
+	int flags = fcntl(fd, F_GETFD);
+	if (flags >= 0)
+		fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
+}
+#endif
+
 int start_command(struct child_process *cmd)
 {
 	int need_in, need_out, need_err;
@@ -79,6 +103,17 @@ fail_pipe:
 	fflush(NULL);
 	cmd->pid = fork();
 	if (!cmd->pid) {
+		/*
+		 * Redirect the channel to write syscall error messages to
+		 * before redirecting the process's stderr so that all die()
+		 * in subsequent call paths use the parent's stderr.
+		 */
+		if (cmd->no_stderr || need_err) {
+			child_err = dup(2);
+			set_cloexec(child_err);
+		}
+		set_die_routine(die_child);
+
 		if (cmd->no_stdin)
 			dup_devnull(0);
 		else if (need_in) {
@@ -126,9 +161,14 @@ fail_pipe:
 		} else {
 			execvp(cmd->argv[0], (char *const*) cmd->argv);
 		}
-		trace_printf("trace: exec '%s' failed: %s\n", cmd->argv[0],
-				strerror(errno));
-		exit(127);
+		/*
+		 * Do not check for cmd->silent_exec_failure; the parent
+		 * process will check it when it sees this exit code.
+		 */
+		if (errno == ENOENT)
+			exit(127);
+		else
+			die_errno("cannot exec '%s'", cmd->argv[0]);
 	}
 	if (cmd->pid < 0)
 		error("cannot fork() for %s: %s", cmd->argv[0],
-- 
1.6.6.115.gd1ab3

[PATCH 2/4] run-command: move wait_or_whine earlier

[Johannes Sixt]

We want to reuse it from start_command.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 run-command.c |   84 ++++++++++++++++++++++++++++----------------------------
 1 files changed, 42 insertions(+), 42 deletions(-)

diff --git a/run-command.c b/run-command.c
index 02c7bfb..dccac37 100644
--- a/run-command.c
+++ b/run-command.c
@@ -39,6 +39,48 @@ static inline void set_cloexec(int fd)
 }
 #endif
 
+static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
+{
+	int status, code = -1;
+	pid_t waiting;
+	int failed_errno = 0;
+
+	while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)
+		;	/* nothing */
+
+	if (waiting < 0) {
+		failed_errno = errno;
+		error("waitpid for %s failed: %s", argv0, strerror(errno));
+	} else if (waiting != pid) {
+		error("waitpid is confused (%s)", argv0);
+	} else if (WIFSIGNALED(status)) {
+		code = WTERMSIG(status);
+		error("%s died of signal %d", argv0, code);
+		/*
+		 * This return value is chosen so that code & 0xff
+		 * mimics the exit code that a POSIX shell would report for
+		 * a program that died from this signal.
+		 */
+		code -= 128;
+	} else if (WIFEXITED(status)) {
+		code = WEXITSTATUS(status);
+		/*
+		 * Convert special exit code when execvp failed.
+		 */
+		if (code == 127) {
+			code = -1;
+			failed_errno = ENOENT;
+			if (!silent_exec_failure)
+				error("cannot run %s: %s", argv0,
+					strerror(ENOENT));
+		}
+	} else {
+		error("waitpid is confused (%s)", argv0);
+	}
+	errno = failed_errno;
+	return code;
+}
+
 int start_command(struct child_process *cmd)
 {
 	int need_in, need_out, need_err;
@@ -272,48 +314,6 @@ fail_pipe:
 	return 0;
 }
 
-static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
-{
-	int status, code = -1;
-	pid_t waiting;
-	int failed_errno = 0;
-
-	while ((waiting = waitpid(pid, &status, 0)) < 0 && errno == EINTR)
-		;	/* nothing */
-
-	if (waiting < 0) {
-		failed_errno = errno;
-		error("waitpid for %s failed: %s", argv0, strerror(errno));
-	} else if (waiting != pid) {
-		error("waitpid is confused (%s)", argv0);
-	} else if (WIFSIGNALED(status)) {
-		code = WTERMSIG(status);
-		error("%s died of signal %d", argv0, code);
-		/*
-		 * This return value is chosen so that code & 0xff
-		 * mimics the exit code that a POSIX shell would report for
-		 * a program that died from this signal.
-		 */
-		code -= 128;
-	} else if (WIFEXITED(status)) {
-		code = WEXITSTATUS(status);
-		/*
-		 * Convert special exit code when execvp failed.
-		 */
-		if (code == 127) {
-			code = -1;
-			failed_errno = ENOENT;
-			if (!silent_exec_failure)
-				error("cannot run %s: %s", argv0,
-					strerror(ENOENT));
-		}
-	} else {
-		error("waitpid is confused (%s)", argv0);
-	}
-	errno = failed_errno;
-	return code;
-}
-
 int finish_command(struct child_process *cmd)
 {
 	return wait_or_whine(cmd->pid, cmd->argv[0], cmd->silent_exec_failure);
-- 
1.6.6.115.gd1ab3

[PATCH 3/4] start_command: detect execvp failures early

[Johannes Sixt]

Previously, failures during execvp could be detected only by
finish_command. However, in some situations it is beneficial for the
parent process to know earlier that the child process will not run.

The idea to use a pipe to signal failures to the parent process and
the test case were lifted from patches by Ilari Liusvaara.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 Makefile               |    1 +
 run-command.c          |   47 ++++++++++++++++++++++++++++++++++++++++++++++-
 t/t0061-run-command.sh |   14 ++++++++++++++
 test-run-command.c     |   35 +++++++++++++++++++++++++++++++++++
 4 files changed, 96 insertions(+), 1 deletions(-)
 create mode 100755 t/t0061-run-command.sh
 create mode 100644 test-run-command.c

diff --git a/Makefile b/Makefile
index 87fc7ff..22c1546 100644
--- a/Makefile
+++ b/Makefile
@@ -1785,6 +1785,7 @@ TEST_PROGRAMS += test-genrandom$X
 TEST_PROGRAMS += test-match-trees$X
 TEST_PROGRAMS += test-parse-options$X
 TEST_PROGRAMS += test-path-utils$X
+TEST_PROGRAMS += test-run-command$X
 TEST_PROGRAMS += test-sha1$X
 TEST_PROGRAMS += test-sigchain$X
 
diff --git a/run-command.c b/run-command.c
index dccac37..473b7f8 100644
--- a/run-command.c
+++ b/run-command.c
@@ -17,6 +17,12 @@ static inline void dup_devnull(int to)
 
 #ifndef WIN32
 static int child_err = 2;
+static int child_notifier = -1;
+
+static void notify_parent()
+{
+	write(child_notifier, "", 1);
+}
 
 static NORETURN void die_child(const char *err, va_list params)
 {
@@ -142,6 +148,11 @@ fail_pipe:
 	trace_argv_printf(cmd->argv, "trace: run_command:");
 
 #ifndef WIN32
+{
+	int notify_pipe[2];
+	if (pipe(notify_pipe))
+		notify_pipe[0] = notify_pipe[1] = -1;
+
 	fflush(NULL);
 	cmd->pid = fork();
 	if (!cmd->pid) {
@@ -156,6 +167,11 @@ fail_pipe:
 		}
 		set_die_routine(die_child);
 
+		close(notify_pipe[0]);
+		set_cloexec(notify_pipe[1]);
+		child_notifier = notify_pipe[1];
+		atexit(notify_parent);
+
 		if (cmd->no_stdin)
 			dup_devnull(0);
 		else if (need_in) {
@@ -196,8 +212,16 @@ fail_pipe:
 					unsetenv(*cmd->env);
 			}
 		}
-		if (cmd->preexec_cb)
+		if (cmd->preexec_cb) {
+			/*
+			 * We cannot predict what the pre-exec callback does.
+			 * Forgo parent notification.
+			 */
+			close(child_notifier);
+			child_notifier = -1;
+
 			cmd->preexec_cb();
+		}
 		if (cmd->git_cmd) {
 			execv_git_cmd(cmd->argv);
 		} else {
@@ -215,6 +239,27 @@ fail_pipe:
 	if (cmd->pid < 0)
 		error("cannot fork() for %s: %s", cmd->argv[0],
 			strerror(failed_errno = errno));
+
+	/*
+	 * Wait for child's execvp. If the execvp succeeds (or if fork()
+	 * failed), EOF is seen immediately by the parent. Otherwise, the
+	 * child process sends a single byte.
+	 * Note that use of this infrastructure is completely advisory,
+	 * therefore, we keep error checks minimal.
+	 */
+	close(notify_pipe[1]);
+	if (read(notify_pipe[0], &notify_pipe[1], 1) == 1) {
+		/*
+		 * At this point we know that fork() succeeded, but execvp()
+		 * failed. Errors have been reported to our stderr.
+		 */
+		wait_or_whine(cmd->pid, cmd->argv[0],
+			      cmd->silent_exec_failure);
+		failed_errno = errno;
+		cmd->pid = -1;
+	}
+	close(notify_pipe[0]);
+}
 #else
 {
 	int s0 = -1, s1 = -1, s2 = -1;	/* backups of stdin, stdout, stderr */
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
new file mode 100755
index 0000000..10b26e4
--- /dev/null
+++ b/t/t0061-run-command.sh
@@ -0,0 +1,14 @@
+#!/bin/sh
+#
+# Copyright (c) 2009 Ilari Liusvaara
+#
+
+test_description='Test run command'
+
+. ./test-lib.sh
+
+test_expect_success 'start_command reports ENOENT' '
+	test-run-command start-command-ENOENT ./does-not-exist
+'
+
+test_done
diff --git a/test-run-command.c b/test-run-command.c
new file mode 100644
index 0000000..a1d5306
--- /dev/null
+++ b/test-run-command.c
@@ -0,0 +1,35 @@
+/*
+ * test-run-command.c: test run command API.
+ *
+ * (C) 2009 Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
+ *
+ * This code is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include "git-compat-util.h"
+#include "run-command.h"
+#include <string.h>
+#include <errno.h>
+
+int main(int argc, char **argv)
+{
+	struct child_process proc;
+
+	memset(&proc, 0, sizeof(proc));
+
+	if(argc < 3)
+		return 1;
+	proc.argv = (const char **)argv+2;
+
+	if (!strcmp(argv[1], "start-command-ENOENT")) {
+		if (start_command(&proc) < 0 && errno == ENOENT)
+			return 0;
+		fprintf(stderr, "FAIL %s\n", argv[1]);
+		return 1;
+	}
+
+	fprintf(stderr, "check usage\n");
+	return 1;
+}
-- 
1.6.6.115.gd1ab3

[PATCH 4/4] Improve error message when a transport helper was not found

[Johannes Sixt]

From: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>

Perviously, the error message was:

    git: 'remote-foo' is not a git-command. See 'git --help'.

By not treating the transport helper as a git command, a more suitable
error is reported:

    fatal: Unable to find remote helper for 'foo'

Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 While I agree mostly with the implementation of your patch, the justification
 was completely off, and I think it was because you did not notice that the
 parent process died from SIGPIPE. When patches 1-3/4 are applied, the
 parent does not die from SIGPIPE anymore, and the issue is now only minor
 (an unsuitable error message instead of no error message). Therefore,
 I rewrote the commit message to tone it down.

 -- Hannes

 transport-helper.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 6ece0d9..d1bc3af 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -102,6 +102,7 @@ static struct child_process *get_helper(struct transport *transport)
 	int refspec_nr = 0;
 	int refspec_alloc = 0;
 	int duped;
+	int code;
 
 	if (data->helper)
 		return data->helper;
@@ -111,13 +112,18 @@ static struct child_process *get_helper(struct transport *transport)
 	helper->out = -1;
 	helper->err = 0;
 	helper->argv = xcalloc(4, sizeof(*helper->argv));
-	strbuf_addf(&buf, "remote-%s", data->name);
+	strbuf_addf(&buf, "git-remote-%s", data->name);
 	helper->argv[0] = strbuf_detach(&buf, NULL);
 	helper->argv[1] = transport->remote->name;
 	helper->argv[2] = remove_ext_force(transport->url);
-	helper->git_cmd = 1;
-	if (start_command(helper))
-		die("Unable to run helper: git %s", helper->argv[0]);
+	helper->git_cmd = 0;
+	helper->silent_exec_failure = 1;
+	code = start_command(helper);
+	if (code < 0 && errno == ENOENT)
+		die("Unable to find remote helper for '%s'", data->name);
+	else
+		exit(code);
+
 	data->helper = helper;
 	data->no_disconnect_req = 0;
 
-- 
1.6.6.115.gd1ab3

Re: [PATCH 3/4] start_command: detect execvp failures early

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Previously, failures during execvp could be detected only by
> finish_command. However, in some situations it is beneficial for the
> parent process to know earlier that the child process will not run.
>
> The idea to use a pipe to signal failures to the parent process and
> the test case were lifted from patches by Ilari Liusvaara.

I wonder if we can do this without pipe, perhaps using "vfork, exec, then
update a variable"....

Re: [PATCH 3/4] start_command: detect execvp failures early

[Johannes Sixt]

On Donnerstag, 14. Januar 2010, Junio C Hamano wrote:
> Johannes Sixt <j6t@kdbg.org> writes:
> > Previously, failures during execvp could be detected only by
> > finish_command. However, in some situations it is beneficial for the
> > parent process to know earlier that the child process will not run.
> >
> > The idea to use a pipe to signal failures to the parent process and
> > the test case were lifted from patches by Ilari Liusvaara.
>
> I wonder if we can do this without pipe, perhaps using "vfork, exec, then
> update a variable"....

Except that some systems implement vfork(2) in terms of fork(2), I heard. 
Moreover, I think that we do way too many things before the exec; isn't that 
dangerous?

-- Hannes

Re: [PATCH 3/4] start_command: detect execvp failures early

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> On Donnerstag, 14. Januar 2010, Junio C Hamano wrote:
>> Johannes Sixt <j6t@kdbg.org> writes:
>> > Previously, failures during execvp could be detected only by
>> > finish_command. However, in some situations it is beneficial for the
>> > parent process to know earlier that the child process will not run.
>> >
>> > The idea to use a pipe to signal failures to the parent process and
>> > the test case were lifted from patches by Ilari Liusvaara.
>>
>> I wonder if we can do this without pipe, perhaps using "vfork, exec, then
>> update a variable"....
>
> Except that some systems implement vfork(2) in terms of fork(2), I heard. 

As long as they implement it correctly, we don't care if it is implemented
in terms of fork or knife or chopstick.  The only thing we care about in
this crazy-idea was the shared address space the child can write into
until it exec or _exit while the parent is in a frozen state.

> Moreover, I think that we do way too many things before the exec; isn't that 
> dangerous?

Yeah, we do seem to do quite a lot more than I thought.  Scratch that
crazy idea, then.