On 20.12.2018 23:31, John Ferlan wrote:
>
>
> 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.
Ok. I thought of a distinct patch too, but having in mind that adding/removing
code in subsequent patches is not preferred I decided to just note this moment
in the message.
>
>>
>> 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 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.
If by otherwise you mean "if we make changes to active config" - then
such changes are saved to status files but they can't be written
directly to config file.
>
> 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.
Well there is at least one case when vm->newDef != NULL but domain is not persistent
- if we undefine domain vm->newDef is not released. This can be fixed but
I'm not sure there are no other cases. So checking vm->persisent is more reliable.
I think this is currenly invariant that if virDomainObjIsActive(vm) && vm->persistent
then vm->newDef != NULL but we can add explicit check.
The best option I think is to fix all cases when vm->newDef != NULL for non persistent
active domain and replace check as you suggested. What do you think?
>
>
>> + !(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.
This is one of the hardest questions - what combinations of flags one should
use for one's purpuses :) I'm not kidding, there are no good definitions of
VIR_DOMAIN_XML_INACTIVE or VIR_DOMAIN_DEF_FORMAT_INACTIVE. So I use same
combination migration uses to pass persistent config to destination -
check qemuMigrationSrcRun.
Hmm, virDomainDefFormatInternal has next hunk at the beginning:
if (def->id == -1)
flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE;
and def->id == -1 for inactive config (@id is set to -1 on parsing
inactive configs and on domain start @def is copied to @newDef before
allocating @id - check qemuProcessInit). So we have this flag anyway :)
Also even the above fact does not have the case AFAIU having this flag for inactive config
does not have much meaning - random hunks of code that I checked filter
dumping info that gained by config on domain start. However I run across a
kind of exception too - @id is not dumped at all if VIR_DOMAIN_DEF_FORMAT_INACTIVE
is set however this is not make much difference.
>
>> + 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.
I should say I'm not looking at this step as "best chance" - if we can
not revert exact state I'd prefer to fail here.
>
>> 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?
Good catch! I think I'll change to:
if (config) {
virDomainObjAssignDef(vm, config, false, NULL);
VIR_STEAL_PTR(newConfig, config);
}
because if we have persistConfig but fail to assing it - means
we are on error path and we won't save newConfig anyway.
>
>> + }
>>
>> /* 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);
>> + }
>> }
This makes me wonder if virDomainObjAssignDef is too overcomplicated
if we need such comments.
Nikolay
>>
>> /* 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
