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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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

> +/* 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.

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

> +    rc = write(fd, intf, strlen(intf));

rc ought not be used for a syscall return value.  (CODING_STYLE)

> +    close(fd);

This is a pretty ad-hoc error handling approach.  Can you please use
the CODYING_STYLE-recommended pattern ?

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

> +/* 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).

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

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

> +/* 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.

> +/* 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.

Thanks,
Ian.

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