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

[Jiri Denemark]

On Mon, Sep 26, 2011 at 15:56:03 -0600, Eric Blake wrote:
> 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.

Oops, yes, I sent a v2 which fixes that. Thanks for the careful review.

Jirka