Re: [PATCH v2 properly indented] fix start_command() bug when stdin is closed
- From
Johannes Sixt <j.sixt@viscovery.net>
- Date
- Aug 26, 2008, 06:33 UTC
- Message-ID
- <48B3A3CC.3060906@viscovery.net>
- In-Reply-To
- <7vbpzgb94q.fsf@gitster.siamese.dyndns.org>
Junio C Hamano schrieb:
Show 27 quoted lines
> Paolo Bonzini <bonzini@gnu.org> 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