Re: [libvirt] [PATCHv2 1/2] build: avoid close, system
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(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 619 bytes)