To resolve the bug: #253.

The restore method should call the qemuProcessRefreshState method
to refreash the state of the devices.

---
 src/qemu/qemu_process.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 6b83a571b9..ebd60a7b84 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -7703,14 +7703,11 @@ qemuProcessStart(virConnectPtr conn,
         if (incoming->deferredURI &&
             qemuMigrationDstRun(driver, vm, incoming->deferredURI, asyncJob) < 0)
             goto stop;
-    } else {
-        /* Refresh state of devices from QEMU. During migration this happens
-         * in qemuMigrationDstFinish to ensure that state information is fully
-         * transferred. */
-        if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
-            goto stop;
     }
 
+    if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
+        goto stop;
+
     if (qemuProcessFinishStartup(driver, vm, asyncJob,
                                  !(flags & VIR_QEMU_PROCESS_START_PAUSED),
                                  incoming ?
-- 
2.27.0

On Fri, Dec 10, 2021 at 11:26:18 +0800, JorhsonDeng wrote:
> To resolve the bug: #253.
> 
> The restore method should call the qemuProcessRefreshState method
> to refreash the state of the devices.

Please also mention that this happens when restoring a VM from a save
image, as it's not clear from the commit message itself.

In general the commit message must be a standalone source of information
describing what's happening. This also means that the summary line
should be more descriptive.


Note that the libvirt project requires that contributors declare
conformance with the Developer certificate of origin:

https://www.libvirt.org/hacking.html#developer-certificate-of-origin

> 
> ---
>  src/qemu/qemu_process.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 6b83a571b9..ebd60a7b84 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -7703,14 +7703,11 @@ qemuProcessStart(virConnectPtr conn,
>          if (incoming->deferredURI &&
>              qemuMigrationDstRun(driver, vm, incoming->deferredURI, asyncJob) < 0)
>              goto stop;
> -    } else {
> -        /* Refresh state of devices from QEMU. During migration this happens
> -         * in qemuMigrationDstFinish to ensure that state information is fully
> -         * transferred. */
> -        if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
> -            goto stop;


So you are removing a comment which says that for migration this is
happening elsewhere without any justification or fix to the code.

The refresh in case of migration is happening in a different place for a
very good reason so we must keep it that way. This means you'll have to
introduce some form of logic which refreshes the state only when
restoring a save image.

>      }
>  
> +    if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
> +        goto stop;
> +
>      if (qemuProcessFinishStartup(driver, vm, asyncJob,
>                                   !(flags & VIR_QEMU_PROCESS_START_PAUSED),
>                                   incoming ?
> -- 
> 2.27.0
> 

Thanks, This is my first fatch to libvirt project and there are some rules 
I am not familar to.
I feel starnge that the comment, becasue the live-migration do the refresh
in qemuMigrationDstFinish, but restore method is not called this function.
And the live-migration in qemuMigrationDstPrepareAny is not the
same function flow with the restore in qemuDomainObjStart. I was wonder
there some forget to the fcuntion qemuProcessRefreshState.














At 2021-12-10 19:37:24, "Peter Krempa" <pkrempa@redhat.com> wrote:
>On Fri, Dec 10, 2021 at 11:26:18 +0800, JorhsonDeng wrote:
>> To resolve the bug: #253.
>> 
>> The restore method should call the qemuProcessRefreshState method
>> to refreash the state of the devices.
>
>Please also mention that this happens when restoring a VM from a save
>image, as it's not clear from the commit message itself.
>
>In general the commit message must be a standalone source of information
>describing what's happening. This also means that the summary line
>should be more descriptive.
>
>
>Note that the libvirt project requires that contributors declare
>conformance with the Developer certificate of origin:
>
>https://www.libvirt.org/hacking.html#developer-certificate-of-origin
>
>> 
>> ---
>>  src/qemu/qemu_process.c | 9 +++------
>>  1 file changed, 3 insertions(+), 6 deletions(-)
>> 
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index 6b83a571b9..ebd60a7b84 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -7703,14 +7703,11 @@ qemuProcessStart(virConnectPtr conn,
>>          if (incoming->deferredURI &&
>>              qemuMigrationDstRun(driver, vm, incoming->deferredURI, asyncJob) < 0)
>>              goto stop;
>> -    } else {
>> -        /* Refresh state of devices from QEMU. During migration this happens
>> -         * in qemuMigrationDstFinish to ensure that state information is fully
>> -         * transferred. */
>> -        if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
>> -            goto stop;
>
>
>So you are removing a comment which says that for migration this is
>happening elsewhere without any justification or fix to the code.
>
>The refresh in case of migration is happening in a different place for a
>very good reason so we must keep it that way. This means you'll have to
>introduce some form of logic which refreshes the state only when
>restoring a save image.
>
>>      }
>>  
>> +    if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
>> +        goto stop;
>> +
>>      if (qemuProcessFinishStartup(driver, vm, asyncJob,
>>                                   !(flags & VIR_QEMU_PROCESS_START_PAUSED),
>>                                   incoming ?
>> -- 
>> 2.27.0
>>