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

Re: [libvirt] [PATCH] Implement dispose method for libxlDomainObjPrivate



Frediano Ziglio wrote:
> When creating a timer/event handler reference counting is used. So it could
> be possible (in theory) that libxlDomainObjPrivateFree is called with
> reference counting >1. The problem is that libxlDomainObjPrivateFree leave
> the object in an invalid state with ctx freed (but still having dandling
> pointer). This can lead timer/event handler to core.
>   

Thanks for the analysis and the patch!

> This patch destroy the object before disposing it so at timer/event object
> is still valid.
>   

I'm not sure that is a good description of the fix :). IMO, it would be
clearer to say

This patch implements a dispose method for libxlDomainObjPrivate, and
moves freeing the libxl ctx to the dispose method, ensuring the ctx is
valid while the object's reference count is > 0.

> Signed-off-by: Frediano Ziglio <frediano ziglio citrix com>
> ---
>  src/libxl/libxl_driver.c |   20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 935919b..1c8cfd7 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -110,6 +110,8 @@ static int
>  libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>               bool start_paused, int restore_fd);
>  
> +static void libxlDomainObjPrivateDispose(void *obj);
> +
>   

libvirt uses the pattern

static void
libxlDomainObjPrivateDispose(void *obj);

I've fixed these small nits and pushed the patch.

Regards,
Jim

>  /* Function definitions */
>  static int
>  libxlDomainObjPrivateOnceInit(void)
> @@ -117,7 +119,7 @@ libxlDomainObjPrivateOnceInit(void)
>      if (!(libxlDomainObjPrivateClass = virClassNew(virClassForObjectLockable(),
>                                                     "libxlDomainObjPrivate",
>                                                     sizeof(libxlDomainObjPrivate),
> -                                                   NULL)))
> +                                                   libxlDomainObjPrivateDispose)))
>          return -1;
>  
>      return 0;
> @@ -418,14 +420,26 @@ libxlDomainObjPrivateAlloc(void)
>  }
>  
>  static void
> -libxlDomainObjPrivateFree(void *data)
> +libxlDomainObjPrivateDispose(void *obj)
>  {
> -    libxlDomainObjPrivatePtr priv = data;
> +    libxlDomainObjPrivatePtr priv = obj;
>  
>      if (priv->deathW)
>          libxl_evdisable_domain_death(priv->ctx, priv->deathW);
>  
>      libxl_ctx_free(priv->ctx);
> +}
> +
> +static void
> +libxlDomainObjPrivateFree(void *data)
> +{
> +    libxlDomainObjPrivatePtr priv = data;
> +
> +    if (priv->deathW) {
> +        libxl_evdisable_domain_death(priv->ctx, priv->deathW);
> +        priv->deathW = NULL;
> +    }
> +
>      virObjectUnref(priv);
>  }
>  
>   


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