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