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

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

>>> On 6/12/2015 at 03:39 PM, in message
<557AFD2F02000066000D4C7D@xxxxxxxxxxxxxxxxxxxxxxx>, "Chun Yan Liu"
<cyliu@xxxxxxxx> wrote: 

>  
>>>> 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? 

Saw the discussion thread and got it. Will update the codes.

>  
> > 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 
>  
>  


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