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

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 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]