On 09/11/2015 09:26 AM, Jiri Denemark wrote:
> When persistently migrating a domain to a destination host where the
> same domain already exists (i.e., it is persistent and shutdown at the
> destination), we would happily through away the original persistent
s/through/throw
> definition without properly freeing it. And when updating the definition
> fails for some reason we don't properly revert to the original state
> leaving the domain broken.
>
> In addition to fixing these issues, the patch also makes sure the domain
> definition parsed from a migration cookie is either used or freed.
>
> Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
> ---
>
> Notes:
> Version 2:
> - new patch
>
> src/qemu/qemu_migration.c | 56 +++++++++++++++++++++++++++++++----------------
> 1 file changed, 37 insertions(+), 19 deletions(-)
>
Ran using my Coverity checker...
One issue - in qemuMigrationPersist can get to 'cleanup:' calling
qemuMigrationCookieGetPersistent when 'mig == NULL' from either the goto
in the "if (!qemuMigrationJobIsActive(vm...)" or "if (!(mig =
qemuMigrationEatCookie(driver, ..." paths
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index c761657..1d556eb 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -525,6 +525,19 @@ qemuMigrationCookieAddPersistent(qemuMigrationCookiePtr mig,
> }
>
>
> +static virDomainDefPtr
> +qemuMigrationCookieGetPersistent(qemuMigrationCookiePtr mig)
> +{
> + virDomainDefPtr def = mig->persistent;
> +
> + mig->persistent = NULL;
> + mig->flags &= ~QEMU_MIGRATION_COOKIE_PERSISTENT;
> + mig->flagsMandatory &= ~QEMU_MIGRATION_COOKIE_PERSISTENT;
> +
> + return def;
> +}
> +
> +
> static int
> qemuMigrationCookieAddNetwork(qemuMigrationCookiePtr mig,
> virQEMUDriverPtr driver,
> @@ -5554,47 +5567,51 @@ qemuMigrationVPAssociatePortProfiles(virDomainDefPtr def)
> static int
> qemuMigrationPersist(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> - qemuMigrationCookiePtr mig)
> + qemuMigrationCookiePtr mig,
> + bool ignoreSaveError)
> {
> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> virCapsPtr caps = NULL;
> virDomainDefPtr vmdef;
> + virDomainDefPtr oldDef = NULL;
> + unsigned int oldPersist = vm->persistent;
> virObjectEventPtr event;
> - bool newVM;
> int ret = -1;
>
> if (!(caps = virQEMUDriverGetCapabilities(driver, false)))
> goto cleanup;
>
> - newVM = !vm->persistent;
> vm->persistent = 1;
> + oldDef = vm->newDef;
> + vm->newDef = qemuMigrationCookieGetPersistent(mig);
>
> - if (mig->persistent)
> - vm->newDef = mig->persistent;
> + if (!(vmdef = virDomainObjGetPersistentDef(caps, driver->xmlopt, vm)))
> + goto error;
>
> - vmdef = virDomainObjGetPersistentDef(caps, driver->xmlopt, vm);
> - if (!vmdef) {
> - if (newVM)
> - vm->persistent = 0;
> - goto cleanup;
> - }
> -
> - if (virDomainSaveConfig(cfg->configDir, vmdef) < 0)
> - goto cleanup;
> + if (virDomainSaveConfig(cfg->configDir, vmdef) < 0 && !ignoreSaveError)
> + goto error;
>
> event = virDomainEventLifecycleNewFromObj(vm,
> VIR_DOMAIN_EVENT_DEFINED,
> - newVM ?
> - VIR_DOMAIN_EVENT_DEFINED_ADDED :
> - VIR_DOMAIN_EVENT_DEFINED_UPDATED);
> + oldPersist ?
> + VIR_DOMAIN_EVENT_DEFINED_UPDATED :
> + VIR_DOMAIN_EVENT_DEFINED_ADDED);
> qemuDomainEventQueue(driver, event);
>
> ret = 0;
>
> cleanup:
> + virDomainDefFree(oldDef);
> virObjectUnref(caps);
> virObjectUnref(cfg);
> return ret;
> +
> + error:
> + virDomainDefFree(vm->newDef);
> + vm->persistent = oldPersist;
This set of changes resolves what I pointed out in patch 1 regarding
setting vm->persistent
> + vm->newDef = oldDef;
> + oldDef = NULL;
> + goto cleanup;
> }
>
>
> @@ -5653,7 +5670,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
> */
> if (flags & VIR_MIGRATE_OFFLINE) {
> if (retcode != 0 ||
> - qemuMigrationPersist(driver, vm, mig) < 0)
> + qemuMigrationPersist(driver, vm, mig, false) < 0)
> goto endjob;
>
> dom = virGetDomain(dconn, vm->def->name, vm->def->uuid);
> @@ -5685,7 +5702,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
> goto endjob;
>
> if (flags & VIR_MIGRATE_PERSIST_DEST) {
> - if (qemuMigrationPersist(driver, vm, mig) < 0) {
> + if (qemuMigrationPersist(driver, vm, mig, !v3proto) < 0) {
> /* Hmpf. Migration was successful, but making it persistent
> * was not. If we report successful, then when this domain
> * shuts down, management tools are in for a surprise. On the
> @@ -5796,6 +5813,7 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
> qemuMonitorSetDomainLog(priv->mon, -1);
> VIR_FREE(priv->origname);
> virDomainObjEndAPI(&vm);
> + virDomainDefFree(qemuMigrationCookieGetPersistent(mig));
If this has a "if (mig)", then Coverity is happy.
ACK with the adjustment
John
> qemuMigrationCookieFree(mig);
> if (orig_err) {
> virSetError(orig_err);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list