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

[Juergen Gross <jgross@suse.com>]

On 06/25/2015 12:07 PM, Chunyan Liu wrote:
> 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@suse.com>
> Signed-off-by: Simon Cao <caobosimon@gmail.com>

Sorry, found another error: You changed too many format specifiers
from "%d" to "%x":

...

> +static char *usb_busaddr_to_busid(libxl__gc *gc, int bus, int addr)
> +{
> +    libxl_ctx *ctx = CTX;
> +    struct dirent *de;
> +    DIR *dir;
> +    char *busid = NULL;
> +
> +    assert(bus > 0 && addr > 0);
> +
> +    if (!(dir = opendir(SYSFS_USB_DEV)))
> +        return NULL;
> +
> +    while((de = readdir(dir))) {
> +        char *filename;
> +        void *buf;
> +        int busnum = -1;
> +        int devnum = -1;
> +
> +        if (!de->d_name)
> +            continue;
> +
> +        filename = GCSPRINTF(SYSFS_USB_DEV"/%s/devnum", de->d_name);
> +        if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL))
> +            sscanf(buf, "%x", &devnum);

That's a decimal number. Use %d, please.

> +
> +        filename = GCSPRINTF(SYSFS_USB_DEV"/%s/busnum", de->d_name);
> +        if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL))
> +            sscanf(buf, "%x", &busnum);

Same here.

> +
> +        if (bus == busnum && addr == devnum) {
> +            busid = libxl__strdup(NOGC, de->d_name);
> +            break;
> +        }
> +    }
> +
> +    closedir(dir);
> +    return busid;
> +}
> +
> +static void usb_busaddr_from_busid(libxl__gc *gc, char *busid,
> +                                   int *bus, int *addr)
> +{
> +    libxl_ctx *ctx = CTX;
> +    char *filename;
> +    void *buf;
> +
> +    assert(busid);
> +
> +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/busnum", busid);
> +    if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL))
> +        sscanf(buf, "%x", bus);

And here.

> +
> +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/devnum", busid);
> +    if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL))
> +        sscanf(buf, "%x", addr);

And here.

> +}

...

> +
> +/* check if USB device type is assignable */
> +static bool is_usb_assignable(libxl__gc *gc, libxl_device_usb *usb)
> +{
> +    libxl_ctx *ctx = CTX;
> +    int classcode;
> +    char *filename;
> +    void *buf = NULL;
> +    char *busid = NULL;
> +
> +    assert(usb->hostbus > 0 && usb->hostaddr > 0);
> +    busid = usb_busaddr_to_busid(gc, usb->hostbus, usb->hostaddr);
> +
> +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/bDeviceClass", busid);
> +    if (libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL))
> +        return false;
> +
> +    sscanf(buf, "%x", &classcode);

This one, too.

> +    return classcode != USBHUB_CLASS_CODE;
> +}

...

> +int libxl_device_usb_getinfo(libxl_ctx *ctx, libxl_device_usb *usb,
> +                             libxl_usbinfo *usbinfo)
> +{
> +    GC_INIT(ctx);
> +    char *filename;
> +    char *busid;
> +    void *buf = NULL;
> +    int buflen, rc;
> +
> +    usbinfo->ctrl = usb->ctrl;
> +    usbinfo->port = usb->port;
> +
> +    busid = usb_busaddr_to_busid(gc, usb->hostbus, usb->hostaddr);
> +    if (!busid) {
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/devnum", busid);
> +    if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL))
> +        sscanf(buf, "%x", &usbinfo->devnum);

Again.

> +
> +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/busnum", busid);
> +    if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL))
> +        sscanf(buf, "%x", &usbinfo->busnum);

And here.

> +
> +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/idVendor", busid);
> +    if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL))
> +        sscanf(buf, "%x", &usbinfo->idVendor);

This one is correct!

> +
> +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/idProduct", busid);
> +    if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, NULL))
> +        sscanf(buf, "%x", &usbinfo->idProduct);

Correct, too.

> +
> +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/manufacturer", busid);
> +    if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, &buflen) &&
> +        buflen > 0) {
> +        /* replace \n to \0 */
> +        if (((char *)buf)[buflen - 1] == '\n')
> +            ((char *)buf)[buflen - 1] = '\0';
> +        usbinfo->manuf = libxl__strdup(NOGC, buf);
> +   }
> +
> +    filename = GCSPRINTF(SYSFS_USB_DEV"/%s/product", busid);
> +    if (!libxl_read_sysfs_file_contents(ctx, filename, &buf, &buflen) &&
> +        buflen > 0) {
> +        /* replace \n to \0 */
> +        if (((char *)buf)[buflen - 1] == '\n')
> +            ((char *)buf)[buflen - 1] = '\0';
> +        usbinfo->prod = libxl__strdup(NOGC, buf);
> +    }
> +
> +    rc = 0;
> +
> +out:
> +    GC_FREE;
> +    return rc;
> +}


Juergen