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@xxxxxxxx>
> ---
> 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@xxxxxxxxxx>
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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list