Re: [PATCH v2 properly indented] fix start_command() bug when stdin is closed

[Johannes Sixt <j.sixt@xxxxxxxxxxxxx>]

Junio C Hamano schrieb:
> Paolo Bonzini <bonzini@xxxxxxx> writes:
> 
>>  int start_command(struct child_process *cmd)
>>  {
>>  	int need_in, need_out, need_err;
>>  	int fdin[2], fdout[2], fderr[2];
>>  
>>  	/*
>> +	 * Make sure that all file descriptors <= 2 are open, otherwise we
>> +	 * mess them up when dup'ing pipes onto stdin/stdout/stderr.  Since
>> +	 * we are at it, save a file descriptor on /dev/null to use it later.
>> +	 */
>> +	if (devnull_fd == -1) {
>> +		devnull_fd = open("/dev/null", O_RDWR);
>> +		while (devnull_fd >= 0 && devnull_fd <= 2)
>> +			devnull_fd = dup(devnull_fd);
>> +		if (devnull_fd == -1)
>> +			die("opening /dev/null failed (%s)", strerror(errno));
>> +	}
>> +
> 
> I may be misreading the patch but, this logic always opens /dev/null, if
> nobody asked for *any* cmd->no_stdXXX and low 3 fds are occupied, and
> worse, it keeps fd=3 open.
> 
> Making sure low fds 0, 1 and 2 are open is a good thing.  I do not think
> clobbering fd=3 is good.

It is sometimes _unnecessary_, but I don't see why it should hurt. The
effect on performance will be in the noise.

> Also shouldn't this be done only on the side that dup()s fds around,
> i.e. in the child process after fork()?  Why is this done for the parent?

Because it must be done *before* the pipe()s are created so that they
don't occupy fds 0-2.

-- Hannes

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