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

2012-05-03 Thread Guannan Ren 

On 05/03/2012 01:17 AM, Osier Yang wrote:

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.



the term 'address' for usb device is used by kernel
maybe not a good name.

The codes has been changed according to the following comments





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



 usbDeviceListNew has raised OOM Error in that case.

 Guannan Ren

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2012-05-02 Thread Martin Kletzander 
On 05/01/2012 10: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(-)
 
 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)
  {
  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()))
 +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)
 +continue;
  found = 1;
  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;
 +}
 +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;

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

2012-05-02 Thread Eric Blake 
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   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2012-05-02 Thread Osier Yang 

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) {
+

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

2012-05-01 Thread Guannan Ren 
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(-)

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)
 {
 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()))
+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)
+continue;
 found = 1;
 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;
+}
+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,
+