[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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(-)
> 
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 025816a..b9f9015 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1084,6 +1084,8 @@ usbDeviceListGet;
>  usbDeviceListNew;
>  usbDeviceListSteal;
>  usbDeviceSetUsedBy;
> +usbFindDeviceByBus;
> +usbFindDeviceByVendor;
>  usbFindDevice;

Nit: Prefix sorts before the longer name.

> +
> +        /*
> +         * 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))

Huh?  Read literally, that says:

If I requested filtering (flags is nonzero), and the filtering that I
requested was any bit other than BY_VENDOR, then filter by vendor.

But that's not what you want.

> +            if (found_prod != product || found_vend != vendor)
> +                continue;

This if condition is filtering - it says, if my vendor doesn't match,
then resume the loop on the next element.

So you _really_ want:

if ((flags & USB_DEVICE_FIND_BY_VENDOR)) &&
    (found_prod != product || found_vend != vendor))
    continue;

> +
> +        if (flags && !(flags & ~USB_DEVICE_FIND_BY_BUS)) {
> +            if (found_bus != bus || found_devno != devno)
> +                continue;

Likewise, this should be:

if ((flags & USB_DEVICE_FIND_BY_BUS) &&
    (found_bus != bus || found_devno != devno))
    continue;

> +            found = true;
> +        }

At which point, if you get this far in one iteration of the loop, none
of your filtering requests rejected the current device, so you can set
found to true.

>  
> -            if (STRPREFIX(de->d_name, "usb"))
> -                tmpstr += 3;
> +        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;
> +        }

And you don't need this clause at all, because you already took care of
the filtering up above.

> +usbDevice *
> +usbFindDevice(unsigned int vendor,
> +              unsigned int product,
> +              unsigned int bus,
> +              unsigned int devno)
> +{
> +    usbDevice *usb;
> +    usbDeviceList *list;
> +
> +    unsigned int flags = USB_DEVICE_FIND_BY_VENDOR|USB_DEVICE_FIND_BY_BUS;
> +    if (!(list = usbDeviceSearch(vendor, product, bus, devno, flags)))

This code is then correct if you fix usbDeviceSearch.

> +
> +usbDevice * usbFindDeviceByBus(unsigned int bus,

No space after * when returning a pointer type.

> +                               unsigned int devno);
> +
> +usbDeviceList * usbFindDeviceByVendor(unsigned int vendor,

Again

> +                                      unsigned int product);
> +
> +usbDevice * usbFindDevice(unsigned int vendor,

Again

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]