The destination libvirt daemon in a migration may segfault if the client disconnects immediately after the migration has begun: # virsh -c qemu+tls://remote/system list --all Id Name State ---------------------------------------------------- ... # timeout --signal KILL 1 \ virsh migrate example qemu+tls://remote/system \ --verbose --compressed --live --auto-converge \ --abort-on-error --unsafe --persistent \ --undefinesource --copy-storage-all --xml example.xml Killed # virsh -c qemu+tls://remote/system list --all error: failed to connect to the hypervisor error: unable to connect to server at 'remote:16514': Connection refused The crash is in: 1531 void 1532 qemuDomainObjEndJob(virQEMUDriverPtr driver, virDomainObjPtr obj) 1533 { 1534 qemuDomainObjPrivatePtr priv = obj->privateData; 1535 qemuDomainJob job = priv->job.active; 1536 1537 priv->jobs_queued--; Backtrace: #0 at qemuDomainObjEndJob at qemu/qemu_domain.c:1537 #1 in qemuDomainRemoveInactive at qemu/qemu_domain.c:2497 #2 in qemuProcessAutoDestroy at qemu/qemu_process.c:5646 #3 in virCloseCallbacksRun at util/virclosecallbacks.c:350 #4 in qemuConnectClose at qemu/qemu_driver.c:1154 ... qemuDomainRemoveInactive calls virDomainObjListRemove, which in this case is holding the last remaining reference to the domain. qemuDomainRemoveInactive then calls qemuDomainObjEndJob, but the domain object has been freed and poisoned by then. This patch bumps the domain's refcount until qemuDomainRemoveInactive has completed. We also ensure qemuProcessAutoDestroy does not return the domain to virCloseCallbacksRun to be unlocked in this case. There is similar logic in bhyveProcessAutoDestroy and lxcProcessAutoDestroy (which call virDomainObjListRemove directly). Signed-off-by: Michael Chapman <mike(a)very.puzzling.org&gt; --- src/qemu/qemu_domain.c | 5 +++++ src/qemu/qemu_process.c | 4 +++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index ac5ca74..d433cba 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2424,11 +2424,16 @@ qemuDomainRemoveInactive(virQEMUDriverPtr driver, VIR_WARN("unable to remove snapshot directory %s", snapDir); VIR_FREE(snapDir); } + + virObjectRef(vm); + virDomainObjListRemove(driver->domains, vm); virObjectUnref(cfg); if (haveJob) qemuDomainObjEndJob(driver, vm); + + virObjectUnref(vm); } void diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 515402e..554399e 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5689,8 +5689,10 @@ qemuProcessAutoDestroy(virDomainObjPtr dom, qemuDomainObjEndJob(driver, dom); - if (!dom->persistent) + if (!dom->persistent) { qemuDomainRemoveInactive(driver, dom); + dom = NULL; + } if (event) qemuDomainEventQueue(driver, event); -- 2.1.0

If a VM migration is aborted, a disk mirror may be failed by QEMU before libvirt has a chance to cancel it. The disk->mirrorState remains at _ABORT in this case, and this breaks subsequent mirrorings of that disk. We should instead check the mirrorState directly and transition to _NONE if it is already aborted. Do the check *after* aborting the block job in QEMU to avoid a race. Signed-off-by: Michael Chapman <mike(a)very.puzzling.org&gt; --- src/qemu/qemu_migration.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 21432c0..39ca37c 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1852,6 +1852,10 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, } else { VIR_WARN("Unable to enter monitor. No block job cancelled"); } + + /* If disk mirror is already aborted, clear the mirror state now */ + if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_ABORT) + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; } if (err) virSetError(err); @@ -1920,6 +1924,10 @@ qemuMigrationCancelDriveMirror(qemuMigrationCookiePtr mig, ret = -1; goto cleanup; } + + /* If disk mirror is already aborted, clear the mirror state now */ + if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_ABORT) + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; } cleanup: -- 2.1.0

On Mon, Mar 30, 2015 at 01:41:01PM +1100, Michael Chapman wrote: We shouldn't be storing the uuid twice. I think the CPU time wasted by doing virUUIDFormat here again is negligible and easier than writing a Hash function that works on VIR_UUID buffers :) Jan

The close callbacks hash are keyed by a UUID-string, but virCloseCallbacksRun was attempting to remove them by raw UUID. This patch ensures the callback entries are removed by UUID-string as well. This bug caused problems when guest migrations were abnormally aborted: # timeout --signal KILL 1 \ virsh migrate example qemu+tls://remote/system \ --verbose --compressed --live --auto-converge \ --abort-on-error --unsafe --persistent \ --undefinesource --copy-storage-all --xml example.xml Killed # virsh migrate example qemu+tls://remote/system \ --verbose --compressed --live --auto-converge \ --abort-on-error --unsafe --persistent \ --undefinesource --copy-storage-all --xml example.xml error: Requested operation is not valid: domain 'example' is not being migrated Signed-off-by: Michael Chapman <mike(a)very.puzzling.org&gt; --- src/util/virclosecallbacks.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c index 4128057..9006d36 100644 --- a/src/util/virclosecallbacks.c +++ b/src/util/virclosecallbacks.c @@ -331,8 +331,9 @@ virCloseCallbacksRun(virCloseCallbacksPtr closeCallbacks, return; for (i = 0; i < list->nentries; i++) { - virHashRemoveEntry(closeCallbacks->list, - list->entries[i].uuid); + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(list->entries[i].uuid, uuidstr); + virHashRemoveEntry(closeCallbacks->list, uuidstr); } virObjectUnlock(closeCallbacks); -- 2.1.0