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

Osier Yang jyang at redhat.com
Wed May 2 17:17:16 UTC 2012


On 2012年05月01日 16:16, 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

Should we name it as usbFindDevByAddress? Given that both 'bus'
and 'device' are used to find the device actually.

>                    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(-)
>
> 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)


Though no difference between 'unsigned' and 'unsigned int',
expect more typing, should we keep consistent across the
project? I.e. Don't introduce new 'unsigned', and substitute
it with 'unsigned int' as a follow up patch.


>   {
>       DIR *dir = NULL;
> -    int ret = -1, found = 0;
> +    int found = 0;
>       char *ignore = NULL;
>       struct dirent *de;
> +    usbDeviceList *list = NULL, *ret = NULL;
> +    usbDevice *usb;
> +
> +    if (!(list = usbDeviceListNew()))

virReportOOMError();

> +        goto cleanup;
>
>       dir = opendir(USB_SYSFS "/devices");
>       if (!dir) {
> @@ -111,48 +120,72 @@ static int usbFindBusByVendor(unsigned vendor, unsigned product,
>       }
>
>       while ((de = readdir(dir))) {
> -        unsigned found_prod, found_vend;
> +        unsigned found_prod, found_vend, found_bus, found_devno;
> +        char *tmpstr = de->d_name;
> +
>           if (de->d_name[0] == '.' || strchr(de->d_name, ':'))
>               continue;
>
>           if (usbSysReadFile("idVendor", de->d_name,
>                              16,&found_vend)<  0)
>               goto cleanup;
> +
>           if (usbSysReadFile("idProduct", de->d_name,
>                              16,&found_prod)<  0)
>               goto cleanup;
>
> -        if (found_prod == product&&  found_vend == vendor) {
> -            /* Lookup bus.addr info */
> -            char *tmpstr = de->d_name;
> -            unsigned found_bus, found_addr;
> +        if (STRPREFIX(de->d_name, "usb"))
> +            tmpstr += 3;
>
> -            if (STRPREFIX(de->d_name, "usb"))
> -                tmpstr += 3;
> +        if (virStrToLong_ui(tmpstr,&ignore, 10,&found_bus)<  0) {
> +            usbReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Failed to parse dir name '%s'"),
> +                           de->d_name);
> +            goto cleanup;
> +        }
> +
> +        if (usbSysReadFile("devnum", de->d_name,
> +                           10,&found_devno)<  0)
> +            goto cleanup;
>
> -            if (virStrToLong_ui(tmpstr,&ignore, 10,&found_bus)<  0) {
> -                usbReportError(VIR_ERR_INTERNAL_ERROR,
> -                               _("Failed to parse dir name '%s'"),
> -                               de->d_name);
> -                goto cleanup;
> -            }
> +        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:
> +            if (found_prod != product || found_vend != vendor)
> +                continue;
> +            break;
>
> -            if (usbSysReadFile("devnum", de->d_name,
> -                               10,&found_addr)<  0)
> -                goto cleanup;
> +        case USB_DEVICE_FIND_BY_BUS:
> +            if (found_bus != bus || found_devno != devno)
> +                continue;
> +            found = 1;
> +            break;
>
> -            *bus = found_bus;
> -            *devno = found_addr;
> +        case USB_DEVICE_FIND_BY_ALL:
> +            if (found_prod != product
> +                || found_vend != vendor
> +                || found_bus != bus
> +                || found_devno != devno)

As a habit:

if (found_prod != product ||
     found_vend != vendor ||
     found_bus != bus ||
     found_devno != devno)

> +                continue;
>               found = 1;

Given 'found' can only be '0' and '1', why not boolean?

>               break;
>           }
> -    }
>
> -    if (!found)
> -        usbReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("Did not find USB device %x:%x"), vendor, product);
> -    else
> -        ret = 0;
> +        usb = usbGetDevice(found_bus, found_devno);
> +        if (!usb)
> +            goto cleanup;
> +
> +        if (usbDeviceListAdd(list, usb)<  0) {
> +            usbFreeDevice(usb);
> +            goto cleanup;
> +        }
> +
> +        if (found) break;

It's desired to be:

if (found)
     break;

> +    }
> +    ret = list;
>
>   cleanup:
>       if (dir) {
> @@ -160,9 +193,69 @@ cleanup:
>           closedir (dir);
>           errno = saved_errno;
>       }
> +
> +    if (!ret)
> +        usbDeviceListFree(list);
>       return ret;
>   }
>
> +usbDeviceList *
> +usbFindDevByVendor(unsigned vendor, unsigned product)
> +{
> +
> +    usbDeviceList *list;
> +    if (!(list = usbDeviceSearch(vendor, product, 0 , 0, USB_DEVICE_FIND_BY_VENDOR)))
> +        return NULL;
> +
> +    if (list->count == 0) {
> +        usbReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Did not find USB device %x:%x"), vendor, product);
> +        usbDeviceListFree(list);
> +        return NULL;
> +    }
> +
> +    return list;
> +}
> +
> +usbDevice *
> +usbFindDevByBus(unsigned bus, unsigned devno)
> +{
> +    usbDeviceList *list;
> +    if (!(list = usbDeviceSearch(0, 0, bus, devno, USB_DEVICE_FIND_BY_BUS)))
> +        return NULL;
> +
> +    if (list->count == 0) {
> +        usbReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Did not find USB device bus:%u device:%u"), bus, devno);
> +        usbDeviceListFree(list);
> +        return NULL;
> +    }
> +
> +    return usbDeviceListGet(list, 0);

The left items in the list are leaked. (In case of there are
multiple devices found)

> +}
> +
> +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;
> +
> +    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);

Likewise.

> +}
> +
>   usbDevice *
>   usbGetDevice(unsigned bus,
>                unsigned devno)
> @@ -207,21 +300,6 @@ usbGetDevice(unsigned bus,
>       return dev;
>   }
>
> -
> -usbDevice *
> -usbFindDevice(unsigned vendor,
> -              unsigned product)
> -{
> -    unsigned bus = 0, devno = 0;
> -
> -    if (usbFindBusByVendor(vendor, product,&bus,&devno)<  0) {
> -        return NULL;
> -    }
> -
> -    return usbGetDevice(bus, devno);
> -}
> -
> -
>   void
>   usbFreeDevice(usbDevice *dev)
>   {
> diff --git a/src/util/hostusb.h b/src/util/hostusb.h
> index afaa32f..7f18bce 100644
> --- a/src/util/hostusb.h
> +++ b/src/util/hostusb.h
> @@ -28,10 +28,26 @@
>   typedef struct _usbDevice usbDevice;
>   typedef struct _usbDeviceList usbDeviceList;
>
> +typedef enum {
> +    USB_DEVICE_FIND_BY_VENDOR = 0,
> +    USB_DEVICE_FIND_BY_BUS = 1<<  0,
> +    USB_DEVICE_FIND_BY_ALL = 1<<  1,
> +} usbDeviceFindFlags;
> +
>   usbDevice *usbGetDevice(unsigned bus,
>                           unsigned devno);
> -usbDevice *usbFindDevice(unsigned vendor,
> -                         unsigned product);
> +
> +usbDevice * usbFindDevByBus(unsigned bus,
> +                            unsigned devno);
> +
> +usbDeviceList * usbFindDevByVendor(unsigned vendor,
> +                                   unsigned product);
> +
> +usbDevice * usbFindDevice(unsigned vendor,
> +                          unsigned product,
> +                          unsigned bus,
> +                          unsigned devno);
> +
>   void       usbFreeDevice (usbDevice *dev);
>   void       usbDeviceSetUsedBy(usbDevice *dev, const char *name);
>   const char *usbDeviceGetUsedBy(usbDevice *dev);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 025816a..290dad7 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1085,6 +1085,8 @@ usbDeviceListNew;
>   usbDeviceListSteal;
>   usbDeviceSetUsedBy;
>   usbFindDevice;
> +usbFindDevByBus;
> +usbFindDevByVendor;
>   usbFreeDevice;
>   usbGetDevice;




More information about the libvir-list mailing list