WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

RE: [Xen-devel] [PATCH V5] libxl: make libxl communicate with xenstored

To: "Jun Zhu (Intern)" <Jun.Zhu@xxxxxxxxxx>
Subject: RE: [Xen-devel] [PATCH V5] libxl: make libxl communicate with xenstored by socket or xenbus driver
From: Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>
Date: Fri, 17 Sep 2010 15:27:08 +0100
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
Delivery-date: Fri, 17 Sep 2010 07:27:44 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <433DDF91DFB08148BAD3FDB6FDDA314C9F35F3BB70@xxxxxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: Citrix Systems, Inc.
References: <433DDF91DFB08148BAD3FDB6FDDA314C9F35F3BB70@xxxxxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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