Re: [libvirt] [PATCH 3/9] qemu: snapshot: save/restore inactive persistent config

On 12/13/18 3:03 AM, Nikolay Shirokovskiy wrote: > In case of active persistent domain snapshot metadata is > not complete. We save only active configuration and on > restore use it both for active and inactive configuration. > Let's fix it and save and restore both in this case. > > In case of active transient or inactive domain we have > only one config and thus everything is good. > > By the way this patch fixes config memleak on error paths. use @config to make it clearer... I see how/why you combined things and although extracting out the memleak is a bit difficult, I think it should be done. Perhaps in the cleanup code to *SaveConfig, you just set @config = NULL "temporarily" since we know/assume virDomainObjAssignDef was successful and we wouldn't want the subsequently added virDomainDefFree(config) to free it on those paths, but we would want it to be free'd on error paths. Then by using @newConfig in this patch, it's even more obvious and of course the temporary config = NULL would be removed... Hope this makes sense.

> > Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; > --- > src/conf/snapshot_conf.c | 1 + > src/conf/snapshot_conf.h | 2 ++ > src/qemu/qemu_driver.c | 44 +++++++++++++++++++++++++++++++++++++++----- > 3 files changed, 42 insertions(+), 5 deletions(-) > Not my area of expertise, but rather than have it sit unreviewed... > diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c > index 5a511b4..2e4a0b9 100644 > --- a/src/conf/snapshot_conf.c > +++ b/src/conf/snapshot_conf.c > @@ -102,6 +102,7 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def) > virDomainSnapshotDiskDefClear(&def->disks[i]); > VIR_FREE(def->disks); > virDomainDefFree(def->dom); > + virDomainDefFree(def->persistDom); > virObjectUnref(def->cookie); > VIR_FREE(def); > } > diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h > index 20a42bd..3da204a 100644 > --- a/src/conf/snapshot_conf.h > +++ b/src/conf/snapshot_conf.h > @@ -75,6 +75,8 @@ struct _virDomainSnapshotDef { > virDomainSnapshotDiskDef *disks; > > virDomainDefPtr dom; > + /* inactive domain config in case of active persistent domain */ > + virDomainDefPtr persistDom; > > virObjectPtr cookie; > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index f7c1d6f..7e0585d 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -15687,6 +15687,12 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, > VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) > goto endjob; > > + if (virDomainObjIsActive(vm) && vm->persistent && As it's easy to get lost or forget... IIRC vm->persistent means that w> have used the Define functions to provide a "config" at some point in

time. Then eventually when/if we modify the "active" object to make a configuration specific change, then the @newDef gets populated with the config change(s); otherwise, the changes are made to the "active" def and eventually/possibly saved. All the magic is hidden away in accessors that I don't think about all that often any more.

So, should the vm->persistent be vm->newDef instead? Either way if the following call is made and vm->newDef == NULL, then eventually in virDomainDefFormatInternal we would core when @def was NULL.

> + !(def->persistDom = qemuDomainDefCopy(driver, vm->newDef, > + VIR_DOMAIN_XML_SECURE | > + VIR_DOMAIN_XML_MIGRATABLE))) Do we need VIR_DOMAIN_XML_INACTIVE too since this is the "next" persistent config def? A variant for parse is used later in patch5.

> + goto endjob; > + Since we are doing this as "best chance" and aren't requiring it, what are your (and perhaps others) thoughts related to making this really optional. As in if the qemuDomainDefCopy fails, then we just throw a VIR_INFO, clear the last error, and continue on? I don't have a preference, but I'm just throwing it out there as an idea.

> if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { > align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; > align_match = false; > @@ -16219,6 +16225,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, > qemuDomainObjPrivatePtr priv; > int rc; > virDomainDefPtr config = NULL; > + virDomainDefPtr persistConfig = NULL; > virQEMUDriverConfigPtr cfg = NULL; > virCapsPtr caps = NULL; > bool was_stopped = false; > @@ -16226,6 +16233,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, > virCPUDefPtr origCPU = NULL; > unsigned int start_flags = VIR_QEMU_PROCESS_START_GEN_VMID; > qemuDomainAsyncJob jobType = QEMU_ASYNC_JOB_START; > + virDomainDefPtr newConfig = NULL; > > virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | > VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED | > @@ -16332,6 +16340,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, > goto endjob; > } > > + if (snap->def->persistDom && > + !(persistConfig = virDomainDefCopy(snap->def->persistDom, caps, > + driver->xmlopt, NULL, true))) > + goto endjob; > + Based only on my previous comment, for this part I agree especially since if we were able to save a config, then we'd want to be sure to restore it. > cookie = (qemuDomainSaveCookiePtr) snap->def->cookie; > > switch ((virDomainState) snap->def->state) { > @@ -16440,8 +16453,18 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, > * failed loadvm attempt? */ > goto endjob; > } > - if (config) { > - virDomainObjAssignDef(vm, config, false, NULL); > + > + /* Older versions do not save inactive config in metadata, instead > + * they use active config for this purpose, so keep this behaviour > + * for backward compat. > + */ > + if (persistConfig) > + VIR_STEAL_PTR(newConfig, persistConfig); > + else > + VIR_STEAL_PTR(newConfig, config); > + > + if (newConfig) { > + virDomainObjAssignDef(vm, newConfig, false, NULL); > virCPUDefFree(priv->origCPU); > VIR_STEAL_PTR(priv->origCPU, origCPU); > } > @@ -16449,8 +16472,10 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, > /* Transitions 2, 3 */ > load: > was_stopped = true; > - if (config) > + if (config) { > virDomainObjAssignDef(vm, config, false, NULL); > + config = NULL; This "makes sense" except for the case where @persistConfig == NULL, then we wouldn't virDomainSaveConfig below as we currently do. So maybe this should be: if (!persistConfig) VIR_STEAL_PTR(newConfig, config); else config = NULL; That way we won't virDomainDefFree @config below (which is what you're trying to avoid), but we will save @config as we did before unless of course @persistConfig is going to be used. Make sense?

> + } > > /* No cookie means libvirt which saved the domain was too old to > * mess up the CPU definitions. > @@ -16471,6 +16496,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, > detail); > if (rc < 0) > goto endjob; > + > + if (persistConfig) { Perhaps we should point out: /* This will save the @persistConfig in the vm->newDef where it * was originally copied from. */ John > + virDomainObjAssignDef(vm, persistConfig, false, NULL); > + VIR_STEAL_PTR(newConfig, persistConfig); > + } > }

> > /* Touch up domain state. */ > @@ -16535,8 +16565,10 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, > qemuDomainRemoveInactive(driver, vm); > goto endjob; > } > - if (config) > + if (config) { > virDomainObjAssignDef(vm, config, false, NULL); > + VIR_STEAL_PTR(newConfig, config); > + } > > if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | > VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) { > @@ -16600,7 +16632,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, > } else if (snap) { > snap->def->current = false; > } > - if (ret == 0 && config && vm->persistent && > + if (ret == 0 && newConfig && vm->persistent && > !(ret = virDomainSaveConfig(cfg->configDir, driver->caps, > vm->newDef ? vm->newDef : vm->def))) { > detail = VIR_DOMAIN_EVENT_DEFINED_FROM_SNAPSHOT; > @@ -16616,6 +16648,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, > virObjectUnref(cfg); > virNWFilterUnlockFilterUpdates(); > virCPUDefFree(origCPU); > + virDomainDefFree(config); > + virDomainDefFree(persistConfig); > > return ret; > } >