Re: [PATCH v2 4/6] run-command: don't die in child when duping /dev/null

From: Brandon Williams <bmwill@google.com>
Date: Apr 13, 2017, 19:43 UTC
Message-ID: <20170413194317.GG96917@google.com>
In-Reply-To: <20170413192928.GA6129@whir>

On 04/13, Eric Wong wrote:

Show 13 quoted lines

> Brandon Williams <bmwill@google.com> wrote:
> > @@ -487,7 +483,7 @@ int start_command(struct child_process *cmd)
> >  		atexit(notify_parent);
> >  
> >  		if (cmd->no_stdin)
> > -			dup_devnull(0);
> > +			dup2(null_fd, 0);
> 
> I prefer we keep error checking for dup2 failures,
> and also add more error checking for unchecked dup2 calls.
> Can be a separate patch, I suppose.
> 
> Ditto for other dup2 changes

I simply figured that since we weren't doing the checks on the other dup2 calls that I would keep it consistent. But if we wanted to add in more error checking then we can can in another patch.

Show 13 quoted lines

> 
> > @@ -558,6 +554,8 @@ int start_command(struct child_process *cmd)
> >  	}
> >  	close(notify_pipe[0]);
> >  
> > +	if (null_fd > 0)
> > +		close(null_fd);
> 
> I would prefer:
> 
> 	if (null_fd >= 0)
> 
> here, even if we currently do not release stdin.

K will do.

> 
> >  	argv_array_clear(&argv);
> >  	free(childenv);

-- 
Brandon Williams