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

[Daniel Veillard]

On Thu, Jan 24, 2013 at 12:45:21PM -0700, Jim Fehlig wrote:
> 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 at suse.com>
> 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
> 

  ACK,

Daniel

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