From mboxrd@z Thu Jan  1 00:00:00 1970
From: Juergen Gross <jgross@suse.com>
Subject: Re: [PATCH V5 3/7] libxl: add pvusb API
Date: Thu, 16 Jul 2015 17:01:29 +0200
Message-ID: <55A7C749.50207@suse.com>
References: <1435226838-3067-1-git-send-email-cyliu@suse.com>
	<1435226838-3067-4-git-send-email-cyliu@suse.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1435226838-3067-4-git-send-email-cyliu@suse.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Chunyan Liu <cyliu@suse.com>, xen-devel@lists.xen.org
Cc: wei.liu2@citrix.com, ian.campbell@citrix.com, george.dunlap@eu.citrix.com, Ian.Jackson@eu.citrix.com, jfehlig@suse.com, Simon Cao <caobosimon@gmail.com>
List-Id: xen-devel@lists.xenproject.org

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