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

[Marc Hartmayer]

On Mon, Feb 04, 2019 at 01:41 PM +0100, Peter Krempa <pkrempa at redhat.com> wrote:
> On Mon, Feb 04, 2019 at 13:36:24 +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).
>>
>> 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 {
>
> This logic does not seem right ...

I’m not familiar with the usage of this function for migration… so
there's a good chance I'm missing something.

>
>> +        /* Refresh state of devices from QEMU. During migration this
>> +         * needs to happen after the state information is fully
>> +         * transferred. */
>
> as this comment clearly states that this should happen after
> migration.

Is qemuProcessFinishStartup not called explicitly in qemuMigrationFinish
for migration?

>
> Here it would happen only when migration is not done.

Without this patch qemuProcessRefreshState is called after
qemuProcessFinishStartup if (!incoming). Now it’s called directly before
qemuProcessFinishStartup if (!incoming). Why should this be wrong now?
What happens in qemuProcessFinishStartup?

Before your change in 93db7eea1b86408e the function call
'qemuProcessRefreshState' was done in qemuProcessFinishStartup (without
any condition).

Thanks.

>
>
>> +        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
>>
--
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294