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