[libvirt] [PATCH 4/7] virfile: Adjust error path for virFileOpenForked

[Eric Blake]

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 at 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150129/14883fd4/attachment-0001.sig>