[libvirt] [PATCH] qemu: Always remove domain object if MigratePrepare fails
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); + } 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; } goto cleanup; } -- 1.7.6.1
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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
If migration failed in Prepare phase after virDomainAssignDef and before a job is started, the domain object was not properly removed. --- Notes: Version 2: - only remove the inactive domain if ret < 0 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..ada5396 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 (ret >= 0 || vm->persistent) + virDomainObjUnlock(vm); + else + qemuDomainRemoveInactive(driver, vm); + } 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; } goto cleanup; } -- 1.7.6.1
On 09/27/2011 07:23 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. --- Notes: Version 2: - only remove the inactive domain if ret< 0
src/qemu/qemu_migration.c | 11 ++++++----- 1 files changed, 6 insertions(+), 5 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Tue, Sep 27, 2011 at 09:26:59 -0600, Eric Blake wrote:
On 09/27/2011 07:23 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. --- Notes: Version 2: - only remove the inactive domain if ret< 0
src/qemu/qemu_migration.c | 11 ++++++----- 1 files changed, 6 insertions(+), 5 deletions(-)
ACK.
Pushed, thanks. Jirka
participants (2)
-
Eric Blake -
Jiri Denemark