[libvirt] [PATCH] qemu: Report errors from iohelper

[Eric Blake]

On 10/23/2012 03:39 AM, Michal Privoznik wrote:
> Currently, we use iohelper when saving/restoring a domain.
> However, if there's some kind of error (like I/O) it is not
> propagated to libvirt. Since it is not qemu who is doing
> the actual write() it will not get error. The iohelper does.
> Therefore we should check for iohelper errors as it makes
> libvirt more user friendly.
> ---
>  src/libvirt_private.syms |    1 +
>  src/qemu/qemu_driver.c   |    7 ++++-
>  src/util/virfile.c       |   47 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/util/virfile.h       |    2 +
>  4 files changed, 55 insertions(+), 2 deletions(-)

I agree that catching iohelper errors is needed, but I'm worried that
you might introduce deadlock with this approach (then again, it may just
be theoretical):

> +++ b/src/qemu/qemu_driver.c
> @@ -2945,6 +2945,7 @@ endjob:
>                  if (rc < 0)
>                      VIR_WARN("Unable to resume guest CPUs after save failure");
>              }
> +            virFileWrapperFdCatchError(wrapperFd);
>          }
>          if (qemuDomainObjEndAsyncJob(driver, vm) == 0)
>              vm = NULL;
> @@ -3282,9 +3283,11 @@ doCoreDump(struct qemud_driver *driver,
>  
>  cleanup:
>      VIR_FORCE_CLOSE(fd);
> -    virFileWrapperFdFree(wrapperFd);
> -    if (ret != 0)
> +    if (ret != 0) {
> +        virFileWrapperFdCatchError(wrapperFd);
>          unlink(path);

You aren't doing any reading of the stderr file descriptor until you get
to the cleanup label.  But suppose that iohelper encounters situations
that cause it to write more than PIPE_BUF bytes to stderr, but while
still continuing to run.  Then, because no one is reading the other end
of the pipe, iohelper blocks on writes to stderr instead of proceeding
on with the rest of its operations, and thus never gets to a point where
it will exit in order to kick libvirtd over to the cleanup label to
start reading the stderr pipe in the first place.

> +++ b/src/util/virfile.c
> @@ -135,6 +135,7 @@ virFileDirectFdFlag(void)
>   * read-write is not supported, just a single direction.  */
>  struct _virFileWrapperFd {
>      virCommandPtr cmd; /* Child iohelper process to do the I/O.  */
> +    int err_fd; /* FD to read stderr of @cmd */
>  };
>  
>  #ifndef WIN32
> @@ -229,6 +230,14 @@ virFileWrapperFdNew(int *fd, const char *name, unsigned int flags)
>          virCommandAddArg(ret->cmd, "0");
>      }
>  
> +    /* In order to catch iohelper stderr, we must:
> +     * - pass a FD to virCommand (-1 to auto-allocate one)
> +     * - change iohelper's env so virLog functions print to stderr
> +     */
> +    ret->err_fd = -1;
> +    virCommandSetErrorFD(ret->cmd, &ret->err_fd);
> +    virCommandAddEnvPair(ret->cmd, "LIBVIRT_LOG_OUTPUTS", "1:stderr");

Would it work to use virCommandSetErrorBuffer() instead, or does that
rely on using virCommandRun() to properly populate the buffer via
poll(), where we are stuck using virCommandRunAsync() instead?  Or,
since we are already using virCommandRunAsync(), does that mean we need
to figure out how to incorporate a poll() to favorably handle stderr in
addition to everything else that iohelper is already  handling?

Or am I worrying too much, and if iohelper hits any issue where it would
be logging to stderr in the first place, it will be exiting before
filling up the stderr pipe and thus never deadlocking?

At any rate, depending on how that question is answered, this code does
indeed look useful.

-- 
Eric Blake   eblake at 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: 617 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20121023/fbd812e3/attachment-0001.sig>