On Wed, Mar 25, 2015 at 02:08:35PM -0600, Jim Fehlig wrote:
A job should be acquired at the beginning of a domain destroy operation, not at the end when cleaning up the domain. Fix two occurances of this late job acquisition in the libxl driver. Doing so renders libxlDomainCleanup unused, so it is removed.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_domain.c | 49 +++++++++++++++++------------------------------- src/libxl/libxl_domain.h | 4 ---- src/libxl/libxl_driver.c | 20 ++++++++++++-------- 3 files changed, 29 insertions(+), 44 deletions(-)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index eca1ff0..e240eae 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -371,6 +371,9 @@ libxlDomainShutdownThread(void *opaque)
cfg = libxlDriverConfigGet(driver);
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) + goto cleanup; +
Won't this a deadlock with 'libxlDomainAutoCoreDump' (line 410) (which also calls : 727 if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) 728 goto cleanup; well, not deadlock - but spin up to 30 seconds? and then give up? Perhaps this patch should be folded in?
From 9f2bac0c28815fc51850290c4b1962881691bfca Mon Sep 17 00:00:00 2001 From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Date: Wed, 25 Mar 2015 22:34:51 -0400 Subject: [PATCH] squash
--- src/libxl/libxl_domain.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index aef0157..774b070 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -723,15 +723,11 @@ libxlDomainAutoCoreDump(libxlDriverPrivatePtr driver, timestr) < 0) goto cleanup; - if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) - goto cleanup; - /* Unlock virDomainObj while dumping core */ virObjectUnlock(vm); libxl_domain_core_dump(cfg->ctx, vm->def->id, dumpfile, NULL); virObjectLock(vm); - ignore_value(libxlDomainObjEndJob(driver, vm)); ret = 0; cleanup: -- 1.8.4.2
if (xl_reason == LIBXL_SHUTDOWN_REASON_POWEROFF) { dom_event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, @@ -384,7 +387,7 @@ libxlDomainShutdownThread(void *opaque) goto restart; case VIR_DOMAIN_LIFECYCLE_PRESERVE: case VIR_DOMAIN_LIFECYCLE_LAST: - goto cleanup; + goto endjob; } } else if (xl_reason == LIBXL_SHUTDOWN_REASON_CRASH) { dom_event = virDomainEventLifecycleNewFromObj(vm, @@ -399,7 +402,7 @@ libxlDomainShutdownThread(void *opaque) goto restart; case VIR_DOMAIN_LIFECYCLE_CRASH_PRESERVE: case VIR_DOMAIN_LIFECYCLE_CRASH_LAST: - goto cleanup; + goto endjob; case VIR_DOMAIN_LIFECYCLE_CRASH_COREDUMP_DESTROY: libxlDomainAutoCoreDump(driver, vm); goto destroy; @@ -420,11 +423,11 @@ libxlDomainShutdownThread(void *opaque) goto restart; case VIR_DOMAIN_LIFECYCLE_PRESERVE: case VIR_DOMAIN_LIFECYCLE_LAST: - goto cleanup; + goto endjob; } } else { VIR_INFO("Unhandled shutdown_reason %d", xl_reason); - goto cleanup; + goto endjob; }
destroy: @@ -433,13 +436,11 @@ libxlDomainShutdownThread(void *opaque) dom_event = NULL; } libxl_domain_destroy(cfg->ctx, vm->def->id, NULL); - if (libxlDomainCleanupJob(driver, vm, reason)) { - if (!vm->persistent) { - virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } - } - goto cleanup; + libxlDomainCleanup(driver, vm, reason); + if (!vm->persistent) + virDomainObjListRemove(driver->domains, vm); + + goto endjob;
restart: if (dom_event) { @@ -447,13 +448,17 @@ libxlDomainShutdownThread(void *opaque) dom_event = NULL; } libxl_domain_destroy(cfg->ctx, vm->def->id, NULL); - libxlDomainCleanupJob(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN); + libxlDomainCleanup(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN); if (libxlDomainStart(driver, vm, false, -1) < 0) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to restart VM '%s': %s"), vm->def->name, err ? err->message : _("unknown error")); }
+ endjob: + if (!libxlDomainObjEndJob(driver, vm)) + vm = NULL; + cleanup: if (vm) virObjectUnlock(vm); @@ -690,26 +695,6 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, }
/* - * Cleanup function for domain that has reached shutoff state. - * Executed in the context of a job. - * - * virDomainObjPtr should be locked on invocation - * Returns true if references remain on virDomainObjPtr, false otherwise. - */ -bool -libxlDomainCleanupJob(libxlDriverPrivatePtr driver, - virDomainObjPtr vm, - virDomainShutoffReason reason) -{ - if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_DESTROY) < 0) - return true; - - libxlDomainCleanup(driver, vm, reason); - - return libxlDomainObjEndJob(driver, vm); -} - -/* * Core dump domain to default dump path. * * virDomainObjPtr must be locked on invocation diff --git a/src/libxl/libxl_domain.h b/src/libxl/libxl_domain.h index a032e46..30855a2 100644 --- a/src/libxl/libxl_domain.h +++ b/src/libxl/libxl_domain.h @@ -108,10 +108,6 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver, virDomainObjPtr vm, virDomainShutoffReason reason);
-bool -libxlDomainCleanupJob(libxlDriverPrivatePtr driver, - virDomainObjPtr vm, - virDomainShutoffReason reason); /* * Note: Xen 4.3 removed the const from the event handler signature. * Detect which signature to use based on diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index da4c1c7..a1977c2 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -1238,10 +1238,13 @@ libxlDomainDestroyFlags(virDomainPtr dom, if (virDomainDestroyFlagsEnsureACL(dom->conn, vm->def) < 0) goto cleanup;
+ if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0) + goto cleanup; + if (!virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("Domain is not running")); - goto cleanup; + goto endjob; }
event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, @@ -1250,18 +1253,19 @@ libxlDomainDestroyFlags(virDomainPtr dom, if (libxl_domain_destroy(cfg->ctx, vm->def->id, NULL) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to destroy domain '%d'"), vm->def->id); - goto cleanup; + goto endjob; }
- if (libxlDomainCleanupJob(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED)) { - if (!vm->persistent) { - virDomainObjListRemove(driver->domains, vm); - vm = NULL; - } - } + libxlDomainCleanup(driver, vm, VIR_DOMAIN_SHUTOFF_DESTROYED); + if (!vm->persistent) + virDomainObjListRemove(driver->domains, vm);
ret = 0;
+ endjob: + if (!libxlDomainObjEndJob(driver, vm)) + vm = NULL; + cleanup: if (vm) virObjectUnlock(vm); -- 1.8.4.5
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel