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

RE: ð´: [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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.