[PATCH 2/6] Automatically close stderr pipes created by run_command

[PATCH 2/6] Automatically close stderr pipes created by run_command

[Shawn O. Pearce]

Like the out pipe and in pipe, we now automatically close the err
pipe if it was requested by the caller and it hasn't been closed
by the caller.  This simplifies anyone who wants to get a pipe to
the stderr stream for another process.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 run-command.c |    3 +++
 run-command.h |    1 +
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/run-command.c b/run-command.c
index 52f80be..7bf2cd7 100644
--- a/run-command.c
+++ b/run-command.c
@@ -51,6 +51,7 @@ int start_command(struct child_process *cmd)
 			return -ERR_RUN_COMMAND_PIPE;
 		}
 		cmd->err = fderr[0];
+		cmd->close_err = 1;
 	}
 
 	cmd->pid = fork();
@@ -161,6 +162,8 @@ int finish_command(struct child_process *cmd)
 		close(cmd->in);
 	if (cmd->close_out)
 		close(cmd->out);
+	if (cmd->close_err)
+		close(cmd->err);
 	return wait_or_whine(cmd->pid);
 }
 
diff --git a/run-command.h b/run-command.h
index 1fc781d..705cf2f 100644
--- a/run-command.h
+++ b/run-command.h
@@ -21,6 +21,7 @@ struct child_process {
 	const char *const *env;
 	unsigned close_in:1;
 	unsigned close_out:1;
+	unsigned close_err:1;
 	unsigned no_stdin:1;
 	unsigned no_stdout:1;
 	unsigned no_stderr:1;
-- 
1.5.4.1.1309.g833c2

Re: [PATCH 2/6] Automatically close stderr pipes created by run_command

[Johannes Sixt]

Shawn O. Pearce schrieb:
> Like the out pipe and in pipe, we now automatically close the err
> pipe if it was requested by the caller and it hasn't been closed
> by the caller.  This simplifies anyone who wants to get a pipe to
> the stderr stream for another process.

IMHO, this is backwards.

The .in, .out, .err members of struct child_process serve two different
purposes:

1. Caller sets them to some fd > 0. This means:
   "Here is a readable (.in) or writable (.out, .err) fd; use it."

2. Caller sets them to -1: This means:
   "Create a pipe and give me the writable (.in) or readable (.out,
   .err) end of it back."

Notice that in a clean implementation:

- case 1. would imply that the fd is "given away", i.e.
  start_command/finish_command take ownership and close it;

- case 2. would imply that the caller takes ownership of the returned
  fd and has to close it.

The current implementation of start_command/finish_command as well as its
callers don't follow these rules (because they are not documented
anywhere). As a nasty side-effect we have double-closes in many places
because some callers close fds even though they are not supposed to do it.
(That's the reason why the close() calls in finish_command cannot check
for errors!)

I've a patch cooking that cleans up this mess. IMHO, your patch makes
things messier.

-- Hannes

Re: [PATCH 2/6] Automatically close stderr pipes created by run_command

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Shawn O. Pearce schrieb:
>> Like the out pipe and in pipe, we now automatically close the err
>> pipe if it was requested by the caller and it hasn't been closed
>> by the caller.  This simplifies anyone who wants to get a pipe to
>> the stderr stream for another process.
>
> IMHO, this is backwards.
>
> The .in, .out, .err members of struct child_process serve two different
> purposes:
>
> 1. Caller sets them to some fd > 0. This means:
>    "Here is a readable (.in) or writable (.out, .err) fd; use it."
>
> 2. Caller sets them to -1: This means:
>    "Create a pipe and give me the writable (.in) or readable (.out,
>    .err) end of it back."
>
> Notice that in a clean implementation:
>
> - case 1. would imply that the fd is "given away", i.e.
>   start_command/finish_command take ownership and close it;
>
> - case 2. would imply that the caller takes ownership of the returned
>   fd and has to close it.

I am puzzled.  In a clean implementation perhaps the fds belong
to the caller in both cases.  Or belong to the callee in both
cases.  Or perhaps sometimes belong to the caller and someimes
to the callee as you wrote above, but that seems arbitrary.

I suspect from the caller's point of view, if we can always make
them owned by the caller, that would be the most consistent and
convenient.  "Here is a fd --- I lend it to you so use it but do
not close --- I may have other uses for it later".

In any case, I agree with you that the current code lacks
consistency either way and we need an order there.

Re: [PATCH 2/6] Automatically close stderr pipes created by run_command

[Johannes Sixt]

On Friday 15 February 2008 18:11, Junio C Hamano wrote:
> Johannes Sixt <j.sixt@viscovery.net> writes:
> > Shawn O. Pearce schrieb:
> >> Like the out pipe and in pipe, we now automatically close the err
> >> pipe if it was requested by the caller and it hasn't been closed
> >> by the caller.  This simplifies anyone who wants to get a pipe to
> >> the stderr stream for another process.
> >
> > IMHO, this is backwards.
> >
> > The .in, .out, .err members of struct child_process serve two different
> > purposes:
> >
> > 1. Caller sets them to some fd > 0. This means:
> >    "Here is a readable (.in) or writable (.out, .err) fd; use it."
> >
> > 2. Caller sets them to -1: This means:
> >    "Create a pipe and give me the writable (.in) or readable (.out,
> >    .err) end of it back."
> >
> > Notice that in a clean implementation:
> >
> > - case 1. would imply that the fd is "given away", i.e.
> >   start_command/finish_command take ownership and close it;
> >
> > - case 2. would imply that the caller takes ownership of the returned
> >   fd and has to close it.
>
> I am puzzled.  In a clean implementation perhaps the fds belong
> to the caller in both cases.  Or belong to the callee in both
> cases.  Or perhaps sometimes belong to the caller and someimes
> to the callee as you wrote above, but that seems arbitrary.

Heh. In a clean implementation you also don't use the same thing to do two 
completely different things. For example, note how in one case .in contains a 
readable, in the other a writable fd, and how fds are used in completely 
different ways.

> I suspect from the caller's point of view, if we can always make
> them owned by the caller, that would be the most consistent and
> convenient.  "Here is a fd --- I lend it to you so use it but do
> not close --- I may have other uses for it later".

Well, I think we could do that, theoretically. But practically in the case 
where a fd > 0 is assigned to .in/.out/.err:

- case .out, .err: the caller is required to close the fd early after 
start_command() because (if this is a pipe) the child won't see EOF;

- case .in: the caller must not read from the fd anyway, else the child gets 
inconsistent input.

So, while there *is* some inconsistency, the inconsistent cases can be clearly 
separated into the cases fd > 0 and fd == -1.

-- Hannes

Re: [PATCH 2/6] Automatically close stderr pipes created by run_command

[Junio C Hamano]

Johannes Sixt <johannes.sixt@telecom.at> writes:

> ... But practically in the case 
> where a fd > 0 is assigned to .in/.out/.err:
>
> - case .out, .err: the caller is required to close the fd early after 
> start_command() because (if this is a pipe) the child won't see EOF;
>
> - case .in: the caller must not read from the fd anyway, else the child gets 
> inconsistent input.
>
> So, while there *is* some inconsistency, the inconsistent cases can be clearly 
> separated into the cases fd > 0 and fd == -1.

Ok, sold.

[PATCH] start_command(), .in/.out/.err = -1: Callers must close the file descriptor

[Johannes Sixt]

By setting .in, .out, or .err members of struct child_process to -1, the
callers of start_command() can request that a pipe is allocated that talks
to the child process and one end is returned by replacing -1 with the
file descriptor.

Previously, a flag was set (for .in and .out, but not .err) to signal
finish_command() to close the pipe end that start_command() had handed out,
so it was optional for callers to close the pipe, and many already do so.
Now we make it mandatory to close the pipe.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 builtin-fetch-pack.c |    4 +++-
 builtin-send-pack.c  |    1 +
 builtin-tag.c        |    3 ++-
 builtin-verify-tag.c |    1 -
 bundle.c             |    1 +
 receive-pack.c       |    2 ++
 run-command.c        |    6 ------
 run-command.h        |    2 --
 8 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index f401352..5ea48ca 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -538,8 +538,10 @@ static int get_pack(int xd[2], char **pack_lockfile)
 	cmd.git_cmd = 1;
 	if (start_command(&cmd))
 		die("fetch-pack: unable to fork off %s", argv[0]);
-	if (do_keep && pack_lockfile)
+	if (do_keep && pack_lockfile) {
 		*pack_lockfile = index_pack_lockfile(cmd.out);
+		close(cmd.out);
+	}
 
 	if (finish_command(&cmd))
 		die("%s failed", argv[0]);
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 8afb1d0..ba9bc91 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -71,6 +71,7 @@ static int pack_objects(int fd, struct ref *refs)
 		refs = refs->next;
 	}
 
+	close(po.in);
 	if (finish_command(&po))
 		return error("pack-objects died with strange error");
 	return 0;
diff --git a/builtin-tag.c b/builtin-tag.c
index 4a4a88c..8f7936d 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -226,12 +226,13 @@ static int do_sign(struct strbuf *buffer)
 
 	if (write_in_full(gpg.in, buffer->buf, buffer->len) != buffer->len) {
 		close(gpg.in);
+		close(gpg.out);
 		finish_command(&gpg);
 		return error("gpg did not accept the tag data");
 	}
 	close(gpg.in);
-	gpg.close_in = 0;
 	len = strbuf_read(buffer, gpg.out, 1024);
+	close(gpg.out);
 
 	if (finish_command(&gpg) || !len || len < 0)
 		return error("gpg failed to sign the tag");
diff --git a/builtin-verify-tag.c b/builtin-verify-tag.c
index cc4c55d..b3010f9 100644
--- a/builtin-verify-tag.c
+++ b/builtin-verify-tag.c
@@ -52,7 +52,6 @@ static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
 
 	write_in_full(gpg.in, buf, len);
 	close(gpg.in);
-	gpg.close_in = 0;
 	ret = finish_command(&gpg);
 
 	unlink(path);
diff --git a/bundle.c b/bundle.c
index 5c95eca..f150c19 100644
--- a/bundle.c
+++ b/bundle.c
@@ -332,6 +332,7 @@ int create_bundle(struct bundle_header *header, const char *path,
 		write_or_die(rls.in, sha1_to_hex(object->sha1), 40);
 		write_or_die(rls.in, "\n", 1);
 	}
+	close(rls.in);
 	if (finish_command(&rls))
 		return error ("pack-objects died");
 
diff --git a/receive-pack.c b/receive-pack.c
index 3267495..a971433 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -132,6 +132,7 @@ static int run_hook(const char *hook_name)
 				break;
 		}
 	}
+	close(proc.in);
 	return hook_status(finish_command(&proc), hook_name);
 }
 
@@ -414,6 +415,7 @@ static const char *unpack(void)
 		if (start_command(&ip))
 			return "index-pack fork failed";
 		pack_lockfile = index_pack_lockfile(ip.out);
+		close(ip.out);
 		status = finish_command(&ip);
 		if (!status) {
 			reprepare_packed_git();
diff --git a/run-command.c b/run-command.c
index 476d00c..2919330 100644
--- a/run-command.c
+++ b/run-command.c
@@ -25,7 +25,6 @@ int start_command(struct child_process *cmd)
 		if (pipe(fdin) < 0)
 			return -ERR_RUN_COMMAND_PIPE;
 		cmd->in = fdin[1];
-		cmd->close_in = 1;
 	}
 
 	need_out = !cmd->no_stdout
@@ -38,7 +37,6 @@ int start_command(struct child_process *cmd)
 			return -ERR_RUN_COMMAND_PIPE;
 		}
 		cmd->out = fdout[0];
-		cmd->close_out = 1;
 	}
 
 	need_err = !cmd->no_stderr && cmd->err < 0;
@@ -157,10 +155,6 @@ static int wait_or_whine(pid_t pid)
 
 int finish_command(struct child_process *cmd)
 {
-	if (cmd->close_in)
-		close(cmd->in);
-	if (cmd->close_out)
-		close(cmd->out);
 	return wait_or_whine(cmd->pid);
 }
 
diff --git a/run-command.h b/run-command.h
index 1fc781d..e9c84d0 100644
--- a/run-command.h
+++ b/run-command.h
@@ -19,8 +19,6 @@ struct child_process {
 	int err;
 	const char *dir;
 	const char *const *env;
-	unsigned close_in:1;
-	unsigned close_out:1;
 	unsigned no_stdin:1;
 	unsigned no_stdout:1;
 	unsigned no_stderr:1;
-- 
1.5.4.1.126.ge5a7d

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

[Johannes Sixt]

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.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 builtin-send-pack.c  |   14 ++++++++------
 builtin-verify-tag.c |    1 -
 bundle.c             |    5 +++--
 run-command.c        |   20 +++++++++++++++++++-
 run-command.h        |   17 +++++++++++++++++
 5 files changed, 47 insertions(+), 10 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 f150c19..eac7350 100644
--- a/bundle.c
+++ b/bundle.c
@@ -335,8 +335,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..6c35d3c 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 in ->in, ->out, and ->err.
+	 */
+
 	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);
 			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 > 1)
+				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 > 0)
+			close(cmd->in);
 		if (need_out)
 			close_pair(fdout);
+		else if (cmd->out > 1)
+			close(cmd->out);
 		if (need_err)
 			close_pair(fderr);
 		return -ERR_RUN_COMMAND_FORK;
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.
+	 * - 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 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!
+	 */
 	int in;
 	int out;
 	int err;
-- 
1.5.4.1.126.ge5a7d

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

[Junio C Hamano]

Johannes Sixt <johannes.sixt@telecom.at> 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...

> diff --git a/run-command.c b/run-command.c
> index 2919330..6c35d3c 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 in ->in, ->out, and ->err.
> +	 */
> +
>  	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"?

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

> @@ -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);

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

> @@ -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 > 0)
> +			close(cmd->in);

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

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

> +	 * - 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()?

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

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Sixt <johannes.sixt@telecom.at> writes:
> 
> These two patches make the calling convention more uniform and
> it feels like a good clean-up of the semantics.
...
> > 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.

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

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

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

Ok, that unwritten assumption was what confused me.  We
definitely need Documentation/technical/api-*.txt before any
more code, don't you agree?

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

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> > 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.
> 
> Ok, that unwritten assumption was what confused me.  We
> definitely need Documentation/technical/api-*.txt before any
> more code, don't you agree?

Yes.

I'm far too tired to write it tonight.  Maybe I will find the
time and energy tomorrow.  Depends on how it goes at day-job
(yes, must go back for yet another day this weekend).

But this API is messy enough that we need explicit rules of
how it should be used, and then make everyone match that.

Unless Johannes Sixt beats me to it, my next patch to you will
be for a proposed documentation improvement.

-- 
Shawn.

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

[Johannes Sixt]

On Saturday 16 February 2008 23:55, Junio C Hamano wrote:
> Johannes Sixt <johannes.sixt@telecom.at> writes:
> >  	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.

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

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

[Johannes Sixt]

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 <johannes.sixt@telecom.at>
---
On Sunday 17 February 2008 10:29, Johannes Sixt wrote:
> On Saturday 16 February 2008 23:55, Junio C Hamano wrote:
> > Johannes Sixt <johannes.sixt@telecom.at> writes:
> > > +	 * - 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

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

[Junio C Hamano]

Thanks, will queue.

Re: [PATCH 2/6] Automatically close stderr pipes created by run_command

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Shawn O. Pearce schrieb:
>> Like the out pipe and in pipe, we now automatically close the err
>> pipe if it was requested by the caller and it hasn't been closed
>> by the caller.  This simplifies anyone who wants to get a pipe to
>> the stderr stream for another process.
>
> IMHO, this is backwards.
>
> The .in, .out, .err members of struct child_process serve two different
> purposes:
> ...
> Notice that in a clean implementation:
> ...
> The current implementation of start_command/finish_command as well as its
> callers don't follow these rules (because they are not documented
> anywhere).

We should define what the clean semantics of this API should be,
and update Documentation/technical/api-run-command.txt, before
going any further.

And I happen to notice two of your names in the list of people
who have been involved in its evolution ;-).