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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org