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

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

>>> On 11/13/2015 at 01:27 AM, in message
<CAFLBxZawnBaQ791ebbv1bmw3DX2+cWfk8QD1JdAVopk0-Ma6Zw@xxxxxxxxxxxxxx>, George
Dunlap <George.Dunlap@xxxxxxxxxxxxx> wrote: 
> On Wed, Oct 21, 2015 at 10:08 AM, Chunyan Liu <cyliu@xxxxxxxx> wrote: 
> > +static int 
> > +get_assigned_devices(libxl__gc *gc, 
> > +                     libxl_device_usb **list, int *num) 
> > +{ 
> > +    char **domlist; 
> > +    unsigned int nd = 0, i, j, k; 
> > +    int rc; 
> > + 
> > +    *list = NULL; 
> > +    *num = 0; 
> > + 
> > +    domlist = libxl__xs_directory(gc, XBT_NULL, "/local/domain", &nd); 
> > +    for (i = 0; i < nd; i++) { 
> > +        char *path, **ctrl_list; 
> > +        unsigned int nc = 0; 
> > + 
> > +        path = GCSPRINTF("/local/domain/%s/device/vusb", domlist[i]); 
> > +        ctrl_list = libxl__xs_directory(gc, XBT_NULL, path, &nc); 
> > + 
> > +        for (j = 0; j < nc; j++) { 
> > +            const char *be_path, *num_ports; 
> > +            int nport; 
> > + 
> > +            rc = libxl__xs_read_checked(gc, XBT_NULL, 
> > +                          GCSPRINTF("%s/%s/backend", path, ctrl_list[j]), 
> > +                          &be_path); 
> > +            if (rc) goto out; 
> > + 
> > +            rc = libxl__xs_read_checked(gc, XBT_NULL, 
> > +                                        GCSPRINTF("%s/num-ports", 
> > be_path), 
> > +                                        &num_ports); 
> > +            if (rc) goto out; 
> > + 
> > +            nport = atoi(num_ports); 
> > +            for (k = 0; k < nport; k++) { 
> > +                libxl_device_usb *usb; 
> > +                const char *portpath, *busid; 
> > + 
> > +                portpath = GCSPRINTF("%s/port/%d", be_path, k + 1); 
> > +                busid = libxl__xs_read(gc, XBT_NULL, portpath); 
> > +                /* If there is USB device attached, add it to list */ 
> > +                if (busid && strcmp(busid, "")) { 
> > +                    GCREALLOC_ARRAY(*list, *num + 1); 
> > +                    usb = *list + *num; 
> > +                    (*num)++; 
> > +                    libxl_device_usb_init(usb); 
> > +                    usb->ctrl = atoi(ctrl_list[j]); 
> > +                    usb->port = k + 1; 
> > +                    rc = usb_busaddr_from_busid(gc, busid, 
> > +                                                &usb->u.hostdev.hostbus, 
> > +                                                &usb->u.hostdev.hostaddr); 
>  
> You should probably set usb->devtype to HOSTDEV here, even though this 
> function is internal. 
>  
> > +                    if (rc) goto out; 
> > +                } 
> > +            } 
> > +        } 
> > +    } 
> > + 
> > +    rc = 0; 
> > + 
> > +out: 
> > +    if (rc) { 
> > +        *list = NULL; 
> > +        *num = 0; 
> > +    } 
> > +    return rc; 
> > +} 
> > + 
> > +static bool is_usbdev_in_array(libxl_device_usb *usbs, int num, 
> > +                               libxl_device_usb *usb) 
> > +{ 
> > +    int i; 
> > + 
> > +    for (i = 0; i < num; i++) { 
> > +        if (usbs[i].u.hostdev.hostbus == usb->u.hostdev.hostbus && 
> > +            usbs[i].u.hostdev.hostaddr == usb->u.hostdev.hostaddr) 
> > +            return true; 
> > +    } 
> > + 
> > +    return false; 
> > +} 
> > + 
> > +/* check if USB device is already assigned to a domain */ 
> > +/* check if USB device type is assignable */ 
> > +static bool is_usb_assignable(libxl__gc *gc, libxl_device_usb *usb) 
> > +{ 
> > +    int classcode; 
> > +    char *filename; 
> > +    void *buf = NULL; 
> > +    char *busid = NULL; 
> > + 
> > +    busid = usb_busaddr_to_busid(gc, usb->u.hostdev.hostbus, 
> > +                                 usb->u.hostdev.hostaddr); 
> > +    if (!busid) return false; 
> > + 
> > +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/bDeviceClass", busid); 
> > +    if (libxl__read_sysfs_file_contents(gc, filename, &buf, NULL)) 
> > +        return false; 
> > + 
> > +    classcode = atoi(buf); 
> > +    return classcode != USBHUB_CLASS_CODE; 
> > +} 
> > + 
> > +/* get usb devices under certain usb controller */ 
> > +static int 
> > +libxl__device_usb_list_for_usbctrl(libxl__gc *gc, uint32_t domid, 
> > +                                   libxl_devid usbctrl, 
> > +                                   libxl_device_usb **usbs, int *num) 
> > +{ 
> > +    const char *fe_path, *be_path, *num_devs; 
> > +    int n, i, rc; 
> > + 
> > +    *usbs = NULL; 
> > +    *num = 0; 
> > + 
> > +    fe_path = GCSPRINTF("%s/device/vusb/%d", 
> > +                        libxl__xs_get_dompath(gc, domid), usbctrl); 
> > + 
> > +    rc = libxl__xs_read_checked(gc, XBT_NULL, 
> > +                                GCSPRINTF("%s/backend", fe_path), 
> > +                                &be_path); 
> > +    if (rc) return rc; 
> > + 
> > +    rc = libxl__xs_read_checked(gc, XBT_NULL, 
> > +                                GCSPRINTF("%s/num-ports", be_path), 
> > +                                &num_devs); 
> > +    if (rc) return rc; 
> > + 
> > +    n = atoi(num_devs); 
> > + 
> > +    for (i = 0; i < n; i++) { 
> > +        char *busid; 
> > +        libxl_device_usb *usb; 
> > + 
> > +        busid = libxl__xs_read(gc, XBT_NULL, 
> > +                               GCSPRINTF("%s/port/%d", be_path, i + 1)); 
> > +        if (busid && strcmp(busid, "")) { 
> > +            GCREALLOC_ARRAY(*usbs, *num + 1); 
> > +            usb = *usbs + *num; 
> > +            (*num)++; 
> > +            libxl_device_usb_init(usb); 
> > +            usb->ctrl = usbctrl; 
> > +            usb->port = i + 1; 
> > +            rc = usb_busaddr_from_busid(gc, busid, 
> > +                                        &usb->u.hostdev.hostbus, 
> > +                                        &usb->u.hostdev.hostaddr); 
>  
> Same thing re devtype. 
>  
> > +int libxl_ctrlport_to_device_usb(libxl_ctx *ctx, 
> > +                                 uint32_t domid, 
> > +                                 int ctrl, 
> > +                                 int port, 
> > +                                 libxl_device_usb *usb) 
> > +{ 
> > +    GC_INIT(ctx); 
> > +    const char *dompath, *be_path, *busid; 
> > +    int rc; 
> > + 
> > +    dompath = libxl__xs_get_dompath(gc, domid); 
> > + 
> > +    rc = libxl__xs_read_checked(gc, XBT_NULL, 
> > +                  GCSPRINTF("%s/device/vusb/%d/backend", dompath, ctrl), 
> > +                  &be_path); 
> > +    if (rc) goto out; 
> > + 
> > +    rc = libxl__xs_read_checked(gc, XBT_NULL, 
> > +                           GCSPRINTF("%s/port/%d", be_path, port), 
> > +                           &busid); 
> > +    if (rc) goto out; 
> > + 
> > +    if (!strcmp(busid, "")) { 
> > +        rc = ERROR_FAIL; 
> > +        goto out; 
> > +    } 
> > + 
> > +    usb->ctrl = ctrl; 
> > +    usb->port = port; 
> > +    rc = usb_busaddr_from_busid(gc, busid, &usb->u.hostdev.hostbus, 
> > +                                &usb->u.hostdev.hostaddr); 
>  
> You definitely need to set usb->devtype here to HOSTDEV. 
>  
> > +libxl_usbinfo = Struct("usbinfo", [ 
> > +    ("ctrl", libxl_devid), 
> > +    ("port", integer), 
> > +    ("busnum", uint8), 
> > +    ("devnum", uint8), 
> > +    ("idVendor", uint16), 
> > +    ("idProduct", uint16), 
> > +    ("prod", string), 
> > +    ("manuf", string), 
> > +    ], dir=DIR_OUT) 
>  
> As mentioned in the review of another patch, it looks like this is 
> vestigal and should be removed. 
>  
> > +void libxl_device_usbctrl_list_free(libxl_device_usbctrl *list, int nr) 
> > +{ 
> > +   int i; 
> > +   for (i = 0; i < nr; i++) 
> > +       libxl_device_usbctrl_dispose(&list[i]); 
> > +   free(list); 
> > +} 
> > + 
> > +void libxl_device_usb_list_free(libxl_device_usb *list, int nr) 
> > +{ 
> > +   int i; 
> > +   for (i = 0; i < nr; i++) 
> > +       libxl_device_usb_dispose(&list[i]); 
> > +   free(list); 
> > +} 
>  
> This is nitpicky, but as long as you're going to repost: the 
> first-level indents in these two functions are only 3 spaces instead 
> of 4.

Take all. Thanks very much!

- Chunyan
 
>  
> Other than that (and my previous comments + Ian's comments) it looks good to  
> me! 
>  
>  -George 
>  
> _______________________________________________ 
> 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