Re: [Xen-devel] [PATCH V4 3/7] libxl: add pvusb API

["Chun Yan Liu" <cyliu@xxxxxxxx>]

>>> On 6/12/2015 at 12:42 AM, in message
<21881.47707.526863.158586@xxxxxxxxxxxxxxxxxxxxxxxx>, Ian Jackson
<Ian.Jackson@xxxxxxxxxxxxx> wrote: 
> Chunyan Liu writes ("[Xen-devel] [PATCH V4 3/7] libxl: add pvusb API"): 
> > Add pvusb APIs, including: 
> ... 
>  
> Thanks for your contribution.  I'm afraid I haven't had time to 
> completely finish my review this, but here's what I have: 
>  
> > --- /dev/null 
> > +++ b/docs/misc/pvusb.txt 
> > @@ -0,0 +1,243 @@ 
> > +1. Overview 
>  
> It's good that you have provided documentation.  But I think this 
> document is a bit confused about its audience. 
>  
> Information about design choices should be removed from this user- and 
> application-facing document, and put in comments in the code, or 
> commit messages, I think. 

Thanks. Will update.

>  
>  
> > +int libxl_device_usbctrl_getinfo(libxl_ctx *ctx, uint32_t domid, 
> > +                                libxl_device_usbctrl *usbctrl, 
> > +                                libxl_usbctrlinfo *usbctrlinfo) 
> > +                                LIBXL_EXTERNAL_CALLERS_ONLY; 
>  
> Why is this function marked LIBXL_EXTERNAL_CALLERS_ONLY ? 

Currently libxl itself won't call it. Exposed for toolstack usage. Will
remove that if it's not proper.

>  
> > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h 
> > index ef7aa1d..89a9f07 100644 
> > --- a/tools/libxl/libxl_internal.h 
> > +++ b/tools/libxl/libxl_internal.h 
> > @@ -2439,6 +2439,18 @@ _hidden void libxl__device_vtpm_add(libxl__egc *egc, 
> >  
> uint32_t domid, 
> ... 
> > +/* AO operation to connect a PVUSB controller. 
> > + * Adding a PVUSB controller will add a 'vusb' device entry in xenstore, 
> > + * and will wait for device connection. 
>  
> In this context I think "will wait for device connection" is 
> misleading.  What you mean is that the vusb is available for adding 
> devices to, but won't have any yet.  Nothing in libxl is "waiting". 

Here I mean libxl_wait_for_device_connection. Since it adds a new device entry
to xenstore, it needs to wait for a moment for frontend/backend connection.

>  
> > diff --git a/tools/libxl/libxl_pvusb.c b/tools/libxl/libxl_pvusb.c 
> > new file mode 100644 
> > index 0000000..a6e1aa1 
> > --- /dev/null 
> > +++ b/tools/libxl/libxl_pvusb.c 
> > @@ -0,0 +1,1249 @@ 
> ... 
> > +void libxl__device_usbctrl_add(libxl__egc *egc, uint32_t domid, 
> > +                               libxl_device_usbctrl *usbctrl, 
> > +                               libxl__ao_device *aodev) 
> > +{ 
> > +    STATE_AO_GC(aodev->ao); 
> ... 
> > +    libxl_domain_config_init(&d_config); 
> > +    libxl_device_usbctrl_init(&usbctrl_saved); 
> > +    libxl_device_usbctrl_copy(CTX, &usbctrl_saved, usbctrl); 
>  
> Wei will need to review the saved/live saved device info handling, and 
> the json, etc. 
>  
> > +static int 
> > +libxl_device_usbctrl_remove_common(libxl_ctx *ctx, uint32_t domid, 
> > +                                   libxl_device_usbctrl *usbctrl, 
> > +                                   const libxl_asyncop_how *ao_how, 
> > +                                   int force) 
>  
> As discussed, you mustn't call this within libxl.
I don't quite understand why. I guess it's the same as usb_add problem,
something related to embedded ao?
As in usb_add:
libxl_device_usb_add()
   AO_CREATE(ctx, domid, ao_how)
   libxl__device_usb_add()
     libxl__device_usb_setdefault()
       libxl_device_usbctrl_add_common()
         AO_CREATE(ctx, domid, NULL)
if this is not allowed, what is the correct way?

> If you need to, you 
> need to break it out into an internal function 
> (libxl__initiate_device_usbctrl_remove, maybe) which makes a callback 
> when done.
>  
> > +libxl_device_usbctrl * 
> > +libxl_device_usbctrl_list(libxl_ctx *ctx, uint32_t domid, int *num) 
> > +{ 
> > +    GC_INIT(ctx); 
> > +    libxl_device_usbctrl *usbctrls = NULL; 
> > +    char *fe_path = NULL; 
> > +    char **dir = NULL; 
> > +    unsigned int ndirs = 0; 
> > + 
> > +    *num = 0; 
> > + 
> > +    fe_path = GCSPRINTF("%s/device/vusb", 
> > +                        libxl__xs_get_dompath(gc, domid)); 
> > +    dir = libxl__xs_directory(gc, XBT_NULL, fe_path, &ndirs); 
> > + 
> > +    if (dir && ndirs) { 
> > +        usbctrls = malloc(sizeof(*usbctrls) * ndirs); 
>  
> Please use libxl__calloc with NOGC. 

Thanks. Missing this one.

>  
> > +        libxl_device_usbctrl* usbctrl; 
> > +        libxl_device_usbctrl* end = usbctrls + ndirs; 
> > +        for (usbctrl = usbctrls; usbctrl < end; usbctrl++, dir++, 
> > (*num)++)  
> { 
> > +            char *tmp; 
> > +            const char *be_path = libxl__xs_read(gc, XBT_NULL, 
> > +                                    GCSPRINTF("%s/%s/backend", fe_path,  
> *dir)); 
> > + 
> > +            libxl_device_usbctrl_init(usbctrl); 
> > +            usbctrl->devid = atoi(*dir); 
>  
> This function (and the corresponding writing code) is quite formulaic. 
> Perhaps a macro or something could be used. 
>  
> I would make a similar criticism of libxl_device_usbctrl_getinfo. 
>  
> > +/* check if USB device is already assigned to a domain */ 
> > +static bool is_usb_assigned(libxl__gc *gc, libxl_device_usb *usb) 
> > +{ 
> > +    libxl_device_usb *usbs; 
> > +    int rc, num; 
> > + 
> > +    rc = libxl__device_usb_assigned_list(gc, &usbs, &num); 
> > +    if (rc) { 
> > +        LOG(ERROR, "Fail to get assigned usb list"); 
> > +        return true; 
>  
> I don't think this is proper error handling.  You need to either 
> encode the boolean return value in the error code, or have the boolean 
> result be a reference parameter. 

Will improve that.

Thanks,
Chunyan

>  
>  
> Thanks, 
> Ian. 
>  
> _______________________________________________ 
> Xen-devel mailing list 
> Xen-devel@xxxxxxxxxxxxx 
> http://lists.xen.org/xen-devel 
>  
>  


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel