From mboxrd@z Thu Jan  1 00:00:00 1970
From: =?windows-1252?Q?J=FCrgen_Gro=DF?= <jgross@suse.com>
Subject: Re: [PATCH RFC V2 3/5] libxl: add pvusb API
Date: Tue, 10 Feb 2015 17:01:22 +0100
Message-ID: <54DA2B52.7060100@suse.com>
References: <1421656131-19366-1-git-send-email-cyliu@suse.com>
	<1421656131-19366-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: <1421656131-19366-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: george.dunlap@eu.citrix.com, lars.kurth@citrix.com, caobosimon@gmail.com, ian.campbell@citrix.com, ian.jackson@citrix.com
List-Id: xen-devel@lists.xenproject.org

Just found other issues:

On 01/19/2015 09:28 AM, Chunyan Liu wrote:
> Add pvusb APIs, including:
>   - attach/detach (create/destroy) virtual usb controller.
>   - attach/detach usb device
>   - list assignable usb devices in host
>   - some other helper functions
>
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> Signed-off-by: Simon Cao <caobosimon@gmail.com>
> ---
>   tools/libxl/Makefile         |    2 +-
>   tools/libxl/libxl.c          |    2 +
>   tools/libxl/libxl.h          |   58 ++
>   tools/libxl/libxl_internal.h |    6 +
>   tools/libxl/libxl_usb.c      | 1277 ++++++++++++++++++++++++++++++++++++++++++
>   tools/libxl/libxlu_cfg_y.c   |  464 ++++++++-------
>   tools/libxl/libxlu_cfg_y.h   |   38 +-
>   7 files changed, 1623 insertions(+), 224 deletions(-)
>   create mode 100644 tools/libxl/libxl_usb.c
>
...
> +/*Get usb device information */
> +static int get_usb_devnum (libxl__gc *gc, const char *intf, char *buf)
> +{
> +    char *path;
> +    int rc = 0;
> +    FILE *fd;
> +
> +    path = libxl__sprintf(gc, "cat "SYSFS_USB_DEVS_PATH"/%s/devnum", intf);
> +    fd = popen(path, "r");
> +    if (fgets(buf, 512, fd) == NULL || ferror(fd))
> +        rc = -1;
> +    pclose(fd);
> +
> +    return rc;
> +}
> +
> +static int get_usb_busnum(libxl__gc *gc, const char *intf, char *buf)
> +{
> +    char *path;
> +    int rc = 0;
> +    FILE *fd;
> +
> +    path = libxl__sprintf(gc, "cat "SYSFS_USB_DEVS_PATH"/%s/busnum", intf);
> +    fd = popen(path, "r");
> +    if (fgets(buf, 512, fd) == NULL || ferror(fd))
> +        rc = -1;
> +    pclose(fd);
> +
> +    return rc;
> +}
> +
> +static int get_usb_idVendor(libxl__gc *gc, const char *intf, char *buf)
> +{
> +    char *path;
> +    int rc = 0;
> +    FILE *fd;
> +
> +    path = libxl__sprintf(gc, "cat "SYSFS_USB_DEVS_PATH"/%s/idVendor", intf);
> +    fd = popen(path, "r");
> +    if (fgets(buf, 512, fd) == NULL || ferror(fd))
> +        rc = -1;
> +    pclose(fd);
> +
> +    return rc;
> +}
> +
> +static int get_usb_idProduct(libxl__gc *gc, const char *intf, char *buf)
> +{
> +    char *path;
> +    int rc = 0;
> +    FILE *fd;
> +
> +    path = libxl__sprintf(gc, "cat "SYSFS_USB_DEVS_PATH"/%s/idProduct", intf);
> +    fd = popen(path, "r");
> +    if (fgets(buf, 512, fd) == NULL || ferror(fd))
> +        rc = -1;
> +    pclose(fd);
> +
> +    return rc;
> +}
> +
> +static int get_usb_manufacturer(libxl__gc *gc, const char *intf, char *buf)
> +{
> +    char *path;
> +    int rc = 0;
> +    FILE *fd;
> +
> +    path = libxl__sprintf(gc, "cat "SYSFS_USB_DEVS_PATH"/%s/manufacturer", intf);

File does not exist in newer kernels (checked 3.16 and 3.19).

> +    fd = popen(path, "r");
> +    if (fgets(buf, 512, fd) == NULL || ferror(fd))
> +        rc = -1;

Another reason not to use popen(): file doesn't exist, but rc = 0.
And buf contains garbage.

> +    pclose(fd);
> +
> +    return rc;
> +}
> +
> +static int get_usb_product(libxl__gc *gc, const char *intf, char *buf)
> +{
> +    char *path;
> +    int rc = 0;
> +    FILE *fd;
> +
> +    path = libxl__sprintf(gc, "cat "SYSFS_USB_DEVS_PATH"/%s/product", intf);

File does not exist in newer kernels (checked 3.16 and 3.19).

> +    fd = popen(path, "r");
> +    if (fgets(buf, 512, fd) == NULL || ferror(fd))
> +        rc = -1;
> +    pclose(fd);
> +
> +    return rc;
> +}
> +
> +int libxl_device_usb_getinfo(libxl_ctx *ctx, char *intf, libxl_usbinfo *usbinfo)
> +{
> +    GC_INIT(ctx);
> +    char buf[512];
> +
> +    if (!get_usb_devnum(gc, intf, buf) )
> +        usbinfo->devnum = atoi(buf);
> +
> +    if ( !get_usb_busnum(gc, intf, buf))
> +        usbinfo->bus = atoi(buf);
> +
> +    if (!get_usb_idVendor(gc, intf, buf) )
> +        usbinfo->idVendor = atoi(buf);

atoi is wrong. idVendor in sysfs is a hex-string.

> +
> +    if (!get_usb_idProduct(gc, intf, buf) )
> +        usbinfo->idProduct  = atoi(buf);

again hex-string.

> +
> +    if (!get_usb_manufacturer(gc, intf, buf) )
> +        usbinfo->manuf = strdup(buf);
> +
> +    if (!get_usb_product(gc, intf, buf) )
> +        usbinfo->prod = strdup(buf);
> +
> +    GC_FREE;
> +    return 0;
> +}