Re: [PATCH] start_command(), if .in/.out > 0, closes file descriptors, not the callers
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 16, 2008, 22:55 UTC
- Message-ID
- <7vmyq07bqe.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <1203183399-4813-2-git-send-email-johannes.sixt@telecom.at>
Johannes Sixt <johannes.sixt@telecom.at> writes:
Show 8 quoted lines
> 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...
Show 19 quoted lines
> 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?
Show 6 quoted lines
> @@ -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?
Show 6 quoted lines
> @@ -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...
Show 11 quoted lines
> 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...
Show 6 quoted lines
> + * - 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()?