Re: [libvirt] [PATCH 6/6] libxl: Find domain object in event handler

Jim Fehlig wrote: > Since libxl provides the domain ID in the event handler callback, > find the domain object based on the ID. This approach prevents > processing the callback on a domain that has already been reaped. > --- > src/libxl/libxl_driver.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c > index 7484b83..e28b641 100644 > --- a/src/libxl/libxl_driver.c > +++ b/src/libxl/libxl_driver.c > @@ -656,22 +656,22 @@ libxlVmReap(libxlDriverPrivatePtr driver, > * Handle previously registered event notification from libxenlight > */ > static void > -libxlEventHandler(void *data, const libxl_event *event) > +libxlEventHandler(void *data ATTRIBUTE_UNUSED, const libxl_event *event) > { > libxlDriverPrivatePtr driver = libxl_driver; > - virDomainObjPtr vm = data; > + virDomainObjPtr vm = NULL; > virDomainEventPtr dom_event = NULL; > > libxlDriverLock(driver); > On xen-unstable, I noticed an occasional deadlock here when libxl invokes the event handler with a SUSPEND shutdown reason. The driver lock is already held by the caller of libxl_domain_suspend(). Inspired by the xl implementation, I've updated this patch to ignore the SUSPEND shutdown reason before acquiring the driver lock. Regards, Jim

>From b9b3cd0259168ccf10f525d1b459bfe790162be3 Mon Sep 17 00:00:00 2001 From: Jim Fehlig <jfehlig(a)suse.com&gt; Date: Mon, 21 Jan 2013 10:36:03 -0700 Subject: [PATCH 6/6] libxl: Domain event handler improvements Since libxl provides the domain ID in the event handler callback, find the domain object based on the ID. This approach prevents processing the callback on a domain that has already been reaped. Also, similar to the xl implementation, ignore the SUSPEND shutdown reason. By calling libxl_domain_suspend(), we know a shutdown event with SUSPEND reason will be generated, but it can be safely ignored since any subsequent cleanup will be done by the callers. --- src/libxl/libxl_driver.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 7484b83..39875aa 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -656,26 +656,32 @@ libxlVmReap(libxlDriverPrivatePtr driver, * Handle previously registered event notification from libxenlight */ static void -libxlEventHandler(void *data, const libxl_event *event) +libxlEventHandler(void *data ATTRIBUTE_UNUSED, const libxl_event *event) { libxlDriverPrivatePtr driver = libxl_driver; - virDomainObjPtr vm = data; + virDomainObjPtr vm = NULL; virDomainEventPtr dom_event = NULL; - - libxlDriverLock(driver); - virObjectLock(vm); - libxlDriverUnlock(driver); + libxl_shutdown_reason xl_reason = event->u.domain_shutdown.shutdown_reason; if (event->type == LIBXL_EVENT_TYPE_DOMAIN_SHUTDOWN) { virDomainShutoffReason reason; - if (event->domid != vm->def->id) + /* Similar to the xl implementation, ignore SUSPEND. Any actions needed + after calling libxl_domain_suspend() are handled by it's callers. */ + if (xl_reason == LIBXL_SHUTDOWN_REASON_SUSPEND) + goto cleanup; + + libxlDriverLock(driver); + vm = virDomainFindByID(&driver->domains, event->domid); + libxlDriverUnlock(driver); + + if (!vm) goto cleanup; - switch (event->u.domain_shutdown.shutdown_reason) { + switch (xl_reason) { case LIBXL_SHUTDOWN_REASON_POWEROFF: case LIBXL_SHUTDOWN_REASON_CRASH: - if (event->u.domain_shutdown.shutdown_reason == LIBXL_SHUTDOWN_REASON_CRASH) { + if (xl_reason == LIBXL_SHUTDOWN_REASON_CRASH) { dom_event = virDomainEventNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, VIR_DOMAIN_EVENT_STOPPED_CRASHED); @@ -694,7 +700,7 @@ libxlEventHandler(void *data, const libxl_event *event) libxlVmStart(driver, vm, 0, -1); break; default: - VIR_INFO("Unhandled shutdown_reason %d", event->u.domain_shutdown.shutdown_reason); + VIR_INFO("Unhandled shutdown_reason %d", xl_reason); break; } } -- 1.8.0.1