Re: ð´: [Xen-devel] [PATCH] libxl: make libxl c ommunicate with xenstored by socket or xenb us driver

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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