On 01/28/2015 03:25 PM, John Ferlan wrote:
Rather than have a dummy waitpid loop and return of the failure status from recvfd, adjust the logic to save the recvfd error & fd and then in priority order:
if waitpid failed, use that if waitpid succeeded, but the child reported failure, use that regardless of recvfd status if waitpid succeeded and reported success, but recvfd failed with anything other than EACCES or ENOTCONN, use recvfd's result otherwise, waitpid and recvfd succeeded, return the fd
NOTE: Original logic to retry the open and force owner mode was "documented" as only being attempted if we had already tried opening with the fork+setuid, but checked flags vs. VIR_FILE_OPEN_NOFORK which is counter to how we would get to that point. So that code was removed.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virfile.c | 67 +++++++++++++++++++++++++++++++----------------------- 1 file changed, 39 insertions(+), 28 deletions(-)
+ /* + * if waitpid succeeded, but the child reported failure, use that + * regardless of recvfd status + */ + if (WIFEXITED(status) && WEXITSTATUS(status) != 0) { + ret = WEXITSTATUS(status); + virReportSystemError(ret,
waitpid reporting !WIFEXITED(status) is a case of the child reporting failure (well, reporting abnormal exit due to signal), so it might simplify things if we just do[1]: if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) { except that then you have to distinguish between abnormal exit vs. exit encoding a known errno value when constructing the error message.
+ + /* if waitpid succeeded and reported success, but recvfd failed with + * anything other than EACCES or ENOTCONN, use recvfd's result + */ + if (WIFEXITED(status) && WEXITSTATUS(status) == 0 && fd < 0 && + !(recvfd_errno == EACCES || recvfd_errno == ENOTCONN)) { + virReportSystemError(recvfd_errno, + _("failed recvfd for child creating '%s'"), + path); + return -recvfd_errno; + }
Hmm. It may be sufficient to just treat ALL recvfd failures as the exit status if we get here, since if recvfd failed, then fd == -1...
+ + /* Some sort of abnormal child termination */ + if (!WIFEXITED(status) || fd == -1) { + VIR_FORCE_CLOSE(fd); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("child exited abnormally, failed to create '%s'"), + path); + return -EACCES;
...and we would be failing anyways, but losing the errno value from recvfd. And if you change point [1] above to catch abnormal exit, then this entire if branch becomes dead. So how about: - if waitpid failed, use that errno value - waitpid succeeded, but if the child exited abnormally, report failure (not sure what errno value to use here) - waitpid succeeded, but if the child reported non-zero status, report failure (use the errno value that the child encoded into exit status) - waitpid succeeded, but if recvfd failed, report recvfd_errno - waitpid and recvfd succeeded, use the fd Sorry for making you churn on this, given my first-round off-list thoughts on the matter, but hopefully it turns out a little easier to read. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org