On Thu, Aug 29, 2019 at 17:55:43 -0300, Maxiwell S. Garcia wrote:
> The snapshot-create operation of running guests saves the live
> XML and uses it to replace the active and inactive domain in
> case of revert. So, the config XML is ignored by the snapshot
> process. This commit changes it and adds the config XML in the
> snapshot XML as the <inactiveDomain> entry.
>
> In case of offline guest, the behavior remains the same and the
> config XML is saved in the snapshot XML as <domain> entry. The
> behavior of older snapshots of running guests, that don't have
> the new <inactiveDomain>, remains the same too. The revert, in
> this case, overrides both active and inactive domain with the
> <domain> entry. So, the <inactiveDomain> in the snapshot XML is
> not required to snapshot work, but it's useful to preserve the
> config XML of running guests.
>
> Signed-off-by: Maxiwell S. Garcia <maxiw...@linux.ibm.com>
> ---
> src/conf/moment_conf.c | 1 +
> src/conf/moment_conf.h | 11 +++++++++++
> src/conf/snapshot_conf.c | 22 ++++++++++++++++++++--
> src/qemu/qemu_driver.c | 37 ++++++++++++++++++++++++++++---------
> 4 files changed, 60 insertions(+), 11 deletions(-)
>
> diff --git a/src/conf/moment_conf.c b/src/conf/moment_conf.c
> index fea13f0f97..f54a44b33e 100644
> --- a/src/conf/moment_conf.c
> +++ b/src/conf/moment_conf.c
> @@ -66,6 +66,7 @@ virDomainMomentDefDispose(void *obj)
> VIR_FREE(def->description);
> VIR_FREE(def->parent_name);
> virDomainDefFree(def->dom);
> + virDomainDefFree(def->inactiveDom);
> }
>
> /* Provide defaults for creation time and moment name after parsing XML */
> diff --git a/src/conf/moment_conf.h b/src/conf/moment_conf.h
> index 9fdbef2172..70cc47bd70 100644
> --- a/src/conf/moment_conf.h
> +++ b/src/conf/moment_conf.h
> @@ -36,7 +36,18 @@ struct _virDomainMomentDef {
> char *parent_name;
> long long creationTime; /* in seconds */
>
> + /*
> + * Store the active domain definition in case of online
> + * guest and the inactive domain definition in case of
> + * offline guest
> + */
> virDomainDefPtr dom;
> +
> + /*
> + * Store the inactive domain definition in case of online
> + * guest and leave NULL in case of offline guest
> + */
> + virDomainDefPtr inactiveDom;
OK, I can live with this since dom/inactiveDom are direct equivalents to
domain/inactiveDomain elements in the snapshot XML, ...
> };
>
> virClassPtr virClassForDomainMomentDef(void);
...
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 78f5471b79..67511b705a 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
...
> @@ -16572,11 +16580,10 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
> snapshot,
> * in the failure cases where we know there was no change? */
> }
>
> - /* Prepare to copy the snapshot inactive xml as the config of this
> - * domain.
> - *
> - * XXX Should domain snapshots track live xml rather
> - * than inactive xml? */
> + /* Prepare to copy the snapshot inactive domain as the config XML
> + * and the snapshot domain as the live XML. In case of inactive domain
> + * NULL, both config and live XML will be copied from snapshot domain.
> + */
> if (snap->def->dom) {
> config = virDomainDefCopy(snap->def->dom, caps,
> driver->xmlopt, priv->qemuCaps, true);
> @@ -16584,6 +16591,15 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
> snapshot,
> goto endjob;
> }
>
> + if (snap->def->inactiveDom) {
> + inactiveConfig = virDomainDefCopy(snap->def->inactiveDom, caps,
> + driver->xmlopt, priv->qemuCaps,
> true);
> + if (!inactiveConfig)
> + goto endjob;
> + } else {
> + inactiveConfig = config;
> + }
> +
...but the code here and the comment above should be changed a bit. You
can't point both config and inactiveConfig to the same virDomainDef
structure (see below). I believe config/inactiveConfig should be used
strictly for active/inactive domain definition (thus config could be
NULL for inactive snapshots) and we should copy the definition twice for
active snapshots without inactiveDom:
+ if (snap->def->inactiveDom) {
+ inactiveConfig = virDomainDefCopy(snap->def->inactiveDom, caps,
+ driver->xmlopt, priv->qemuCaps,
true);
+ if (!inactiveConfig)
+ goto endjob;
+ } else {
+ /* Inactive domain definition is missing:
+ * - either this is an old active snapshot and we need to copy the
+ * active definition as an inactive one
+ * - or this is an inactive snapshot which means config contains the
+ * inactive definition.
+ */
+ if (snapdef->state == VIR_DOMAIN_SNAPSHOT_RUNNING ||
+ snapdef->state == VIR_DOMAIN_SNAPSHOT_PAUSED) {
+ inactiveConfig = virDomainDefCopy(snap->def->dom, caps,
+ driver->xmlopt,
priv->qemuCaps, true);
+ if (!inactiveConfig)
+ goto endjob;
+ } else {
+ VIR_STEAL_PTR(inactiveConfig, config);
+ }
+ }
> cookie = (qemuDomainSaveCookiePtr) snapdef->cookie;
>
> switch ((virDomainSnapshotState) snapdef->state) {
> @@ -16686,16 +16702,19 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
> snapshot,
> goto endjob;
> }
> if (config) {
> - virDomainObjAssignDef(vm, config, false, NULL);
> virCPUDefFree(priv->origCPU);
> VIR_STEAL_PTR(priv->origCPU, origCPU);
> }
> + if (inactiveConfig)
> + virDomainObjAssignDef(vm, inactiveConfig, false, NULL);
If both domain and inactiveDomain elements were present in the snapshot
XML, you'd leak config here.
I realized that the config can be leaked even with the current code if
qemuDomainRevertToSnapshot fails so I sent a patch fixing that:
https://www.redhat.com/archives/libvir-list/2019-September/msg00382.html
Once the memory leak is fixed, this hunk needs a trivial change to fit
the new code.
> } else {
> /* Transitions 2, 3 */
> load:
> was_stopped = true;
> + if (inactiveConfig)
> + virDomainObjAssignDef(vm, inactiveConfig, false, NULL);
> if (config)
> - virDomainObjAssignDef(vm, config, false, NULL);
> + virDomainObjAssignDef(vm, config, true, NULL);
In case inactiveDomain was not present in the snapshot XML, both
inactiveConfig and config would contain the same pointer. Thus you'd
assign a single domain definition twice, which would result in funny
things such as memory corruption and double free once one of them is
freed for some reason.
This is fixed by using the modified code for filling config and
inactiveConfig (see above) and following the changes I made to avoid
leaking memory (msg00382.html).
>
> /* No cookie means libvirt which saved the domain was too old to
> * mess up the CPU definitions.
> @@ -16781,8 +16800,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr
> snapshot,
> qemuProcessEndJob(driver, vm);
> goto cleanup;
> }
> - if (config)
> - virDomainObjAssignDef(vm, config, false, NULL);
> + if (inactiveConfig)
> + virDomainObjAssignDef(vm, inactiveConfig, false, NULL);
This should be ok since we are reverting to a snapshot of a shutoff
domain and thus the snapshot XML will contain only one domain
definition.
However, this hunk needs a trivial change to fit the new code after the
memory leak fix is pushed.
>
> if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
> VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) {
With the suggested changes
Reviewed-by: Jiri Denemark <jdene...@redhat.com>
I'll send the patch with all modifications (which I will push once the
memory leak fix is acked) as a reply to this email for completeness.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
