[RFC PATCH 0/2] Report remote helper exec failures

[RFC PATCH 0/2] Report remote helper exec failures

[Ilari Liusvaara]

Actually give useful error messages if executing git remote helper fails
for some reason.

The previous error message was:

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

This changes the error message to:

'Unable to find remote helper for "nonexistent"'

or

'Unable to run helper HelperThatGetsEACCESS: Permission denied' 
(or whatever the errno is).


Patch series is based on v1.6.6.

One of the changes is adjacent to changes in external helper dispatch
support change and gets merge conflict. Here's how I resolved that
conflict for testing:

        helper->argv[2] = remove_ext_force(transport->url);
        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));
        }

The first line in that comes from dispatch support and the rest come from
exec failure reporting.


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

 run-command.c      |   72 +++++++++++++++++++++++++++++++++++++++++++++++++--
 transport-helper.c |   14 +++++++---
 2 files changed, 79 insertions(+), 7 deletions(-)

[RFC PATCH 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>
---
 run-command.c |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 69 insertions(+), 3 deletions(-)

diff --git a/run-command.c b/run-command.c
index cf2d8f7..d334d0f 100644
--- a/run-command.c
+++ b/run-command.c
@@ -76,9 +76,60 @@ fail_pipe:
 	trace_argv_printf(cmd->argv, "trace: run_command:");
 
 #ifndef WIN32
+{
+	int report_pipe[2] = {-1, -1};
+
+	if (pipe(report_pipe) < 0)
+		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;
+		while(close(report_pipe[1]) < 0 && errno != EBADF);
+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.
+			 */
+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;
+		while(close(report_pipe[0]) < 0 && errno != EBADF);
+
+		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));
+			while(close(report_pipe[1]) < 0 && errno != EBADF);
+			report_pipe[1] = -1;
+		}
+
 		if (cmd->no_stdin)
 			dup_devnull(0);
 		else if (need_in) {
@@ -126,13 +177,28 @@ 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));
+
+		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;
+		else if (r < 0)
+			warning("Can't write exec status report: %s\n",
+				strerror(errno));
+
 		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 */
-- 
1.6.6.3.gaa2e1

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

[Junio C Hamano]

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

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

Thanks; I think overall this is a good idea, even though I have no clue
if WIN32 side wants a similar support.

 - At first reading, the "while (close(fd) < 0 && errno != EBADF);"
   pattern was a bit of eyesore.  It might be worth factoring that out to
   a small static helper function that a smart compiler would
   automatically inline (or mark it as a static inline).

 - Is it guaranteed that a failed pipe(2) never touches its int fildes[2]
   parameter, or the values are undefined when it fails?  The approach
   would save one extra variable, compared to an alternative approach to
   keep an explicit variable to record a pipe failure, but It feels iffy
   that the code relies on them being left as their initial -1 values.

 - Should we worry about partial write as well (you seem to warn when you
   get a partial read)?  Is it worth using xread() and friends found in
   wrapper.c instead of goto read/write_again?

 - Shouldn't any of these warning() be die() instead?

 - The two extra file descriptors this patch uses are allocated after all
   the existing pipe flipping is done, and all the dup's done in the child
   process are to use dup2() to set up the known file descriptors at low
   numbers, so I don't think we have to worry about this patch changing
   the behaviour of the process pair by changing the assignment of file
   descriptors (we had a bug or two in the past that made subprocess
   misbehave under some funky condition, e.g. run with fd#0 closed).

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:
>
>> 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.
>
> Thanks; I think overall this is a good idea, even though I have no clue
> if WIN32 side wants a similar support.

By the way, at least it should be pretty straightforward to add a test or
two for [1/2] if not [2/2].

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>  - At first reading, the "while (close(fd) < 0 && errno != EBADF);"
>    pattern was a bit of eyesore.  It might be worth factoring that out to
>    a small static helper function that a smart compiler would
>    automatically inline (or mark it as a static inline).

This also is a minor style thing, but we prefer your

>>+		while(close(report_pipe[1]) < 0 && errno != EBADF);

formatted like this:

	while (foobar)
        	; /* noop */

to

 (1) have SP after syntactic keyword like while/if/switch to differentiate
     from function calls; and 

 (2) make the no-op stand out for a bit more visibility.

 

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

[Ilari Liusvaara]

On Thu, Dec 24, 2009 at 11:35:04PM -0800, Junio C Hamano wrote:
> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:
> 
> Thanks; I think overall this is a good idea, even though I have no clue
> if WIN32 side wants a similar support.

It would. But I have no clue about WIN32. But there are other people
on this list who have. :-)
 
>  - At first reading, the "while (close(fd) < 0 && errno != EBADF);"
>    pattern was a bit of eyesore.  It might be worth factoring that out to
>    a small static helper function that a smart compiler would
>    automatically inline (or mark it as a static inline).

Done (with bit of reformatting to add space, line break and comment.

>  - Is it guaranteed that a failed pipe(2) never touches its int fildes[2]
>    parameter, or the values are undefined when it fails?  The approach
>    would save one extra variable, compared to an alternative approach to
>    keep an explicit variable to record a pipe failure, but It feels iffy
>    that the code relies on them being left as their initial -1 values.

I added explicit set to -1 on failure case (I think failed pipe doesn't touch
those, but you never know about what some oddball OS is going to do).

>  - Should we worry about partial write as well (you seem to warn when you
>    get a partial read)?  Is it worth using xread() and friends found in
>    wrapper.c instead of goto read/write_again?

That's hairy code. One really can't print any errors in write path, as those
errors would go to who knows where due to redirections (I took the error
warning out).

That partial read warning is more for detecting 'can't happen' situations
since pipe should be large enough to atomically transfer integer.

>  - Shouldn't any of these warning() be die() instead?

If error reporting failures are fatal, all of them.

-Ilari

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

[Johannes Sixt]

On Donnerstag, 24. Dezember 2009, Ilari Liusvaara wrote:
> 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.

The only really *important* errno of a failed exec is ENOENT. For this case, 
wouldn't it be easier to do the PATH lookup manually in the parent (before 
the fork()), and use execv() in the forked child rather than execvp()?

There is already a path lookup function in compat/mingw.c; it could certainly 
need some improvement, but it is a starter.

That said, we don't need the stunt that you implemented on WIN32, because by 
the time mingw_spawnvpe() returns, we have a running child process.

-- Hannes

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

[Ilari Liusvaara]

On Fri, Dec 25, 2009 at 03:39:34PM +0100, Johannes Sixt wrote:
> 
> The only really *important* errno of a failed exec is ENOENT. For this case, 
> wouldn't it be easier to do the PATH lookup manually in the parent (before 
> the fork()), and use execv() in the forked child rather than execvp()?

In fact there is API for getting all valid commands on $PATH for given command
prefix. That would take care of ENOENT and most of EACCESS.

But OTOH, its nice to be able to report any error.

-Ilari

[RFC PATCH 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

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

[Junio C Hamano]

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

> 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>

Looks pretty straightforward, except that I have this nagging feeling that
we should *not* be married to the idea of "'proc->git_cmd = 1' is merely a
way to save you from typing 'git-' prefix in start_command(proc)".

> 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)) {

For example, we might later want to use different $PATH only when running
a git subcommand, and telling run_command() explicitly that we are running
a git thing would help if you don't add "git-" to the command line and
drop "proc->git_cmd = 1" in the caller like your patch does.

> +		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

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

[Ilari Liusvaara]

On Thu, Dec 24, 2009 at 11:44:49PM -0800, Junio C Hamano wrote:
> Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:
> 
> Looks pretty straightforward, except that I have this nagging feeling that
> we should *not* be married to the idea of "'proc->git_cmd = 1' is merely a
> way to save you from typing 'git-' prefix in start_command(proc)".

That is already broken. If nothing previous broke it, then 1/2 of this
series did.

The immediate executable to run for 'git-foo' && git_cmd = 0: 'git-foo'.
The immediate executable to run for 'foo' && git_cmd = 1: 'git'(!).

And one wants exec status for 'git-remote-foo', NOT for 'git'. Thus,
git_cmd must be 0 (at least without additional flags or flag values).
 
> For example, we might later want to use different $PATH only when running
> a git subcommand, and telling run_command() explicitly that we are running
> a git thing would help if you don't add "git-" to the command line and
> drop "proc->git_cmd = 1" in the caller like your patch does.

Well, that would require new flag (or git_cmd field value) to mean do direct
exec with gitexecdir in $PATH. Otherwise, you would either break this piece
of code, or it would be already broken (depending on value of git_cmd).

-Ilari