[libvirt] [PATCH] qemu: Do not override config XML in case of snapshot revert

[Daniel Henrique Barboza]

Tried to reproduce the error using my x86 laptop but got hit by
https://bugzilla.redhat.com/show_bug.cgi?id=1689216 when trying
to create the snapshot using upstream code:

$ sudo ./run tools/virsh snapshot-create-as ub1810-cpu-hotplug snap
error: operation failed: Failed to take snapshot: Error: Nested VMX 
virtualization does not support live migration yet


Since this issue is arch independent I tried it out with a Power server.
Here's the behavior with current upstream:


- relevant piece of VM XML:

<domain type='kvm'>
   <name>dhb</name>
   <uuid>d3fe0169-2be6-431c-85a5-cc178601bb69</uuid>
   <memory unit='KiB'>67108864</memory>
   <currentMemory unit='KiB'>67108864</currentMemory>
   <vcpu placement='static' current='4'>16</vcpu>

$ sudo ./run tools/virsh setvcpus dhb 8 --live

$
$ sudo ./run tools/virsh snapshot-create-as dhb snap1
Domain snapshot snap1 created
$
$ sudo ./run tools/virsh snapshot-revert dhb snap1

$
$ sudo ./run tools/virsh dumpxml --inactive dhb
<domain type='kvm'>
   <name>dhb</name>
   <uuid>d3fe0169-2be6-431c-85a5-cc178601bb69</uuid>
   <memory unit='KiB'>67108864</memory>
   <currentMemory unit='KiB'>67108864</currentMemory>
   <vcpu placement='static' current='8'>16</vcpu>
   <vcpus>

The inactive XML got overwritten by the vcpu hotplug, which is not intended.


After the patch, the problem isn't reproduced: the inactive VM XML was
preserved after the snapshot-revert.


Maxiwell, you (or the commiter) can add the following in the commit-msg:

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


Since the bug you're fixing here also fixes this RH bugzilla.


Reviewed-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
Tested-by: Daniel Henrique Barboza <danielhb413 at gmail.com>




On 4/30/19 2:54 PM, Maxiwell S. Garcia wrote:
> Snapshot create operation saves the live XML and uses it to replace the
> domain definition in case of revert. But the VM config XML is not saved
> and the revert operation does not address this issue. This commit
> prevents the config XML from being overridden by snapshot definition.
>
> An active domain stores both current and new definitions. The current
> definition (vm->def) stores the live XML and the new definition
> (vm->newDef) stores the config XML. In an inactive domain, only the
> config XML is persistent, and it's saved in vm->def.
>
> The revert operation uses the virDomainObjAssignDef() to set the
> snapshot definition in vm->newDef, if domain is active, or in vm->def
> otherwise. But before that, it saves the old value to return to
> caller. This return is used here to restore the config XML after
> all snapshot startup process finish.
>
> Signed-off-by: Maxiwell S. Garcia <maxiwell at linux.ibm.com>
> ---
>   src/qemu/qemu_driver.c | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index b2ac737d1f..a73122454a 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -16251,6 +16251,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>       qemuDomainObjPrivatePtr priv;
>       int rc;
>       virDomainDefPtr config = NULL;
> +    virDomainDefPtr inactiveConfig = NULL;
>       virQEMUDriverConfigPtr cfg = NULL;
>       virCapsPtr caps = NULL;
>       bool was_stopped = false;
> @@ -16465,7 +16466,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>                   goto endjob;
>               }
>               if (config) {
> -                virDomainObjAssignDef(vm, config, false, NULL);
> +                virDomainObjAssignDef(vm, config, false, &inactiveConfig);
>                   virCPUDefFree(priv->origCPU);
>                   VIR_STEAL_PTR(priv->origCPU, origCPU);
>               }
> @@ -16474,7 +16475,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>           load:
>               was_stopped = true;
>               if (config)
> -                virDomainObjAssignDef(vm, config, false, NULL);
> +                virDomainObjAssignDef(vm, config, false, &inactiveConfig);
>   
>               /* No cookie means libvirt which saved the domain was too old to
>                * mess up the CPU definitions.
> @@ -16533,6 +16534,9 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>                                                    detail);
>               }
>           }
> +        if (inactiveConfig)
> +            VIR_STEAL_PTR(vm->newDef, inactiveConfig);
> +
>           break;
>   
>       case VIR_DOMAIN_SNAPSHOT_SHUTDOWN:
> @@ -16560,8 +16564,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>               qemuProcessEndJob(driver, vm);
>               goto cleanup;
>           }
> -        if (config)
> -            virDomainObjAssignDef(vm, config, false, NULL);
> +        if (config) {
> +            virDomainObjAssignDef(vm, config, false, &inactiveConfig);
> +            if (inactiveConfig)
> +                VIR_STEAL_PTR(vm->newDef, inactiveConfig);
> +        }
>   
>           if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
>                        VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) {

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190513/2ff2284e/attachment-0001.htm>