Re: [Xen-devel] [PATCH RFC V2 3/5] libxl: add pvusb API

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

>>> On 3/3/2015 at 07:38 PM, in message <1425382696.24959.112.camel@xxxxxxxxxx>,
Ian Campbell <ian.campbell@xxxxxxxxxx> wrote: 
> On Mon, 2015-01-19 at 16:28 +0800, Chunyan Liu wrote: 
> > Add pvusb APIs, including: 
> >  - attach/detach (create/destroy) virtual usb controller. 
> >  - attach/detach usb device 
> >  - list assignable usb devices in host 
> >  - some other helper functions 
> >  
> > Signed-off-by: Chunyan Liu <cyliu@xxxxxxxx> 
> > Signed-off-by: Simon Cao <caobosimon@xxxxxxxxx> 
> > --- 
> >  tools/libxl/Makefile         |    2 +- 
> >  tools/libxl/libxl.c          |    2 + 
> >  tools/libxl/libxl.h          |   58 ++ 
> >  tools/libxl/libxl_internal.h |    6 + 
> >  tools/libxl/libxl_usb.c      | 1277  
> ++++++++++++++++++++++++++++++++++++++++++ 
> >  tools/libxl/libxlu_cfg_y.c   |  464 ++++++++------- 
> >  tools/libxl/libxlu_cfg_y.h   |   38 +- 
> >  7 files changed, 1623 insertions(+), 224 deletions(-) 
> >  create mode 100644 tools/libxl/libxl_usb.c 
> >  
> > diff --git a/tools/libxl/Makefile b/tools/libxl/Makefile 
> > index b417372..08cdb12 100644 
> > --- a/tools/libxl/Makefile 
> > +++ b/tools/libxl/Makefile 
> > @@ -95,7 +95,7 @@ LIBXL_OBJS = flexarray.o libxl.o libxl_create.o  
> libxl_dm.o libxl_pci.o \ 
> >                     libxl_internal.o libxl_utils.o libxl_uuid.o \ 
> >                     libxl_json.o libxl_aoutils.o libxl_numa.o \ 
> >                     libxl_save_callout.o _libxl_save_msgs_callout.o \ 
> > -                   libxl_qmp.o libxl_event.o libxl_fork.o $(LIBXL_OBJS-y) 
> > +                   libxl_qmp.o libxl_event.o libxl_fork.o libxl_usb.o 
> > $(LIBXL_OBJS-y) 
>  
> The contents of that file looks Linux specific, so I think you might 
> have to arrange for it to only be built there and also to provide a 
> libxl_nousb.c with stubs returning appropriate errors to be used on 
> other platforms. 
>  
> Or it may be possible (even better) to refactor the linux specific bits 
> of libxl_usb.c into libxl_linux.c and leave the common stuff behind. 
>  
> I thought libxl_pci.c would be a good example of this, but it doesn't 
> seem to have any conditional stuff, yet I expected it to be Linux 
> specific. I've no idea how that works :-(. Maybe usb can get away with 
> it too. 
>  
> > +int libxl_intf_to_device_usb(libxl_ctx *ctx, uint32_t domid, 
> > +                            char *intf, libxl_device_usb *usb) 
> > +                            LIBXL_EXTERNAL_CALLERS_ONLY; 
>  
> I wasn't sure what an "intf" was on patch #1. hopefully your answer 
> there will help me understand what this is for! 

As in patch#1, it means syfs interface for the usb device, like: 2-1.6.
This function will be used when detaching a usb device, use indicates
2-1.6, it will return the corresponding libxl_device_usb structure.

>  
> > diff --git a/tools/libxl/libxl_usb.c b/tools/libxl/libxl_usb.c 
> > new file mode 100644 
> > index 0000000..830a846 
> > --- /dev/null 
> > +++ b/tools/libxl/libxl_usb.c 
>  
> Apart from my not yet understanding the interface semantics and the 
> potential for Linux-specificness mentioned above the actual code here 
> looks reasonably OK to me. I have smaller and larger comments below 
> though. 
>  
> > +static int libxl__device_usbctrl_setdefault(libxl__gc *gc, uint32_t domid, 
> > +                                            libxl_device_usbctrl *usbctrl) 
> > +{ 
> [...] 
> > +    if(!usbctrl->backend_domid) 
>  
> Missing space before (. 
>  
>  
> > +static int libxl__usbctrl_add_xenstore(libxl__gc *gc, uint32_t domid, 
> > +                                       libxl_device_usbctrl *usbctrl) 
> > +{ 
> > +    libxl_ctx *ctx = libxl__gc_owner(gc); 
> > +    flexarray_t *front; 
> > +    flexarray_t *back; 
> > +    libxl__device *device; 
> > +    xs_transaction_t tran; 
> > +    int rc = 0; 
> > + 
> > +    GCNEW(device); 
> > +    rc = libxl__device_from_usbctrl(gc, domid, usbctrl, device); 
> > +    if (rc) goto out; 
> > + 
> > +    front = flexarray_make(gc, 4, 1); 
> > +    back = flexarray_make(gc, 12, 1); 
> > + 
> > +    flexarray_append(back, "frontend-id"); 
> > +    flexarray_append(back, libxl__sprintf(gc, "%d", domid)); 
>  
> GCSPRINTF would be good for all of these (and in lots of other places 
> too).
>  
> flexarray_append_pair is also nice for adding key+value at the same time 
> since it makes it easier to see what goes together. 

Got it. Will update all in next version.

>  
> > +retry_transaction: 
> > +    tran = xs_transaction_start(ctx->xsh); 
>  
> Please follow the design pattern in e.g. libxl__device_vtpm_add or 
> device_disk_add and use libxl__xs_transaction_start/commit/abort here 
> inside a for (;;). 

OK. Got it.

>  
> > +static int libxl__device_usbctrl_add(libxl__gc *gc, uint32_t domid, 
> > +                                     libxl_device_usbctrl *usbctrl) 
> > +{ 
> > [...] 
> > +    if (libxl__usbctrl_add_xenstore(gc, domid, usbctrl) < 0){ 
>  
> Space before { please. 
>  
> > +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) 
> > +{ 
> > [...] 
> > +    for (i = 0; i < numusb; i++) { 
> > +        if (libxl__device_usb_remove_common(gc, domid, &usbs[i], 0)) { 
> > +            fprintf(stderr, "libxl_device_usb_remove failed.\n"); 
>  
> Use LOG*( please, not fprintf (this is true everywhere in libxl in case 
> I missed any other). 
>  
> > +/* usb device functions */ 
> > + 
> > +/* Following functions are to get assignable usb devices */ 
> > +static int 
> > +libxl__device_usb_assigned_list(libxl__gc *gc, 
> > +                                libxl_device_usb **list, int *num) 
> > +{ 
> > +    char **domlist; 
> > +    unsigned int nd = 0, i, j; 
> > +    char *be_path; 
> > +    libxl_device_usb *usb; 
> > + 
> > +    *list = NULL; 
> > +    *num = 0; 
> > + 
> > +    domlist = libxl__xs_directory(gc, XBT_NULL, "/local/domain", &nd); 
> > +    be_path = libxl__sprintf(gc,"/local/domain/0/backend/vusb"); 
>  
> GCSPRINTF. 
>  
> Also I notice a hardcoded 0, shouldn't that be backend_domid somehow? 

Yes, it is. Will update. Thanks for pointing out.

> I've seen a few /0/ hardcoded here and there in this patch, which if not 
> backend_domid perhaps ought to at least be LIBXL_TOOLSTACK_DOMID. 
>  
> If not then is it necessary to dup the string, can't it just be a const 
> string literal? 
>  
> > +static bool is_usb_assignable(libxl__gc *gc, char *intf) 
> > +{ 
> > +    char buf[5]; 
> > + 
> > +    if (get_usb_bDeviceClass(gc, intf, buf) < 0) 
> > +        return false; 
> > + 
> > +    if (strcmp(buf, USBHUB_CLASS_CODE)) 
> > +        return false; 
>  
>  
> OOI are hubs inherently unassignable, or is that a short coming of the 
> current driver code? 

It's right. I'm very sorry here and other few places out of my mistake,
some changes not included in this patch when sending. I noticed that
when sending these patches to Juergen for his kernel patches testing.
Will send the correct ones in next version.

>  
> > +/* get all usb devices of the domain */ 
> > +static libxl_device_usb * 
> > +libxl_device_usb_list_all(libxl__gc *gc, uint32_t domid, int *num) 
>  
> Should be in libxl__ namespace, or since it is static you could omit the 
> namespacing completely if you prefer. 
>  
> > +/* xenstore usb data */ 
> > +static int libxl__device_usb_add_xenstore(libxl__gc *gc, uint32_t domid, 
> > +                                          libxl_device_usb *usb) 
> > +{ 
> > +    libxl_ctx *ctx = CTX; 
>  
> FYI although it's not required, you can just use CTX in the code if you 
> prefer. 
>  
> > [...] 
> > +    be_path = libxl__sprintf(gc, "%s/port/%d", be_path, usb->port); 
> > +    LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Adding new usb device to  
> xenstore"); 
>  
> The LOG*( macros will help shorten this line. 
>  
> > +    path = libxl__sprintf(gc, "cat "SYSFS_USB_DEVS_PATH"/%s/manufacturer", 
> >  
> intf); 
>  
> Whoah, what now? 
>  
> What you want here is to open the fd and use read on it. We may even
 
Yes. I also think it's odd and so already updated this kinds of places locally
for Juergen's testing. Will be posted in next version. Thanks.
 
> have existing helpers for doing so. Certainly using popen on a string 
> starting with cat isn't what is wanted (at least not without a big and 
> convincing comment explaining why this should be needed). 
>  
> I see a bunch of this in this area. 
>  
> > +int libxl_device_usb_getinfo(libxl_ctx *ctx, char *intf, libxl_usbinfo  
> *usbinfo) 
> > +{ 
> > +    GC_INIT(ctx); 
> > +    char buf[512]; 
> > + 
> > +    if (!get_usb_devnum(gc, intf, buf) ) 
>  
> In consistent spacing. 
>  
> > +        usbinfo->devnum = atoi(buf); 
> > + 
> > +    if ( !get_usb_busnum(gc, intf, buf)) 
> > +        usbinfo->bus = atoi(buf); 
> > + 
> > +    if (!get_usb_idVendor(gc, intf, buf) ) 
> > +        usbinfo->idVendor = atoi(buf); 
> > + 
> > +    if (!get_usb_idProduct(gc, intf, buf) ) 
> > +        usbinfo->idProduct  = atoi(buf); 
> > + 
> > +    if (!get_usb_manufacturer(gc, intf, buf) ) 
> > +        usbinfo->manuf = strdup(buf); 
>  
> libxl_strdup with NOGC to get correct error handling please.

Got it. Will update.

- Chunyan
 
>  
> > + 
> > +    if (!get_usb_product(gc, intf, buf) ) 
> > +        usbinfo->prod = strdup(buf); 
> > + 
> > +    GC_FREE; 
> > +    return 0; 
> > +} 
> > + 
>  
>  
>  


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