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

[George Dunlap <George.Dunlap@eu.citrix.com>]

On Wed, Oct 21, 2015 at 10:08 AM, Chunyan Liu <cyliu@suse.com> 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.

Other than that (and my previous comments + Ian's comments) it looks good to me!

 -George