Re: [libvirt] [PATCH 4/6] libxl: Explicitly remove timeouts

25 Jan 2013

On Mon, Jan 21, 2013 at 12:22:22PM -0700, Jim Fehlig wrote:
...
I've noticed that libxl can invoke timeout reregister/modify hooks
after returning from libxl_ctx_free.  Explicitly remove the
timeouts before freeing the libxl ctx to avoid executing hooks on
stale objects.
---
 src/libxl/libxl_conf.h   |  6 ++++
 src/libxl/libxl_driver.c | 71 +++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
index 3d5a461..f6167e6 100644
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -79,6 +79,9 @@ struct _libxlDriverPrivate {
     char *saveDir;
 };
+typedef struct _libxlEventHookInfo libxlEventHookInfo;
+typedef libxlEventHookInfo *libxlEventHookInfoPtr;
+
 typedef struct _libxlDomainObjPrivate libxlDomainObjPrivate;
 typedef libxlDomainObjPrivate *libxlDomainObjPrivatePtr;
 struct _libxlDomainObjPrivate {
@@ -87,6 +90,9 @@ struct _libxlDomainObjPrivate {
     /* per domain libxl ctx */
     libxl_ctx *ctx;
     libxl_evgen_domain_death *deathW;
+
+    /* list of libxl timeout registrations */
+    libxlEventHookInfoPtr timerRegistrations;
 };
# define LIBXL_SAVE_MAGIC "libvirt-xml\n \0 \r"
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 530a17f..08dffd6 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -59,10 +59,39 @@
 /* Number of Xen scheduler parameters */
 #define XEN_SCHED_CREDIT_NPARAM   2
+/* Append an event registration to the list of registrations */
+#define LIBXL_EV_REG_APPEND(head, add)                 \
+    do {                                               \
+        libxlEventHookInfoPtr temp;                    \
+        if (head) {                                    \
+            temp = head;                               \
+            while (temp->next)                         \
+                temp = temp->next;                     \
+            temp->next = add;                          \
+        } else {                                       \
+            head = add;                                \
+        }                                              \
+    } while (0)
+
+/* Remove an event registration from the list of registrations */
+#define LIBXL_EV_REG_REMOVE(head, del)                 \
+    do {                                               \
+        libxlEventHookInfoPtr temp;                    \
+        if (head == del) {                             \
+            head = head->next;                         \
+        } else {                                       \
+            temp = head;                               \
+            while (temp->next && temp->next != del)    \
+                temp = temp->next;                     \
+            if (temp->next) {                          \
+                temp->next = del->next;                \
+            }                                          \
+        }                                              \
+    } while (0)
+
 /* Object used to store info related to libxl event registrations */
-typedef struct _libxlEventHookInfo libxlEventHookInfo;
-typedef libxlEventHookInfo *libxlEventHookInfoPtr;
 struct _libxlEventHookInfo {
+    libxlEventHookInfoPtr next;
     libxlDomainObjPrivatePtr priv;
     void *xl_priv;
     int id;
@@ -225,7 +254,11 @@ libxlTimerCallback(int timer ATTRIBUTE_UNUSED, void *timer_info)
     virObjectUnlock(p);
     libxl_osevent_occurred_timeout(p->ctx, info->xl_priv);
     virObjectLock(p);
-    virEventRemoveTimeout(info->id);
+    /* timeout could have been freed while the lock was dropped.
+       Only remove it from the list if it still exists.
+    */
+    if (virEventRemoveTimeout(info->id) == 0)
+        LIBXL_EV_REG_REMOVE(p->timerRegistrations, info);
     virObjectUnlock(p);
 }
@@ -269,6 +302,9 @@ libxlTimeoutRegisterEventHook(void *priv,
        event objects. */
     virObjectRef(info->priv);
+    virObjectLock(info->priv);
+    LIBXL_EV_REG_APPEND(info->priv->timerRegistrations, info);
+    virObjectUnlock(info->priv);
     info->xl_priv = xl_priv;
     *hndp = info;
@@ -308,10 +344,35 @@ libxlTimeoutDeregisterEventHook(void *priv ATTRIBUTE_UNUSED,
     libxlDomainObjPrivatePtr p = info->priv;
virObjectLock(p);
-    virEventRemoveTimeout(info->id);
+    /* Only remove the timeout from the list if removal from the
+       event loop is successful.
+    */
+    if (virEventRemoveTimeout(info->id) == 0)
+        LIBXL_EV_REG_REMOVE(p->timerRegistrations, info);
     virObjectUnlock(p);
 }
+static void
+libxlRegisteredTimeoutsCleanup(libxlDomainObjPrivatePtr priv)
+{
+    libxlEventHookInfoPtr info;
+
+    virObjectLock(priv);
+    info = priv->timerRegistrations;
+    while (info) {
+        /* libxl expects the event to be deregistered when calling
+           libxl_osevent_occurred_timeout, but we dont want the event info
+           destroyed.  Disable the timeout and only remove it after returning
+           from libxl. */
+        virEventUpdateTimeout(info->id, -1);
+        libxl_osevent_occurred_timeout(priv->ctx, info->xl_priv);
+        virEventRemoveTimeout(info->id);
+        info = info->next;
+    }
+    priv->timerRegistrations = NULL;
+    virObjectUnlock(priv);
+}
+
 static const libxl_osevent_hooks libxl_event_callbacks = {
     .fd_register = libxlFDRegisterEventHook,
     .fd_modify = libxlFDModifyEventHook,
@@ -565,6 +626,8 @@ libxlVmCleanup(libxlDriverPrivatePtr driver,
         vm->def->id = -1;
         vm->newDef = NULL;
     }
+
+    libxlRegisteredTimeoutsCleanup(priv);
 }
/*
ACK, just one small nit, usually we format multi-line comments as

/*
 * bla
 * bla
 */

  could you update your patches accordingly :-) ?

    thanks !

Daniel

-- 
Daniel Veillard      | Open Source and Standards, Red Hat
veillard@redhat.com  | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | virtualization library  http://libvirt.org/

    