RE: [Xen-devel] [PATCH V5] libxl: make libxl communicate with xenstored by socket or xenbus driver

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Fri, 2010-09-17 at 15:02 +0100, Jun Zhu (Intern) wrote:
> This version adds gc as a parameter to libxl__xs_open, and uses 
> LIBXL__LOG_ERRNO for logging. If some functions cannot use the ctx, It should 
> transfer NULL to libxl__xs_open to disable logging. (But it will make users 
> difficult to find the problem when no logging is output.)
> To make consistent with other functions in libxl__xshelp.c, I use gc as its 
> parameter, not ctx. In the libxl_ctx_init function, I add âlibxl__gc gc = 
> LIBXL_INIT_GC(ctx)â to get the gc of ctx. Please check this.
> 
> Signed-off-by: Jun Zhu <Jun.Zhu@xxxxxxxxxx>
> 
> diff -r cca905a429aa tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c     Tue Sep 14 15:39:36 2010 +0100
> +++ b/tools/libxl/libxl.c     Fri Sep 17 14:58:50 2010 +0100
> @@ -40,6 +40,8 @@
>  
>  int libxl_ctx_init(libxl_ctx *ctx, int version, xentoollog_logger *lg)
>  {
> +    libxl__gc gc = LIBXL_INIT_GC(ctx);
> +

If you add one of these then you also need to add a libxl__free_all(&gc)
on every exit path from the function.

Special care needs to be taken in libxl_ctx_init to ensure the ctx is
initialised enough to be able to create a gc. I suspect it is safe as is
but perhaps moving the "gc = LIBXL_INIT_GC(ctx)" later in the function
will prevent accidents? e.g. put it just before this:

+    ctx->xsh = libxl__xs_open(&gc);
     if (!ctx->xsh) {
         xc_interface_close(ctx->xch);
         return ERROR_FAIL;


> diff -r cca905a429aa tools/libxl/libxl_dom.c
> --- a/tools/libxl/libxl_dom.c Tue Sep 14 15:39:36 2010 +0100
> +++ b/tools/libxl/libxl_dom.c Fri Sep 17 14:58:50 2010 +0100
> @@ -326,14 +326,16 @@
>  
>      snprintf(path, sizeof(path), 
> "/local/domain/0/device-model/%u/logdirty/cmd", domid);
>  
> -    xsh = xs_daemon_open();
> +    xsh = libxl__xs_open(NULL);
> +    if (!xsh)
> +        return;

This is libxl__domain_suspend_common_switch_qemu_logdirty[0] which is
used as a callback from xc_domain_suspend. IMHO any function which takes
a callback should also take a void * closure, which in this case could
be used to pass the context from the caller of xc_domain_save to this
function. Given that xc does not do this I suppose this is fine as is.

[0] BTW please add this to your ~/.hgrc to automatically annotate
functions in diffs:
"""
        [diff]
        showfunc = True
"""

> diff -r cca905a429aa tools/libxl/libxl_utils.c
> --- a/tools/libxl/libxl_utils.c       Tue Sep 14 15:39:36 2010 +0100
> +++ b/tools/libxl/libxl_utils.c       Fri Sep 17 14:58:50 2010 +0100
> @@ -366,9 +366,11 @@
>  
> 
>  int libxl_ctx_postfork(libxl_ctx *ctx) {
> +    libxl__gc gc = LIBXL_INIT_GC(ctx);
>      if (ctx->xsh) xs_daemon_destroy_postfork(ctx->xsh);
> -    ctx->xsh = xs_daemon_open();
> -    if (!ctx->xsh) return ERROR_FAIL;
> +    ctx->xsh = libxl__xs_open(&gc);
> +    if (!ctx->xsh)
> +        return ERROR_FAIL;
>      return 0;
>  }
 
Another place where libxl__free_all(&gc) is needed.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel