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@xxxxxxxxxx>
> ---
> 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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list