[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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/


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]