Re: [libvirt] [PATCH v2 1/3] usb: create functions to search usb device accurately
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 620 bytes)