Re: [PATCHv2 1/2] build: avoid close, system

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 01/29/2011 04:40 AM, Matthias Bolte wrote:
>>
>> -    VIR_FORCE_CLOSE(infd);
>> +    if (infd != STDIN_FILENO)
>> +        VIR_FORCE_CLOSE(infd);
>>     VIR_FORCE_CLOSE(null);
>> -    tmpfd = childout;   /* preserve childout value */
>> -    VIR_FORCE_CLOSE(tmpfd);
>> -    if (childerr > 0 &&
>> +    if (childout != STDOUT_FILENO) {
>> +        tmpfd = childout;   /* preserve childout value */
>> +        VIR_FORCE_CLOSE(tmpfd);
> 
> Took me a moment to understand this. I think in case you pass parent's
> std{in,out,err} to child's std{in,out,err} this works. But when you
> exchange stdout and stderr like this: parent std{in,err,out} -> child
> std{in,out,err}, then childout != STDOUT_FILENO is wrong and should be
> childout > STDERR_FILENO, otherwise you could close the parent's
> stderr here.

Okay, I made that change (I doubt that anyone will use virExec to swap
out/err from the parent to err/out in the child, but we might as well
make the change for robustness sake).

> 
>> +    }
>> +    if (childerr > STDERR_FILENO &&
>>         childerr != childout) {
>>         VIR_FORCE_CLOSE(childerr);
>> -        childout = -1;
> 
> Technically it's okay to remove this like as childout isn't accessed
> afterwards anymore. But by using the tmpfd variable we tricked
> VIR_FORCE_CLOSE and should finish it's job here.

But if childout > STDERR_FILENO, while childerr = 2, then this branch of
code would not be reached anyway.  I don't see any reason to set
childout to -1; the only reason that line was added in the first place
was due to the search-and-replace nature of converting all close() to
VIR_FORCE_CLOSE(), when tmpfd was introduced in the first place so as
not to invalidate the later comparison of childerr != childout.  You are
right that childout isn't accessed later, so there's no risk of a
double-close() bug.

> 
> ACK, with that comments addressed.

I've pushed this series now.

-- 
Eric Blake   eblake@xxxxxxxxxx    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]