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

3 May 2012

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

    