[libvirt] [PATCH] Fix peer2peer migration with transient VMs

[Wen Congyang]

At 05/20/2011 06:04 PM, Daniel P. Berrange Write:
> The qemuMigrationConfirm method shouldn't deal with final VM
> cleanup, since it can be called from the peer2peer migration,
> which expects to still use the 'vm' object afterwards.
> 
> Push the cleanup code out of qemuMigrationConfirm, into its
> caller, qemuDomainMigrateConfirm3
> 
> * src/qemu/qemu_driver.c: Add VM cleanup code to
>   qemuDomainMigrateConfirm3
> * src/qemu/qemu_migration.c, src/qemu/qemu_migration.h: Remove
>   job handling cleanup from qemuMigrationConfirm
> ---
>  src/qemu/qemu_driver.c    |   25 +++++++++++++++++++++-
>  src/qemu/qemu_migration.c |   51 +++++++++++++++------------------------------
>  src/qemu/qemu_migration.h |    3 +-
>  3 files changed, 42 insertions(+), 37 deletions(-)
> 
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 4fdf5e9..771e401 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -6292,6 +6292,7 @@ qemuDomainMigrateConfirm3(virDomainPtr domain,
>      struct qemud_driver *driver = domain->conn->privateData;
>      virDomainObjPtr vm;
>      int ret = -1;
> +    virErrorPtr orig_err;
>  
>      virCheckFlags(VIR_MIGRATE_LIVE |
>                    VIR_MIGRATE_PEER2PEER |
> @@ -6312,11 +6313,33 @@ qemuDomainMigrateConfirm3(virDomainPtr domain,
>          goto cleanup;
>      }
>  
> +    if (qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
> +        goto cleanup;
> +
>      ret = qemuMigrationConfirm(driver, domain->conn, vm,
>                                 cookiein, cookieinlen,
> -                               flags, cancelled, false);
> +                               flags, cancelled);
> +
> +    orig_err = virSaveLastError();
> +
> +    if (qemuDomainObjEndJob(vm) == 0) {
> +        vm = NULL;
> +    } else if (!virDomainObjIsActive(vm) &&
> +               (!vm->persistent || (flags & VIR_MIGRATE_UNDEFINE_SOURCE))) {
> +        if (flags & VIR_MIGRATE_UNDEFINE_SOURCE)
> +            virDomainDeleteConfig(driver->configDir, driver->autostartDir, vm);
> +        virDomainRemoveInactive(&driver->domains, vm);
> +        vm = NULL;
> +    }
> +
> +    if (orig_err) {
> +        virSetError(orig_err);
> +        virFreeError(orig_err);
> +    }
>  
>  cleanup:
> +    if (vm)
> +        virDomainObjUnlock(vm);
>      qemuDriverUnlock(driver);
>      return ret;
>  }
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 40c40f3..88cab77 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1920,7 +1920,7 @@ finish:
>      cookieoutlen = 0;
>      ret = qemuMigrationConfirm(driver, sconn, vm,
>                                 cookiein, cookieinlen,
> -                               flags, cancelled, true);
> +                               flags, cancelled);
>      /* If Confirm3 returns -1, there's nothing more we can
>       * do, but fortunately worst case is that there is a
>       * domain left in 'paused' state on source.
> @@ -2086,12 +2086,6 @@ int qemuMigrationPerform(struct qemud_driver *driver,
>          event = virDomainEventNewFromObj(vm,
>                                           VIR_DOMAIN_EVENT_STOPPED,
>                                           VIR_DOMAIN_EVENT_STOPPED_MIGRATED);
> -        if (!vm->persistent || (flags & VIR_MIGRATE_UNDEFINE_SOURCE)) {
> -            virDomainDeleteConfig(driver->configDir, driver->autostartDir, vm);
> -            if (qemuDomainObjEndJob(vm) > 0)
> -                virDomainRemoveInactive(&driver->domains, vm);
> -            vm = NULL;
> -        }
>      }
>  
>      ret = 0;
> @@ -2113,9 +2107,17 @@ endjob:
>                                           VIR_DOMAIN_EVENT_RESUMED,
>                                           VIR_DOMAIN_EVENT_RESUMED_MIGRATED);
>      }
> -    if (vm &&
> -        qemuDomainObjEndJob(vm) == 0)
> -        vm = NULL;
> +    if (vm) {
> +        if (qemuDomainObjEndJob(vm) == 0) {
> +            vm = NULL;
> +        } else if (!virDomainObjIsActive(vm) &&
> +                   (!vm->persistent || (flags & VIR_MIGRATE_UNDEFINE_SOURCE))) {
> +            if (flags & VIR_MIGRATE_UNDEFINE_SOURCE)
> +                virDomainDeleteConfig(driver->configDir, driver->autostartDir, vm);
> +            virDomainRemoveInactive(&driver->domains, vm);
> +            vm = NULL;
> +        }
> +    }
>  
>  cleanup:
>      if (vm)
> @@ -2306,9 +2308,8 @@ int qemuMigrationConfirm(struct qemud_driver *driver,
>                           virDomainObjPtr vm,
>                           const char *cookiein,
>                           int cookieinlen,
> -                         unsigned int flags,
> -                         int retcode,
> -                         bool skipJob)
> +                         unsigned int flags ATTRIBUTE_UNUSED,
> +                         int retcode)
>  {
>      qemuMigrationCookiePtr mig;
>      virDomainEventPtr event = NULL;
> @@ -2317,14 +2318,10 @@ int qemuMigrationConfirm(struct qemud_driver *driver,
>      if (!(mig = qemuMigrationEatCookie(vm, cookiein, cookieinlen, 0)))
>          return -1;
>  
> -    if (!skipJob &&
> -        qemuDomainObjBeginJobWithDriver(driver, vm) < 0)
> -        goto cleanup;
> -
>      if (!virDomainObjIsActive(vm)) {
>          qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                          _("guest unexpectedly quit"));
> -        goto endjob;
> +        goto cleanup;
>      }
>  
>      /* Did the migration go as planned?  If yes, kill off the
> @@ -2337,12 +2334,6 @@ int qemuMigrationConfirm(struct qemud_driver *driver,
>          event = virDomainEventNewFromObj(vm,
>                                           VIR_DOMAIN_EVENT_STOPPED,
>                                           VIR_DOMAIN_EVENT_STOPPED_MIGRATED);
> -        if (!vm->persistent || (flags & VIR_MIGRATE_UNDEFINE_SOURCE)) {
> -            virDomainDeleteConfig(driver->configDir, driver->autostartDir, vm);
> -            if (qemuDomainObjEndJob(vm) > 0)
> -                virDomainRemoveInactive(&driver->domains, vm);
> -            vm = NULL;
> -        }
>      } else {
>  
>          /* run 'cont' on the destination, which allows migration on qemu
> @@ -2354,7 +2345,7 @@ int qemuMigrationConfirm(struct qemud_driver *driver,
>              if (virGetLastError() == NULL)
>                  qemuReportError(VIR_ERR_INTERNAL_ERROR,
>                                  "%s", _("resume operation failed"));
> -            goto endjob;
> +            goto cleanup;
>          }
>  
>          event = virDomainEventNewFromObj(vm,
> @@ -2362,22 +2353,14 @@ int qemuMigrationConfirm(struct qemud_driver *driver,
>                                           VIR_DOMAIN_EVENT_RESUMED_MIGRATED);
>          if (virDomainSaveStatus(driver->caps, driver->stateDir, vm) < 0) {
>              VIR_WARN("Failed to save status on vm %s", vm->def->name);
> -            goto endjob;
> +            goto cleanup;
>          }
>      }
>  
>      qemuMigrationCookieFree(mig);
>      rv = 0;
>  
> -endjob:
> -    if (vm &&
> -        !skipJob &&
> -        qemuDomainObjEndJob(vm) == 0)
> -        vm = NULL;
> -
>  cleanup:
> -    if (vm)
> -        virDomainObjUnlock(vm);
>      if (event)
>          qemuDomainEventQueue(driver, event);
>      return rv;
> diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h
> index a350579..08e3acc 100644
> --- a/src/qemu/qemu_migration.h
> +++ b/src/qemu/qemu_migration.h
> @@ -90,8 +90,7 @@ int qemuMigrationConfirm(struct qemud_driver *driver,
>                           const char *cookiein,
>                           int cookieinlen,
>                           unsigned int flags,
> -                         int retcode,
> -                         bool skipJob);
> +                         int retcode);
>  
>  
>  int qemuMigrationToFile(struct qemud_driver *driver, virDomainObjPtr vm,

I test migration with this patch, and it can finish successfully.
I read this patch, and it looks fine to me.

ACK.