[updated patch v2 0/2] Improve remote helper exec failure reporting

[updated patch v2 0/2] Improve remote helper exec failure reporting

[Ilari Liusvaara]

Changes since updated patch (second round):
- Work around deadlock with pager.
- Fix exec reporting if read end of report pipe winds up to be fd #0.
- Limit number of retries in force_close for unexpected errors.

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

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

[updated 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.

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: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
---
 Makefile               |    1 +
 run-command.c          |  103 ++++++++++++++++++++++++++++++++++++++++++++++--
 t/t0061-run-command.sh |   13 ++++++
 test-run-command.c     |   35 ++++++++++++++++
 4 files changed, 148 insertions(+), 4 deletions(-)
 create mode 100755 t/t0061-run-command.sh
 create mode 100644 test-run-command.c

diff --git a/Makefile b/Makefile
index 4a1e5bc..452ad50 100644
--- a/Makefile
+++ b/Makefile
@@ -1725,6 +1725,7 @@ TEST_PROGRAMS += test-parse-options$X
 TEST_PROGRAMS += test-path-utils$X
 TEST_PROGRAMS += test-sha1$X
 TEST_PROGRAMS += test-sigchain$X
+TEST_PROGRAMS += test-run-command$X
 
 all:: $(TEST_PROGRAMS)
 
diff --git a/run-command.c b/run-command.c
index cf2d8f7..1c5b103 100644
--- a/run-command.c
+++ b/run-command.c
@@ -15,6 +15,22 @@ static inline void dup_devnull(int to)
 	close(fd);
 }
 
+static inline void force_close(int fd)
+{
+	int err = 0;
+	/*
+	 * Retry EINTRs undefinitely, exit on EBADF immediately, other
+	 * errors retry only up to three times (even if pipe close
+	 * shouldn't cause other errors, but you never know with
+	 * what broken systems may return on closed file descriptor).
+	 * consequences of failure to close pipe here may include
+	 * deadlocking.
+	 */
+	while (close(fd) < 0 && errno != EBADF && err < 3)
+		if(errno != EINTR)
+			err++;
+}
+
 int start_command(struct child_process *cmd)
 {
 	int need_in, need_out, need_err;
@@ -76,9 +92,64 @@ 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;
+		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");
+			errno = EBADMSG;
+		} else {
+			failed_errno = ret;
+			/*
+			 * Clean up the process that did the failed execution
+			 * so no zombies remain.
+			 */
+wait_again:
+			r = waitpid(cmd->pid, &ret, 0);
+			if (r < 0 && errno != ECHILD)
+				goto wait_again;
+			cmd->pid = -1;
+		}
+	} else if (!cmd->pid) {
+		int r = 0;
+		int flags;
+		force_close(report_pipe[0]);
+
+		flags = fcntl(report_pipe[1], F_GETFD);
+		if (flags < 0)
+			r = -1;
+		flags |= FD_CLOEXEC;
+		r = (r || fcntl(report_pipe[1], F_SETFD, flags));
+		if (r) {
+			warning("Can't FD_CLOEXEC pipe for exec status "
+				"report: %s\n", strerror(errno));
+			force_close(report_pipe[1]);
+			report_pipe[1] = -1;
+		}
+
 		if (cmd->no_stdin)
 			dup_devnull(0);
 		else if (need_in) {
@@ -119,20 +190,44 @@ 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.
+			 */
+			force_close(report_pipe[1]);
+			report_pipe[1] = -1;
 			cmd->preexec_cb();
+		}
 		if (cmd->git_cmd) {
 			execv_git_cmd(cmd->argv);
 		} 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));
+
+		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;
+
 		exit(127);
-	}
-	if (cmd->pid < 0)
+	} 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 */
diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
new file mode 100755
index 0000000..1d9e82a
--- /dev/null
+++ b/t/t0061-run-command.sh
@@ -0,0 +1,13 @@
+#!/bin/sh
+#
+# Copyright (c) 2009 Ilari Liusvaara
+#
+
+test_description='Test run command'
+
+. ./test-lib.sh
+
+test_expect_success "reporting ENOENT" \
+"test-run-command 1"
+
+test_done
diff --git a/test-run-command.c b/test-run-command.c
new file mode 100644
index 0000000..6297785
--- /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)
+{
+	char* procs[2];
+	struct child_process proc;
+	memset(&proc, 0, sizeof(proc));
+
+	if(argc < 2)
+		return 1;
+
+	if (argv[1][1] == '1')
+		procs[0] = "does-not-exist-62896869286";
+	procs[1] = NULL;
+	proc.argv = (const char **)procs;
+
+	if (!run_command(&proc))
+		return 1;
+	if (errno != ENOENT)
+		return 1;
+	return 0;
+}
-- 
1.6.6.3.gaa2e1

Re: [updated patch v2 1/2] Report exec errors from run-command

[Johannes Sixt]

Ilari Liusvaara schrieb:
> +static inline void force_close(int fd)
> +{
> +	int err = 0;
> +	/*
> +	 * Retry EINTRs undefinitely, exit on EBADF immediately, other
> +	 * errors retry only up to three times (even if pipe close
> +	 * shouldn't cause other errors, but you never know with
> +	 * what broken systems may return on closed file descriptor).
> +	 * consequences of failure to close pipe here may include
> +	 * deadlocking.
> +	 */
> +	while (close(fd) < 0 && errno != EBADF && err < 3)
> +		if(errno != EINTR)
> +			err++;

What's the point to iterate on all errors except EBADF? If the close() 
fails once, it will fail again.

> +			/*
> +			 * Clean up the process that did the failed execution
> +			 * so no zombies remain.
> +			 */
> +wait_again:
> +			r = waitpid(cmd->pid, &ret, 0);
> +			if (r < 0 && errno != ECHILD)
> +				goto wait_again;

You really should iterate only on well-known errors. What's wrong with

		while (waitpid(pid, &status, 0) < 0 && errno == EINTR)
			;	/* nothing */

similar to wait_or_whine()'s call to waitpid() and to avoid goto.

> +int main(int argc, char **argv)
> +{
> +	char* procs[2];
> +	struct child_process proc;
> +	memset(&proc, 0, sizeof(proc));
> +
> +	if(argc < 2)
> +		return 1;
> +
> +	if (argv[1][1] == '1')
> +		procs[0] = "does-not-exist-62896869286";
> +	procs[1] = NULL;
> +	proc.argv = (const char **)procs;
> +
> +	if (!run_command(&proc))
> +		return 1;
> +	if (errno != ENOENT)
> +		return 1;
> +	return 0;
> +}

This test is not specific enough: It would pass even without your change 
to start_command(), because finish_command() detects the ENOENT case. You 
really want to test that you see ENOENT after start_command() (i.e., 
before finish_command()).

-- Hannes

[updated 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 |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 5078c71..0965c9b 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -31,13 +31,19 @@ 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] = transport->url;
-	helper->git_cmd = 1;
-	if (start_command(helper))
-		die("Unable to run helper: git %s", helper->argv[0]);
+	helper->git_cmd = 0;
+	if (start_command(helper)) {
+		if (errno == ENOENT)
+			die("Unable to find remote helper for \"%s\"",
+				data->name);
+		else
+			die("Unable to run helper %s: %s", helper->argv[0],
+				strerror(errno));
+	}
 	data->helper = helper;
 
 	write_str_in_full(helper->in, "capabilities\n");
-- 
1.6.6.3.gaa2e1