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@xxxxxxxxxxxxx>
> ---
> 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 we
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;
> }
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
