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] libxl: make libxl c ommunicate with xenstore

To: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
Subject: RE: ð´: [Xen-devel] [PATCH] libxl: make libxl c ommunicate with xenstored by socket or xenb us driver
From: "Jun Zhu (Intern)" <Jun.Zhu@xxxxxxxxxx>
Date: Wed, 8 Sep 2010 17:53:30 +0100
Accept-language: en-US
Acceptlanguage: en-US
Cc: Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Wed, 08 Sep 2010 09:58:38 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <19591.44117.955603.913010@xxxxxxxxxxxxxxxxxxxxxxxx>
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>
References: <433DDF91DFB08148BAD3FDB6FDDA314C9F35F3BB57@xxxxxxxxxxxxxxxxxxxxxxxxx> <1283439564.12544.9720.camel@xxxxxxxxxxxxxxxxxxxxxx> <433DDF91DFB08148BAD3FDB6FDDA314C9F35F3BB5B@xxxxxxxxxxxxxxxxxxxxxxxxx>, <19591.44117.955603.913010@xxxxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
Thread-index: ActPauaf5GcFKe3KRiq74Ay+/5CwegAC3clp
Thread-topic: ð´: [Xen-devel] [PATCH] libxl: make libxl c ommunicate with xenstored by socket or xenb us driver
> It's not clear to me why you're moving the open call about. 
This is because the current version does not invoke xs_daemon_close(xsh) if the 
asprintf is return.

> this function always seems to leak kvs[0]
kvs[1]?

I will try to make a new path according to your suggestion. Thanks!


Jun Zhu
Citrix Systems UK
________________________________________
From: Ian Jackson
Sent: 08 September 2010 11:31
To: Jun Zhu (Intern)
Cc: Ian Campbell; xen-devel@xxxxxxxxxxxxxxxxxxx
Subject: Re: ð´: [Xen-devel] [PATCH] libxl: make libxl c        ommunicate with 
xenstored by socket or xenb     us driver

Jun Zhu (Intern) writes ("ð´: [Xen-devel] [PATCH] libxl: make libxl c 
ommunicate with xenstored by socket or xenb us driver"):
> libxl uses socket or xenbus dev to communicate with xenstored.

Thanks for this patch.  I have a few comments:

> @@ -1387,10 +1385,8 @@
>  {
>      libxl_device_model_starting *starting = for_spawn;
>      char *kvs[3];
> -    int rc;
>      struct xs_handle *xsh;
>
> -    xsh = xs_daemon_open();
>      /* we mustn't use the parent's handle in the child */
>
>      kvs[0] = "image/device-model-pid";
> @@ -1398,10 +1394,11 @@
>          return;
>      kvs[2] = NULL;
>
> -    rc = xs_writev(xsh, XBT_NULL, starting->dom_path, kvs);
> -    if (rc)
> -        return;
> -    xs_daemon_close(xsh);
> +    xsh = libxl_xs_open();
> +    if (!xsh)
> +     fprintf(stderr, "cannot connect to xenstore");
> +    xs_writev(xsh, XBT_NULL, starting->dom_path, kvs);
> +    libxl_xs_close(xsh);
>  }

It's not clear to me why you're moving the open call about.  Your
error handling is incorrect, as passing a null xsh to xs_writev will
cause a segfault.  Also, while you're there you should probably fix
the memory leak: this function always seems to leak kvs[0].

> -    xsh = xs_daemon_open();
> +    xsh = libxl_xs_open();
> +    if (!xsh)
> +        XL_LOG_ERRNOVAL(ctx, XL_LOG_ERROR, errno,
> +             "cannot connect to xenstore");

Logging is not sufficient; you need to set rc and "goto out", or
return an error code, too.  This pattern occurs a number of times in
your patch.

Finally, your patch seems to have been truncated at some point.
patch(1) complains:
  patch: **** malformed patch at line 172:

Thanks,
Ian.

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