[libvirt] [PATCH 1/3] usb: create functions to search usb device accurately

[Guannan Ren]

On 05/04/2012 05:38 AM, Eric Blake wrote:
> On 05/03/2012 03:21 PM, Eric Blake wrote:
>> On 05/03/2012 04:51 AM, Guannan Ren wrote:
>>> usbFindDevice():get usb device according to
>>>                  idVendor, idProduct, bus, device
>>>                  it is the exact match of the four parameters
>>>
>>> usbFindDeviceByBus():get usb device according to bus, device
>>>                    it returns only one usb device same as usbFindDevice
>>>
>>> usbFindDeviceByVendor():get usb device according to idVendor,idProduct
>>>                       it probably returns multiple usb devices.
>>>
>>> usbDeviceSearch(): a helper function to do the actual search
>>> ---
>>>   src/libvirt_private.syms |    2 +
>>>   src/util/hostusb.c       |  209 +++++++++++++++++++++++++++++++++-------------
>>>   src/util/hostusb.h       |   22 ++++--
>>>   3 files changed, 170 insertions(+), 63 deletions(-)
> This patch doesn't even compile!
>
> qemu/qemu_hostdev.c: In function 'qemuPrepareHostdevUSBDevices':
> qemu/qemu_hostdev.c:599:33: error: too few arguments to function
> 'usbFindDevice'
> ../src/util/hostusb.h:40:13: note: declared here
>
> You can't change the signature of usbFindDevice unless you update all
> callers in the same patch.

         I compiled it and tested it , then sent it out.
         I think the changes on usbFindDevice caller is in PATCH2/3 and 
PATCH 3/3
         "usb = usbFindDevice(vendor, product, bus, device);" are there.







>
> By the way, here are some suggestions I have for your patch:
>
> diff --git i/src/util/hostusb.c w/src/util/hostusb.c
> index cad0a6c..533b9c7 100644
> --- i/src/util/hostusb.c
> +++ w/src/util/hostusb.c
> @@ -1,5 +1,5 @@
>   /*
> - * Copyright (C) 2009-2011 Red Hat, Inc.
> + * Copyright (C) 2009-2012 Red Hat, Inc.
>    *
>    * This library is free software; you can redistribute it and/or
>    * modify it under the terms of the GNU Lesser General Public
> @@ -65,9 +65,10 @@ struct _usbDeviceList {
>   };
>
>   typedef enum {
> -    USB_DEVICE_ALL = 0,
> -    USB_DEVICE_FIND_BY_VENDOR = 1,
> -    USB_DEVICE_FIND_BY_BUS = 2,
> +    USB_DEVICE_ALL            = 0,      /* no filtering */
> +    USB_DEVICE_FIND_BY_VENDOR = 1<<  0, /* filter to vendor matches */
> +    USB_DEVICE_FIND_BY_BUS    = 1<<  1, /* filter to bus matches */
> +    USB_DEVICE_FIND_ONE       = 1<<  2, /* filter to first match */


        This patch is great, but "USB_DEVICE_FIND_ONE" is not very useful.
        The bus, devno number could identify usb device enough, the bool 
'found'
        just stop the loop because we are sure that the rest of usb 
devices are no need
        to search.




>   } usbDeviceFindFlags;
>
>   static int usbSysReadFile(const char *f_name, const char *d_name,
> @@ -108,7 +109,6 @@ usbDeviceSearch(unsigned int vendor,
>                   unsigned int flags)
>   {
>       DIR *dir = NULL;
> -    bool found = false;
>       char *ignore = NULL;
>       struct dirent *de;
>       usbDeviceList *list = NULL, *ret = NULL;
> @@ -154,29 +154,13 @@ usbDeviceSearch(unsigned int vendor,
>                              10,&found_devno)<  0)
>               goto cleanup;
>
> -        /*
> -         * Don't set found to true in order to continue the loop
> -         * to find multiple USB devices with same idVendor and idProduct
> -         */
> -        if (flags&&  !(flags&  ~USB_DEVICE_FIND_BY_VENDOR))
> -            if (found_prod != product || found_vend != vendor)
> -                continue;
> -
> -        if (flags&&  !(flags&  ~USB_DEVICE_FIND_BY_BUS)) {
> -            if (found_bus != bus || found_devno != devno)
> -                continue;
> -            found = true;
> -        }
> +        if ((flags&  USB_DEVICE_FIND_BY_VENDOR)&&
> +            (found_prod != product || found_vend != vendor))
> +            continue;
>
> -        if ((flags&  USB_DEVICE_FIND_BY_BUS)
> -&&  (flags&  USB_DEVICE_FIND_BY_VENDOR)) {
> -            if (found_prod != product ||
> -                found_vend != vendor  ||
> -                found_bus != bus ||
> -                found_devno != devno)
> -                continue;
> -            found = true;
> -        }
> +        if ((flags&  USB_DEVICE_FIND_BY_BUS)&&
> +            (found_bus != bus || found_devno != devno))
> +            continue;
>
>           usb = usbGetDevice(found_bus, found_devno);
>           if (!usb)
> @@ -187,7 +171,7 @@ usbDeviceSearch(unsigned int vendor,
>               goto cleanup;
>           }
>
> -        if (found)
> +        if (flags&  USB_DEVICE_FIND_ONE)
>               break;
>       }
>       ret = list;
> @@ -195,7 +179,7 @@ usbDeviceSearch(unsigned int vendor,
>   cleanup:
>       if (dir) {
>           int saved_errno = errno;
> -        closedir (dir);
> +        closedir(dir);
>           errno = saved_errno;
>       }
>
> @@ -209,7 +193,8 @@ usbFindDeviceByVendor(unsigned int vendor, unsigned
> product)
>   {
>
>       usbDeviceList *list;
> -    if (!(list = usbDeviceSearch(vendor, product, 0 , 0,
> USB_DEVICE_FIND_BY_VENDOR)))
> +    if (!(list = usbDeviceSearch(vendor, product, 0 , 0,
> +                                 USB_DEVICE_FIND_BY_VENDOR)))
>           return NULL;
>
>       if (list->count == 0) {
> @@ -228,12 +213,14 @@ usbFindDeviceByBus(unsigned int bus, unsigned devno)
>       usbDevice *usb;
>       usbDeviceList *list;
>
> -    if (!(list = usbDeviceSearch(0, 0, bus, devno,
> USB_DEVICE_FIND_BY_BUS)))
> +    if (!(list = usbDeviceSearch(0, 0, bus, devno,
> +                                 USB_DEVICE_FIND_BY_BUS |
> USB_DEVICE_FIND_ONE)))
>           return NULL;
>
>       if (list->count == 0) {
>           usbReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("Did not find USB device bus:%u device:%u"),
> bus, devno);
> +                       _("Did not find USB device bus:%u device:%u"),
> +                       bus, devno);
>           usbDeviceListFree(list);
>           return NULL;
>       }
> @@ -254,7 +241,9 @@ usbFindDevice(unsigned int vendor,
>       usbDevice *usb;
>       usbDeviceList *list;
>
> -    unsigned int flags = USB_DEVICE_FIND_BY_VENDOR|USB_DEVICE_FIND_BY_BUS;
> +    unsigned int flags = (USB_DEVICE_FIND_BY_VENDOR |
> +                          USB_DEVICE_FIND_BY_BUS |
> +                          USB_DEVICE_FIND_ONE);
>       if (!(list = usbDeviceSearch(vendor, product, bus, devno, flags)))
>           return NULL;
>
>
>