[libvirt] [PATCH v2 8/8] qemu: Fix some corner cases in persistent migration

[John Ferlan]

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 at redhat.com>
> ---
> 
> 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);
>