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

[Jim Fehlig <jfehlig@xxxxxxxx>]

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@xxxxxxxx>
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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list