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

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

>>> On 1/13/2016 at 02:31 AM, in message
<22165.18064.452737.543799@xxxxxxxxxxxxxxxxxxxxxxxx>, Ian Jackson
<Ian.Jackson@xxxxxxxxxxxxx> wrote: 
> Chunyan Liu writes ("[PATCH V12 3/5] libxl: add pvusb API"): 
> > Add pvusb APIs, including: 
> >  - attach/detach (create/destroy) virtual usb controller. 
> >  - attach/detach usb device 
> >  - list usb controller and usb devices 
> >  - some other helper functions 
> >  
> > Signed-off-by: Chunyan Liu <cyliu@xxxxxxxx> 
> > Signed-off-by: Simon Cao <caobosimon@xxxxxxxxx> 
> > Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx> 
> > Reviewed-by: George Dunlap <george.dunlap@xxxxxxxxxx> 
>  
> Thanks.  I have re-reviewed this and found a few issues, I'm afraid, 
> mostly in the error handling. 
>  
> > +/* find first unused controller:port and give that to usb device */ 
> > +static int 
> > +libxl__device_usbdev_set_default_usbctrl(libxl__gc *gc, uint32_t domid, 
> > +                                         libxl_device_usbdev *usbdev) 
> > +{ 
> ... 
> > +            path = GCSPRINTF("%s/backend/vusb/%d/%d/port/%d", 
> > +                             libxl__xs_get_dompath(gc,  
> LIBXL_TOOLSTACK_DOMID), 
> > +                             domid, usbctrls[i].devid, j + 1); 
> > +            tmp = libxl__xs_read(gc, XBT_NULL, path); 
>  
> I think this probably ought to be libxl__xs_read_checked ?  (With 
> corresponding change to handling of return value.) 
OK.
>  
> > +/* get original driver path of usb interface, stored in @drvpath */ 
> > +static int usbintf_get_drvpath(libxl__gc *gc, const char *intf, char  
> **drvpath) 
> > +{ 
> > +    char *spath, *dp = NULL; 
> > +    struct stat st; 
> > +    int rc; 
> > + 
> > +    spath = GCSPRINTF(SYSFS_USB_DEV "/%s/driver", intf); 
> > + 
> > +    rc = lstat(spath, &st); 
> > +    if (rc == 0) { 
> > +        /* Find the canonical path to the driver. */ 
> > +        dp = libxl__zalloc(gc, PATH_MAX); 
> > +        dp = realpath(spath, dp); 
>  
> This call to realpath seems to lack error checking/handling. 

Will update.

>  
> > +static int sysfs_write_intf(libxl__gc *gc, const char *intf, const char  
> *path) 
> > +{ 
>  
> What is `intf' here ?  Maybe `interface' ?  But there is nothing usb 
> specific about this function.  Looking for similar code elsewhere this 
> function seems very similar to libxl_pci.c:sysfs_write_bdf, but I 
> won't ask you to try to refactor these two functions together. 

Yes, it means 'interface'. It is referring to old xend/xm naming.
For example, a USB device in Linux sysfs is 3-11, under it, there might be
3-11:1.0 and 3-11:1.1 (these are two interfaces). To assign this USB device
to guest, both 3-11:1.0 and 3-11:1.1 should be unbind from original driver
and bind to usbback.

>  
> > +    rc = write(fd, intf, strlen(intf)); 
>  
> rc ought not be used for a syscall return value.  (CODING_STYLE) 

Will update.

>  
> > +    close(fd); 
>  
> This is a pretty ad-hoc error handling approach.  Can you please use 
> the CODYING_STYLE-recommended pattern ? 

Will update.

> > +static int bind_usbintf(libxl__gc *gc, const char *intf, const char  
> *drvpath) 
> > +{ 
> > +    char *path; 
> > +    struct stat st; 
> > + 
> > +    path = GCSPRINTF("%s/%s", drvpath, intf); 
> > +    /* if already bound, return */ 
> > +    if (!lstat(path, &st)) 
> > +        return 0; 
>  
> If lstat fails, you need to check the error return to see why. 

Will update.

>  
> > +/* Encode usb interface so that it could be written to xenstore as a key. 
> > + * 
> > + * Since xenstore key cannot include '.' or ':', we'll change '.' to '_', 
> > + * change ':' to '@'. For example, 3-1:2.1 will be encoded to 3-1@2_1. 
> > + * This will be used to save original driver of USB device to xenstore. 
> > + */ 
> > +static char *usb_interface_xenstore_encode(libxl__gc *gc, const char  
> *busid) 
> > +{ 
> > +    char *str = libxl__strdup(gc, busid); 
> > +    int i, len = strlen(str); 
> > + 
> > +    for (i = 0; i < len; i++) { 
> > +        if (str[i] == '.') 
> > +            str[i] = '_'; 
> > +        if (str[i] == ':') 
> > +            str[i] = '@'; 
>  
> I know I'm late with this comment and it's trivial and my 
> comaintainers may disagree, but I would find this 
>  
>   +        if (str[i] == '.') str[i] = '_'; 
>   +        if (str[i] == ':') str[i] = '@'; 
>  
> clearer because the layout reflects the regular structure of the code. 
> But please don't change this until another maintainer has commented 
> and said whether they agree with me.  Certainly this is observation of 
> mine does not block this patch. 
>  
> > +static int usbback_dev_unassign(libxl__gc *gc, const char *busid) 
> > +{ 
> > +    char **intfs = NULL; 
> > +    char *usbdev_encode = NULL; 
> > +    char *path = NULL; 
> > +    int i, num = 0; 
> > +    int rc; 
> > + 
> > +    if (usbdev_get_all_interfaces(gc, busid, &intfs, &num) < 0) 
> > +        return ERROR_FAIL; 
>  
> usbdev_get_all_interfaces returns a libxl error code, which you should 
> preserved.  So assign the result to rc, and `if (rc) goto out;'.  Same 
> goes for unbind_usbintf (and maybe other calls elsewhere in this 
> file). 
Will update.

>  
> > +        path = GCSPRINTF(USBBACK_INFO_PATH "/%s/%s/driver_path", 
> > +                         usbdev_encode, usbintf_encode); 
> > +        rc = libxl__xs_read_checked(gc, XBT_NULL, path, &drvpath); 
> > +        if (rc) continue; 
>  
> This anomalous error handling deserves a comment I think.  (See also 
> below.) 
>  
> > +        if (drvpath && bind_usbintf(gc, intf, drvpath)) 
> > +            LOGE(WARN, "Couldn't rebind %s to %s", intf, drvpath); 
> > +    } 
> > + 
> > +    /* finally, remove xenstore driver path */ 
> > +    path = GCSPRINTF(USBBACK_INFO_PATH "/%s", usbdev_encode); 
> > +    libxl__xs_rm_checked(gc, XBT_NULL, path); 
>  
> If you are deliberately throwing away errors (which I think maybe you 
> are), please say so in a comment. 

Got it.

>  
> Ought this function to really report success if these calls fail ? 

I think so. Till here, the USB device has already been unbound from usbback and
removed from xenstore. usb-list cannot list it any more. 

>  
> > +/* Bind USB device to "usbback" driver. 
> > + * 
> > + * If there are many interfaces under USB device, check each interface, 
> > + * unbind from original driver and bind to "usbback" driver. 
> > + */ 
> > +static int usbback_dev_assign(libxl__gc *gc, const char *busid) 
> > +{ 
>  
> Many calls here throw away the rc and invent ERROR_FAIL instead. 
>  
> And this one: 
>  
> > +            if (libxl__xs_write_checked(gc, XBT_NULL, path, drvpath) < 0) 
> > +                goto out; 
>  
> fails to set rc at all. 

Thanks. Will update. 

>  
> > +/* AO operation to add a usb device. 
> > + * 
> > + * Generally, it does: 
> > + * 1) check if the usb device type is assignable 
> > + * 2) check if the usb device is already assigned to a domain 
> > + * 3) add 'busid' of the usb device to xenstore contoller/port/. 
> > + *    (PVUSB driver watches the xenstore changes and will detect that.) 
> > + * 4) unbind usb device from original driver and bind to usbback. 
> > + *    If usb device has many interfaces, then: 
> > + *    - unbind each interface from its original driver and bind to 
> > usbback. 
> > + *    - store the original driver to xenstore for later rebinding when 
> > + *      detaching the device. 
> > + * 
> > + * Before calling this function, aodev should be properly filled: 
> > + * aodev->ao, aodev->callback, aodev->update_json, ... 
> > + */ 
> > +void libxl__device_usbdev_add(libxl__egc *egc, uint32_t domid, 
> > +                              libxl_device_usbdev *usbdev, 
> > +                              libxl__ao_device *aodev) 
> > +{ 
> ... 
> > +    if (is_usbdev_in_array(assigned, num_assigned, usbdev)) { 
> > +        LOG(ERROR, "USB device already attached to a domain"); 
> > +        rc = ERROR_FAIL; 
>  
> Maybe this should be ERROR_INVAL. 

OK. Will update.

- Chunyan


>  
> Thanks, 
> Ian. 
>  
>  


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