Re: [PATCH] start_command(), if .in/.out > 0, closes file descriptors, not the callers

Johannes Sixt [off-list ref] writes:

Callers of start_command() can set the members .in and .out of struct
child_process to a value > 0 to specify that this descriptor is used as
the stdin or stdout of the child process.

Previously, if start_command() was successful, this descriptor was closed
upon return. Here we now make sure that the descriptor is also closed in
case of failures. All callers are updated not to close the file descriptor
themselves after start_command() was called.

These two patches make the calling convention more uniform and
it feels like a good clean-up of the semantics.

But I have to wonder...

Why check for "2 or more"?

Could the caller have specified FD #1 (its own usual stdout) to
be used by the child?

Likewise, could the caller have told the child to use its own
stdin?

Here "in" is checked for "1 or more", unlike the above hunk...

Wouldn't this be better if 0, 1, or 2 specify inheriting
these respectively?

I am confused...

+	 * - Specify > 0 to give away a FD as follows:
+	 *     .in: a readable FD, becomes child's stdin
+	 *     .out: a writable FD, becomes child's stdout/stderr
+	 *     .err > 0 not supported
+	 *   The specified FD is closed by start_command(), even in case
+	 *   of errors!

Perhaps you would need to spell out the semantic differences you
are assigning to "inherit" vs "give away".  I presume the former
is something run_command() would not touch vs the latter is
closed by run_command()?

Junio C Hamano [off-list ref] wrote:

Johannes Sixt [off-list ref] writes:

These two patches make the calling convention more uniform and
it feels like a good clean-up of the semantics.

...

quoted

diff --git a/run-command.h b/run-command.h
index e9c84d0..0e67472 100644
--- a/run-command.h
+++ b/run-command.h

@@ -14,6 +14,23 @@ enum {
 struct child_process {
 	const char **argv;
 	pid_t pid;
+	/*
+	 * Using .in, .out, .err:
+	 * - Specify 0 to inherit stdin, stdout, stderr from parent.

Wouldn't this be better if 0, 1, or 2 specify inheriting
these respectively?

I am confused...

Using 0 to inherit stdin/stdout/stderr makes it easy for callers
of run_command() to setup something like this:

	struct child_process p;
	memset(&p, 0, sizeof(p));
	p.git_cmd = 1;
	p.argv = args;
	run_command(&p);

and have the child inherit stdin/stdout/stderr automatically,
much as a fork() automatically inherits those into the child
process.

So I agree with keeping 0 for inheriting, rather than needing to set
"p.in = 0; p.out = 1; p.err = 2".  We don't always need (or want)
the redirection service.

I think its OK (although maybe somewhat crazy) for the caller to
set "p.in = 1" and thus ask start_command() to close(1) before
it returns.

What happens if the parent has already closed fds 1 and 2 but then
opens a network socket somewhere and wants that socket to be stdin
of the child?  Its at fd 1.  But the parent doesn't want to track
that fd either, she wants it to go into the child and only the child.
So it seems reasonable to make this the "give away" case, and that's
what it always had been.

Of course the same argument can be said for fd 0; if the parent does
close(0) before some other open/socket/pipe call then its obviously
possible for the parent to get a fd that it wants the child to take
over and close.

We're basically assuming that the parent will always keep its
own stdin open if it will be spawning children.  We all know what
happens when we assume (we double close file descriptors) but I
think its a reasonable safe assumption to be making.
 
-- 
Shawn.

On Saturday 16 February 2008 23:55, Junio C Hamano wrote:

Johannes Sixt [off-list ref] writes:

quoted

 	need_in = !cmd->no_stdin && cmd->in < 0;
 	if (need_in) {
-		if (pipe(fdin) < 0)
+		if (pipe(fdin) < 0) {
+			if (cmd->out > 1)
+				close(cmd->out);

Why check for "2 or more"?

Later in the code, where we set up the redirections in the child process, we 
have this:
		...
		} else if (cmd->out > 1) {
			dup2(cmd->out, 1);
			close(cmd->out);
		}

and I thought it was good to also compare cmd->out > 1 in other situations. 
But now that I think about it, this > 1 is to be understood like this:

		if (cmd->out > 0)
			dup2(cmd->out, 1) unless cmd->out == 1;

so it's an optimization - an unnecessary one since dup2(1, 1) is supposed to 
succeed and be a noop.

quoted

+	 * - Specify > 0 to give away a FD as follows:
+	 *     .in: a readable FD, becomes child's stdin
+	 *     .out: a writable FD, becomes child's stdout/stderr
+	 *     .err > 0 not supported
+	 *   The specified FD is closed by start_command(), even in case
+	 *   of errors!

Perhaps you would need to spell out the semantic differences you
are assigning to "inherit" vs "give away".  I presume the former
is something run_command() would not touch vs the latter is
closed by run_command()?

I'll clearify it.

-- Hannes

Callers of start_command() can set the members .in and .out of struct
child_process to a value > 0 to specify that this descriptor is used as
the stdin or stdout of the child process.

Previously, if start_command() was successful, this descriptor was closed
upon return. Here we now make sure that the descriptor is also closed in
case of failures. All callers are updated not to close the file descriptor
themselves after start_command() was called.

Note that earlier run_gpg_verify() of git-verify-tag set .out = 1, which
worked because start_command() treated this as a special case, but now
this is incorrect because it closes the descriptor. The intent here is to
inherit stdout to the child, which is achieved by .out = 0.

Signed-off-by: Johannes Sixt <redacted>
---
On Sunday 17 February 2008 10:29, Johannes Sixt wrote:

On Saturday 16 February 2008 23:55, Junio C Hamano wrote:

quoted

Johannes Sixt [off-list ref] writes:

quoted

+	 * - Specify > 0 to give away a FD as follows:
+	 *     .in: a readable FD, becomes child's stdin
+	 *     .out: a writable FD, becomes child's stdout/stderr
+	 *     .err > 0 not supported
+	 *   The specified FD is closed by start_command(), even in case
+	 *   of errors!

Perhaps you would need to spell out the semantic differences you
are assigning to "inherit" vs "give away".  I presume the former
is something run_command() would not touch vs the latter is
closed by run_command()?

I'll clearify it.

And here it is.

The other issues you raised -- ->out sometimes compared > 0, sometimes > 1,
and treated different from ->in -- I addressed, too, by adjusting all uses
that were inconsitent. Note that there is still one ->out > 0, but that is
because we don't have set up need_out at this time, and I also left the
cmd->out > 1 in the child process alone.

Note also the amended commit message that describes the change in
builtin-verify-tag.c.

-- Hannes

 builtin-send-pack.c  |   14 ++++++++------
 builtin-verify-tag.c |    1 -
 bundle.c             |    5 +++--
 run-command.c        |   22 ++++++++++++++++++++--
 run-command.h        |   18 ++++++++++++++++++
 5 files changed, 49 insertions(+), 11 deletions(-)

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index ba9bc91..b0cfae8 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c

@@ -404,12 +404,15 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 	if (!remote_tail)
 		remote_tail = &remote_refs;
 	if (match_refs(local_refs, remote_refs, &remote_tail,
-					       nr_refspec, refspec, flags))
+		       nr_refspec, refspec, flags)) {
+		close(out);
 		return -1;
+	}
 
 	if (!remote_refs) {
 		fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
 			"Perhaps you should specify a branch such as 'master'.\n");
+		close(out);
 		return 0;
 	}
 

@@ -496,12 +499,11 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 
 	packet_flush(out);
 	if (new_refs && !args.dry_run) {
-		if (pack_objects(out, remote_refs) < 0) {
-			close(out);
+		if (pack_objects(out, remote_refs) < 0)
 			return -1;
-		}
 	}
-	close(out);
+	else
+		close(out);
 
 	if (expect_status_report)
 		ret = receive_status(in, remote_refs);

@@ -649,7 +651,7 @@ int send_pack(struct send_pack_args *my_args,
 	conn = git_connect(fd, dest, args.receivepack, args.verbose ? CONNECT_VERBOSE : 0);
 	ret = do_send_pack(fd[0], fd[1], remote, dest, nr_heads, heads);
 	close(fd[0]);
-	close(fd[1]);
+	/* do_send_pack always closes fd[1] */
 	ret |= finish_connect(conn);
 	return !!ret;
 }

diff --git a/builtin-verify-tag.c b/builtin-verify-tag.c
index b3010f9..f3ef11f 100644
--- a/builtin-verify-tag.c
+++ b/builtin-verify-tag.c

@@ -45,7 +45,6 @@ static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
 	memset(&gpg, 0, sizeof(gpg));
 	gpg.argv = args_gpg;
 	gpg.in = -1;
-	gpg.out = 1;
 	args_gpg[2] = path;
 	if (start_command(&gpg))
 		return error("could not run gpg.");

diff --git a/bundle.c b/bundle.c
index 4352ce8..0ba5df1 100644
--- a/bundle.c
+++ b/bundle.c

@@ -336,8 +336,9 @@ int create_bundle(struct bundle_header *header, const char *path,
 	close(rls.in);
 	if (finish_command(&rls))
 		return error ("pack-objects died");
-
-	return bundle_to_stdout ? close(bundle_fd) : commit_lock_file(&lock);
+	if (!bundle_to_stdout)
+		commit_lock_file(&lock);
+	return 0;
 }
 
 int unbundle(struct bundle_header *header, int bundle_fd)

diff --git a/run-command.c b/run-command.c
index 2919330..743757c 100644
--- a/run-command.c
+++ b/run-command.c

@@ -20,10 +20,18 @@ int start_command(struct child_process *cmd)
 	int need_in, need_out, need_err;
 	int fdin[2], fdout[2], fderr[2];
 
+	/*
+	 * In case of errors we must keep the promise to close FDs
+	 * that have been passed in via ->in and ->out.
+	 */
+
 	need_in = !cmd->no_stdin && cmd->in < 0;
 	if (need_in) {
-		if (pipe(fdin) < 0)
+		if (pipe(fdin) < 0) {
+			if (cmd->out > 0)
+				close(cmd->out);
 			return -ERR_RUN_COMMAND_PIPE;
+		}
 		cmd->in = fdin[1];
 	}
 

@@ -34,6 +42,8 @@ int start_command(struct child_process *cmd)
 		if (pipe(fdout) < 0) {
 			if (need_in)
 				close_pair(fdin);
+			else if (cmd->in)
+				close(cmd->in);
 			return -ERR_RUN_COMMAND_PIPE;
 		}
 		cmd->out = fdout[0];

@@ -44,8 +54,12 @@ int start_command(struct child_process *cmd)
 		if (pipe(fderr) < 0) {
 			if (need_in)
 				close_pair(fdin);
+			else if (cmd->in)
+				close(cmd->in);
 			if (need_out)
 				close_pair(fdout);
+			else if (cmd->out)
+				close(cmd->out);
 			return -ERR_RUN_COMMAND_PIPE;
 		}
 		cmd->err = fderr[0];

@@ -55,8 +69,12 @@ int start_command(struct child_process *cmd)
 	if (cmd->pid < 0) {
 		if (need_in)
 			close_pair(fdin);
+		else if (cmd->in)
+			close(cmd->in);
 		if (need_out)
 			close_pair(fdout);
+		else if (cmd->out)
+			close(cmd->out);
 		if (need_err)
 			close_pair(fderr);
 		return -ERR_RUN_COMMAND_FORK;

@@ -118,7 +136,7 @@ int start_command(struct child_process *cmd)
 
 	if (need_out)
 		close(fdout[1]);
-	else if (cmd->out > 1)
+	else if (cmd->out)
 		close(cmd->out);
 
 	if (need_err)

diff --git a/run-command.h b/run-command.h
index e9c84d0..debe307 100644
--- a/run-command.h
+++ b/run-command.h

@@ -14,6 +14,24 @@ enum {
 struct child_process {
 	const char **argv;
 	pid_t pid;
+	/*
+	 * Using .in, .out, .err:
+	 * - Specify 0 for no redirections (child inherits stdin, stdout,
+	 *   stderr from parent).
+	 * - Specify -1 to have a pipe allocated as follows:
+	 *     .in: returns the writable pipe end; parent writes to it,
+	 *          the readable pipe end becomes child's stdin
+	 *     .out, .err: returns the readable pipe end; parent reads from
+	 *          it, the writable pipe end becomes child's stdout/stderr
+	 *   The caller of start_command() must close the returned FDs
+	 *   after it has completed reading from/writing to it!
+	 * - Specify > 0 to set a channel to a particular FD as follows:
+	 *     .in: a readable FD, becomes child's stdin
+	 *     .out: a writable FD, becomes child's stdout/stderr
+	 *     .err > 0 not supported
+	 *   The specified FD is closed by start_command(), even in case
+	 *   of errors!
+	 */
 	int in;
 	int out;
 	int err;

-- 
1.5.4.1.126.ge5a7d

Thanks, will queue.