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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list