[libvirt] [PATCH] qemu: Refresh state before starting the VCPUs

[Daniel P. Berrangé]

On Mon, Feb 04, 2019 at 01:36:24PM +0100, Marc Hartmayer wrote:
> For normal starts (no incoming migration) the refresh of the QEMU
> state must be done before the VCPUs getting started since otherwise
> there might be a race condition between a possible shutdown of the
> guest OS and the QEMU monitor queries.
> 
> This fixes "qemu: migration: Refresh device information after
> transferring state" (93db7eea1b864).

Hmm, this is the second bug that this patch has been responsible for
now

It has also resulted in a  deadlock in libguestfs during startup
when there is a chardev in blocking connect mode in QEMU. If the
guest writes data to the chardev before it is accepted by the
server libvirt can deadlock in startup as QEMU is blocked and
won't respond to QMP.  The core problem is that the startup code
must not rely on being able to use QMP /after/ the vCPUs have
been started untill the virDomainCreate API has completed

  https://bugzilla.redhat.com/show_bug.cgi?id=1661940

IMHO we need to just revert that original broken commit entirely
and find a different way to fix the problem it was attacking.

> 
> Signed-off-by: Marc Hartmayer <mhartmay at linux.ibm.com>
> ---
>  src/qemu/qemu_process.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index dace5aaca102..2a3763f40d49 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -6929,10 +6929,17 @@ qemuProcessStart(virConnectPtr conn,
>      }
>      relabel = true;
>  
> -    if (incoming &&
> -        incoming->deferredURI &&
> -        qemuMigrationDstRun(driver, vm, incoming->deferredURI, asyncJob) < 0)
> -        goto stop;
> +    if (incoming) {
> +        if (incoming->deferredURI &&
> +            qemuMigrationDstRun(driver, vm, incoming->deferredURI, asyncJob) < 0)
> +            goto stop;
> +    } else {
> +        /* Refresh state of devices from QEMU. During migration this
> +         * needs to happen after the state information is fully
> +         * transferred. */
> +        if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
> +            goto stop;
> +    }
>  
>      if (qemuProcessFinishStartup(driver, vm, asyncJob,
>                                   !(flags & VIR_QEMU_PROCESS_START_PAUSED),
> @@ -6945,11 +6952,6 @@ qemuProcessStart(virConnectPtr conn,
>          /* Keep watching qemu log for errors during incoming migration, otherwise
>           * unset reporting errors from qemu log. */
>          qemuMonitorSetDomainLog(priv->mon, NULL, NULL, NULL);
> -
> -        /* Refresh state of devices from qemu. During migration this needs to
> -         * happen after the state information is fully transferred. */
> -        if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
> -            goto stop;
>      }
>  
>      ret = 0;
> -- 
> 2.17.0
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|