Johannes Sixt <johannes.sixt@xxxxxxxxxx> 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()?
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
