[libvirt] [PATCHv2 1/2] build: avoid close, system
Eric Blake
eblake at redhat.com
Sat Jan 29 17:41:03 UTC 2011
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 at redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110129/235c5f31/attachment-0001.sig>
More information about the libvir-list
mailing list