[libvirt] [PATCH] qemu: Always remove domain object if MigratePrepare fails

[Eric Blake]

On 09/26/2011 05:50 AM, Jiri Denemark wrote:
> If migration failed in Prepare phase after virDomainAssignDef and before
> a job is started, the domain object was not properly removed.
> ---
>   src/qemu/qemu_migration.c |   11 ++++++-----
>   1 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 0a5a13d..ea49093 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1178,8 +1178,12 @@ cleanup:
>       virDomainDefFree(def);
>       VIR_FORCE_CLOSE(dataFD[0]);
>       VIR_FORCE_CLOSE(dataFD[1]);
> -    if (vm)
> -        virDomainObjUnlock(vm);
> +    if (vm) {
> +        if (vm->persistent)
> +            virDomainObjUnlock(vm);
> +        else
> +            qemuDomainRemoveInactive(driver, vm);

I think this part is not quite right.  This will also remove a transient 
destination vm on the success path.

> +    }
>       if (event)
>           qemuDomainEventQueue(driver, event);
>       qemuMigrationCookieFree(mig);
> @@ -1188,9 +1192,6 @@ cleanup:
>   endjob:
>       if (qemuMigrationJobFinish(driver, vm) == 0) {
>           vm = NULL;
> -    } else if (!vm->persistent) {
> -        qemuDomainRemoveInactive(driver, vm);
> -        vm = NULL;
>       }

I agree that moving this hunk out of endjob: and into cleanup: means 
that you will properly clean up for more failure cases, but I think 
you're missing a (ret < 0) check in cleanup: for this to be 
bullet-proof.  I'd also feel more comfortable if Dan could review this 
and/or run the TCK migration suite on it.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org