On 05/01/2012 02:16 AM, Guannan Ren wrote:
usbFindDevice():get usb device according to idVendor, idProduct, bus, device it is the most strict search
usbFindDevByBus():get usb device according to bus, device it returns only one usb device same as usbFindDevice
usbFindDevByVendor():get usb device according to idVendor,idProduct it probably returns multiple usb devices.
usbDeviceSearch(): a helper function to do the actual search --- src/util/hostusb.c | 162 ++++++++++++++++++++++++++++++++++++++------------- src/util/hostusb.h | 20 ++++++- 2 files changed, 138 insertions(+), 44 deletions(-)
Looks like a reasonable set of functions from the description. It feels a bit weird to have FindDevice spelled out but FindDevByBus abbreviated; should we rename it to FindDeviceByBus?
diff --git a/src/util/hostusb.c b/src/util/hostusb.c index 92f52a2..73dc959 100644 --- a/src/util/hostusb.c +++ b/src/util/hostusb.c @@ -94,13 +94,22 @@ cleanup: return ret; }
-static int usbFindBusByVendor(unsigned vendor, unsigned product, - unsigned *bus, unsigned *devno) +static usbDeviceList * +usbDeviceSearch(unsigned vendor, + unsigned product, + unsigned bus, + unsigned devno, + unsigned flags)
Since this function is static, I think the enum that controls its behavior should live in this C file, rather than in a header included by other files that can't use this function.
{ DIR *dir = NULL; - int ret = -1, found = 0; + int found = 0;
Is found a count (incremented for each hit) or a boolean (set to true when we find one, regardless of whether we find more)? If the latter, then s/int/bool/; s/0/false/
char *ignore = NULL;
+ switch (flags) { + /* + * Don't set found to 1 in order to continue the loop + * to find multiple USB devices with same idVendor and idProduct + */ + case USB_DEVICE_FIND_BY_VENDOR:
I think this is not quite right. Your flags should be a bitmask: FIND_BY_VENDOR = 1, FIND_BY_BUS = 2, Then you do this pseudocode: if (flags & USB_DEVICE_FIND_BY_VENDOR) continue loop unless product and vendor match if (flags & USB_DEVICE_FIND_BY_BUS) continue loop unless bus and devno match if we get this far, update found This would also let you pass in flags of 0 to search for _all_ USB devices , without regards to vendor or bus (we don't have a function for this right now, but could add one if needed), as well as passing in flags of 3 (BY_VENDOR|BY_BUS) to do an exact match of all four parameters (this would be usbFindDevice).
+ + if (found) break;
Coding conventions request writing this as two lines.
+usbDevice * +usbFindDevice(unsigned vendor, + unsigned product, + unsigned bus, + unsigned devno) +{ + usbDeviceList *list; + if (!(list = usbDeviceSearch(vendor, product, + bus, devno, USB_DEVICE_FIND_BY_ALL))) + return NULL;
Allocates a list...
+ + if (list->count == 0) { + usbReportError(VIR_ERR_INTERNAL_ERROR, + _("Did not find USB device %x:%x bus:%u device:%u"), + vendor, product, bus, devno); + usbDeviceListFree(list); + return NULL; + } + + return usbDeviceListGet(list, 0);
and returns the first element of the list, but loses the reference to the list itself. Memory leak.
+typedef enum { + USB_DEVICE_FIND_BY_VENDOR = 0, + USB_DEVICE_FIND_BY_BUS = 1 << 0, + USB_DEVICE_FIND_BY_ALL = 1 << 1, +} usbDeviceFindFlags;
See above - this enum should live in the .c file before the static function it affects, and should be a bit-mask of two separate filtering features. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org