3:16 a.m.
https://bugzilla.redhat.com/show_bug.cgi?id=815755
The set of patch tries to fix the issue when multiple usb devices with
same idVendor, idProduct are availible on host, the usb device with
lowest bus:device will be attached to guest if usb xml file is given like
this:
<hostdev mode='subsystem' type='usb' managed='yes'>
<source>
<vendor id='0x15e1'/>
<product id='0x2007'/>
</source>
</hostdev>
The reason is that the usb hotplug function searchs usb device in system files
to match vendor and product id, the file with lowest number is always found
first.
After fix, in this case, libvirt will report an error as follows:
At the same time, the usb part of domain initilization is also update in patch 2/3
# virsh attach-device rhel6u1 /tmp/usb.xml
error: Failed to attach device from /tmp/usb.xml
error: XML error: multiple USB deivces 15e1:2007, use <address> to specify one.
The patch also fix the problem when using the following xml, the usb device could
be hotplugged in two domains without raising errors.
<hostdev mode='subsystem' type='usb' managed='yes'>
<source>
<address bus='6' device='4'/>
</source>
</hostdev>
3:16 a.m.
New subject: [libvirt] [PATCH v2 1/3] usb: create functions to search usb device accurately
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,
+ _("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);
+}
+
+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);
+}
+
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;
--
1.7.7.5
8:13 a.m.
New subject: [libvirt] [PATCH v2 1/3] usb: create functions to search usb device accurately
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;
+ 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);
+}
+
+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);
+}
+
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;
This looks good to me, but I'd prefer one more ACK with mine. I'll go
through the rest of the series, but the ACK assurance will be the same
as I'm not very familiar with this still.
BTW I had problems applying this patch to current head, I had to add a
newline in the middle of:
@@ -1085,6 +1085,8 @@ usbDeviceListNew;
and make it:
@@ -1085,6 +1085,8 @@
usbDeviceListNew;
Don't know how that happened, though, other patches apply fine :)
Martin
11:53 a.m.
New subject: [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
12:17 p.m.
New subject: [libvirt] [PATCH v2 1/3] usb: create functions to search usb device accurately
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;
3:31 a.m.
New subject: [libvirt] [PATCH v2 1/3] usb: create functions to search usb device accurately
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
3:16 a.m.
New subject: [libvirt] [PATCH v2 2/3] qemu: make use of usb search function to initialize usb devices
refactor qemuPrepareHostdevUSBDevices function, make it focus on
adding usb device to activeUsbHostdevs after check. After that,
the usb hotplug function qemuDomainAttachHostDevice also could use
it.
expand qemuPrepareHostUSBDevices to perform the usb search,
rollback on failure.
---
src/qemu/qemu_hostdev.c | 118 ++++++++++++++++++++++++++++++-----------------
src/qemu/qemu_hostdev.h | 3 +-
2 files changed, 76 insertions(+), 45 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 8594fb2..a3d4ad4 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -565,13 +565,51 @@ qemuPrepareHostPCIDevices(struct qemud_driver *driver,
int
qemuPrepareHostdevUSBDevices(struct qemud_driver *driver,
const char *name,
- virDomainHostdevDefPtr *hostdevs,
- int nhostdevs)
+ usbDeviceList *list)
{
- int ret = -1;
int i;
+ usbDevice *tmp;
+
+ for (i = 0; i < usbDeviceListCount(list); i++) {
+ usbDevice *usb = usbDeviceListGet(list, i);
+ if ((tmp = usbDeviceListFind(driver->activeUsbHostdevs, usb))) {
+ const char *other_name = usbDeviceGetUsedBy(tmp);
+
+ if (other_name)
+ qemuReportError(VIR_ERR_OPERATION_INVALID,
+ _("USB device %s is in use by domain %s"),
+ usbDeviceGetName(tmp), other_name);
+ else
+ qemuReportError(VIR_ERR_OPERATION_INVALID,
+ _("USB device %s is already in use"),
+ usbDeviceGetName(tmp));
+ return -1;
+ }
+
+ usbDeviceSetUsedBy(usb, name);
+ VIR_DEBUG("Adding %03d.%03d dom=%s to activeUsbHostdevs",
+ usbDeviceGetBus(usb), usbDeviceGetDevno(usb), name);
+ /*
+ * The caller is responsible to steal the list of usb
+ * from the usbDeviceList that passed in on success,
+ * perform rollback on failure.
+ */
+ if (usbDeviceListAdd(driver->activeUsbHostdevs, usb) < 0)
+ return -1;
+
+ }
+ return 0;
+}
+
+static int
+qemuPrepareHostUSBDevices(struct qemud_driver *driver,
+ virDomainDefPtr def)
+{
+ int i, ret = -1;
usbDeviceList *list;
usbDevice *tmp;
+ virDomainHostdevDefPtr *hostdevs = def->hostdevs;
+ int nhostdevs = def->nhostdevs;
/* To prevent situation where USB device is assigned to two domains
* we need to keep a list of currently assigned USB devices.
@@ -586,64 +624,65 @@ qemuPrepareHostdevUSBDevices(struct qemud_driver *driver,
*/
for (i = 0 ; i < nhostdevs ; i++) {
virDomainHostdevDefPtr hostdev = hostdevs[i];
+ usbDevice *usb = NULL;
if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
continue;
if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)
continue;
- /* Resolve a vendor/product to bus/device */
- if (hostdev->source.subsys.u.usb.vendor) {
- usbDevice *usb
- = usbFindDevice(hostdev->source.subsys.u.usb.vendor,
- hostdev->source.subsys.u.usb.product);
+ unsigned vendor = hostdev->source.subsys.u.usb.vendor;
+ unsigned product = hostdev->source.subsys.u.usb.product;
+ unsigned bus = hostdev->source.subsys.u.usb.bus;
+ unsigned device = hostdev->source.subsys.u.usb.device;
+ if (vendor && bus) {
+ usb = usbFindDevice(vendor, product, bus, device);
if (!usb)
goto cleanup;
- if ((tmp = usbDeviceListFind(driver->activeUsbHostdevs, usb))) {
- const char *other_name = usbDeviceGetUsedBy(tmp);
-
- if (other_name)
- qemuReportError(VIR_ERR_OPERATION_INVALID,
- _("USB device %s is in use by domain %s"),
- usbDeviceGetName(tmp), other_name);
- else
- qemuReportError(VIR_ERR_OPERATION_INVALID,
- _("USB device %s is already in use"),
- usbDeviceGetName(tmp));
- usbFreeDevice(usb);
+ } else if (vendor && !bus) {
+ usbDeviceList *devs = usbFindDevByVendor(vendor, product);
+ if (!devs)
+ goto cleanup;
+
+ if (usbDeviceListCount(devs) > 1) {
+ qemuReportError(VIR_ERR_XML_ERROR,
+ _("multiple USB deivces %x:%x, "
+ "use <address> to specify one"),
vendor, product);
+ usbDeviceListFree(devs);
goto cleanup;
}
+ usb = usbDeviceListGet(devs, 0);
+ usbDeviceListSteal(devs, usb);
+ usbDeviceListFree(devs);
hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb);
hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb);
- if (usbDeviceListAdd(list, usb) < 0) {
- usbFreeDevice(usb);
+ } else if (!vendor && bus) {
+ usb = usbFindDevByBus(bus, device);
+ if (!usb)
goto cleanup;
- }
+ }
+ if (!usb)
+ goto cleanup;
+
+ if (usbDeviceListAdd(list, usb) < 0) {
+ usbFreeDevice(usb);
+ goto cleanup;
}
}
- /* Loop 2: Mark devices in temporary list as used by @name
+ /* Mark devices in temporary list as used by @name
* and add them do driver list. However, if something goes
* wrong, perform rollback.
*/
- for (i = 0; i < usbDeviceListCount(list); i++) {
- tmp = usbDeviceListGet(list, i);
- usbDeviceSetUsedBy(tmp, name);
+ if (qemuPrepareHostdevUSBDevices(driver, def->name, list) < 0)
+ goto inactivedevs;
- VIR_DEBUG("Adding %03d.%03d dom=%s to activeUsbHostdevs",
- usbDeviceGetBus(tmp), usbDeviceGetDevno(tmp), name);
- if (usbDeviceListAdd(driver->activeUsbHostdevs, tmp) < 0) {
- usbFreeDevice(tmp);
- goto inactivedevs;
- }
- }
-
- /* Loop 3: Temporary list was successfully merged with
+ /* Loop 2: Temporary list was successfully merged with
* driver list, so steal all items to avoid freeing them
* in cleanup label.
*/
@@ -669,13 +708,6 @@ cleanup:
return ret;
}
-static int
-qemuPrepareHostUSBDevices(struct qemud_driver *driver,
- virDomainDefPtr def)
-{
- return qemuPrepareHostdevUSBDevices(driver, def->name, def->hostdevs,
def->nhostdevs);
-}
-
int qemuPrepareHostDevices(struct qemud_driver *driver,
virDomainDefPtr def)
{
diff --git a/src/qemu/qemu_hostdev.h b/src/qemu/qemu_hostdev.h
index 371630a..a8acccf 100644
--- a/src/qemu/qemu_hostdev.h
+++ b/src/qemu/qemu_hostdev.h
@@ -38,8 +38,7 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver,
int nhostdevs);
int qemuPrepareHostdevUSBDevices(struct qemud_driver *driver,
const char *name,
- virDomainHostdevDefPtr *hostdevs,
- int nhostdevs);
+ usbDeviceList *list);
int qemuPrepareHostDevices(struct qemud_driver *driver,
virDomainDefPtr def);
void qemuReattachPciDevice(pciDevice *dev, struct qemud_driver *driver);
--
1.7.7.5
10:11 a.m.
New subject: [libvirt] [PATCH v2 2/3] qemu: make use of usb search function to initialize usb devices
On 05/01/2012 10:16 AM, Guannan Ren wrote:
refactor qemuPrepareHostdevUSBDevices function, make it focus on
adding usb device to activeUsbHostdevs after check. After that,
the usb hotplug function qemuDomainAttachHostDevice also could use
it.
expand qemuPrepareHostUSBDevices to perform the usb search,
rollback on failure.
---
src/qemu/qemu_hostdev.c | 118 ++++++++++++++++++++++++++++++-----------------
src/qemu/qemu_hostdev.h | 3 +-
2 files changed, 76 insertions(+), 45 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 8594fb2..a3d4ad4 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -565,13 +565,51 @@ qemuPrepareHostPCIDevices(struct qemud_driver *driver,
int
qemuPrepareHostdevUSBDevices(struct qemud_driver *driver,
const char *name,
- virDomainHostdevDefPtr *hostdevs,
- int nhostdevs)
+ usbDeviceList *list)
{
- int ret = -1;
int i;
+ usbDevice *tmp;
+
+ for (i = 0; i < usbDeviceListCount(list); i++) {
+ usbDevice *usb = usbDeviceListGet(list, i);
+ if ((tmp = usbDeviceListFind(driver->activeUsbHostdevs, usb))) {
+ const char *other_name = usbDeviceGetUsedBy(tmp);
+
+ if (other_name)
+ qemuReportError(VIR_ERR_OPERATION_INVALID,
+ _("USB device %s is in use by domain %s"),
+ usbDeviceGetName(tmp), other_name);
+ else
+ qemuReportError(VIR_ERR_OPERATION_INVALID,
+ _("USB device %s is already in use"),
+ usbDeviceGetName(tmp));
+ return -1;
+ }
+
+ usbDeviceSetUsedBy(usb, name);
+ VIR_DEBUG("Adding %03d.%03d dom=%s to activeUsbHostdevs",
+ usbDeviceGetBus(usb), usbDeviceGetDevno(usb), name);
+ /*
+ * The caller is responsible to steal the list of usb
+ * from the usbDeviceList that passed in on success,
+ * perform rollback on failure.
+ */
+ if (usbDeviceListAdd(driver->activeUsbHostdevs, usb) < 0)
+ return -1;
+
+ }
+ return 0;
+}
+
+static int
+qemuPrepareHostUSBDevices(struct qemud_driver *driver,
+ virDomainDefPtr def)
+{
+ int i, ret = -1;
usbDeviceList *list;
usbDevice *tmp;
+ virDomainHostdevDefPtr *hostdevs = def->hostdevs;
+ int nhostdevs = def->nhostdevs;
/* To prevent situation where USB device is assigned to two domains
* we need to keep a list of currently assigned USB devices.
@@ -586,64 +624,65 @@ qemuPrepareHostdevUSBDevices(struct qemud_driver *driver,
*/
for (i = 0 ; i < nhostdevs ; i++) {
virDomainHostdevDefPtr hostdev = hostdevs[i];
+ usbDevice *usb = NULL;
if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
continue;
if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)
continue;
- /* Resolve a vendor/product to bus/device */
- if (hostdev->source.subsys.u.usb.vendor) {
- usbDevice *usb
- = usbFindDevice(hostdev->source.subsys.u.usb.vendor,
- hostdev->source.subsys.u.usb.product);
+ unsigned vendor = hostdev->source.subsys.u.usb.vendor;
+ unsigned product = hostdev->source.subsys.u.usb.product;
+ unsigned bus = hostdev->source.subsys.u.usb.bus;
+ unsigned device = hostdev->source.subsys.u.usb.device;
+ if (vendor && bus) {
+ usb = usbFindDevice(vendor, product, bus, device);
if (!usb)
goto cleanup;
- if ((tmp = usbDeviceListFind(driver->activeUsbHostdevs, usb))) {
- const char *other_name = usbDeviceGetUsedBy(tmp);
-
- if (other_name)
- qemuReportError(VIR_ERR_OPERATION_INVALID,
- _("USB device %s is in use by domain
%s"),
- usbDeviceGetName(tmp), other_name);
- else
- qemuReportError(VIR_ERR_OPERATION_INVALID,
- _("USB device %s is already in use"),
- usbDeviceGetName(tmp));
- usbFreeDevice(usb);
+ } else if (vendor && !bus) {
+ usbDeviceList *devs = usbFindDevByVendor(vendor, product);
+ if (!devs)
+ goto cleanup;
+
+ if (usbDeviceListCount(devs) > 1) {
+ qemuReportError(VIR_ERR_XML_ERROR,
+ _("multiple USB deivces %x:%x, "
+ "use <address> to specify one"),
vendor, product);
+ usbDeviceListFree(devs);
goto cleanup;
}
+ usb = usbDeviceListGet(devs, 0);
+ usbDeviceListSteal(devs, usb);
+ usbDeviceListFree(devs);
hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb);
hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb);
- if (usbDeviceListAdd(list, usb) < 0) {
- usbFreeDevice(usb);
+ } else if (!vendor && bus) {
+ usb = usbFindDevByBus(bus, device);
+ if (!usb)
goto cleanup;
- }
+ }
+ if (!usb)
+ goto cleanup;
+
+ if (usbDeviceListAdd(list, usb) < 0) {
+ usbFreeDevice(usb);
+ goto cleanup;
}
}
- /* Loop 2: Mark devices in temporary list as used by @name
+ /* Mark devices in temporary list as used by @name
* and add them do driver list. However, if something goes
* wrong, perform rollback.
*/
- for (i = 0; i < usbDeviceListCount(list); i++) {
- tmp = usbDeviceListGet(list, i);
- usbDeviceSetUsedBy(tmp, name);
+ if (qemuPrepareHostdevUSBDevices(driver, def->name, list) < 0)
+ goto inactivedevs;
- VIR_DEBUG("Adding %03d.%03d dom=%s to activeUsbHostdevs",
- usbDeviceGetBus(tmp), usbDeviceGetDevno(tmp), name);
- if (usbDeviceListAdd(driver->activeUsbHostdevs, tmp) < 0) {
- usbFreeDevice(tmp);
- goto inactivedevs;
- }
- }
-
- /* Loop 3: Temporary list was successfully merged with
+ /* Loop 2: Temporary list was successfully merged with
* driver list, so steal all items to avoid freeing them
* in cleanup label.
*/
@@ -669,13 +708,6 @@ cleanup:
return ret;
}
-static int
-qemuPrepareHostUSBDevices(struct qemud_driver *driver,
- virDomainDefPtr def)
-{
- return qemuPrepareHostdevUSBDevices(driver, def->name, def->hostdevs,
def->nhostdevs);
-}
-
int qemuPrepareHostDevices(struct qemud_driver *driver,
virDomainDefPtr def)
{
diff --git a/src/qemu/qemu_hostdev.h b/src/qemu/qemu_hostdev.h
index 371630a..a8acccf 100644
--- a/src/qemu/qemu_hostdev.h
+++ b/src/qemu/qemu_hostdev.h
@@ -38,8 +38,7 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver,
int nhostdevs);
int qemuPrepareHostdevUSBDevices(struct qemud_driver *driver,
const char *name,
- virDomainHostdevDefPtr *hostdevs,
- int nhostdevs);
+ usbDeviceList *list);
int qemuPrepareHostDevices(struct qemud_driver *driver,
virDomainDefPtr def);
void qemuReattachPciDevice(pciDevice *dev, struct qemud_driver *driver);
This looks good also, ACK.
Martin
12:29 p.m.
New subject: [libvirt] [PATCH v2 2/3] qemu: make use of usb search function to initialize usb devices
On 05/01/2012 02:16 AM, Guannan Ren wrote:
refactor qemuPrepareHostdevUSBDevices function, make it focus on
adding usb device to activeUsbHostdevs after check. After that,
the usb hotplug function qemuDomainAttachHostDevice also could use
it.
expand qemuPrepareHostUSBDevices to perform the usb search,
rollback on failure.
---
+ if (vendor && bus) {
+ usb = usbFindDevice(vendor, product, bus, device);
Can bus be 0, or does the USB bus numbering start with 1? If 0 is a
valid bus number, then you need to know whether bus was specified, not
whether it is non-zero.
+ } else if (vendor && !bus) {
+ usbDeviceList *devs = usbFindDevByVendor(vendor, product);
+ if (!devs)
+ goto cleanup;
+
+ if (usbDeviceListCount(devs) > 1) {
+ qemuReportError(VIR_ERR_XML_ERROR,
+ _("multiple USB deivces %x:%x, "
s/deivces/devices/
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3:03 a.m.
New subject: [libvirt] [PATCH v2 2/3] qemu: make use of usb search function to initialize usb devices
On 05/03/2012 01:29 AM, Eric Blake wrote:
On 05/01/2012 02:16 AM, Guannan Ren wrote:
> refactor qemuPrepareHostdevUSBDevices function, make it focus on
> adding usb device to activeUsbHostdevs after check. After that,
> the usb hotplug function qemuDomainAttachHostDevice also could use
> it.
>
> expand qemuPrepareHostUSBDevices to perform the usb search,
> rollback on failure.
> ---
>
> + if (vendor&& bus) {
> + usb = usbFindDevice(vendor, product, bus, device);
Can bus be 0, or does the USB bus numbering start with 1? If 0 is a
valid bus number, then you need to know whether bus was specified, not
whether it is non-zero.
Bus number start with 1,
"
# ls /sys/bus/usb/devices/
1-0:1.0 1-1.3 1-1.3.1:1.0 1-1:1.0 1-1 1-1.3.1 1-1.3:1.0 usb1
The names that begin with "usb" refer to USB controllers.
More accurately, they refer to the "root hub" associated with each
controller.
The number is the USB bus number. In the example there is only one
controller,
so its bus is number 1. Hence the name "usb1"
"
http://www.linux-usb.org/FAQ.html#ts5
If bus is specified, bus should be greater than zero( it will be
ignored to specify zero)
if not specified, usbFindDevByVendor is invoked to get device.
> + } else if (vendor&& !bus) {
> + usbDeviceList *devs = usbFindDevByVendor(vendor, product);
> + if (!devs)
> + goto cleanup;
> +
> + if (usbDeviceListCount(devs)> 1) {
> + qemuReportError(VIR_ERR_XML_ERROR,
> + _("multiple USB deivces %x:%x, "
s/deivces/devices/
12:45 p.m.
New subject: [libvirt] [PATCH v2 2/3] qemu: make use of usb search function to initialize usb devices
On 2012年05月01日 16:16, Guannan Ren wrote:
refactor qemuPrepareHostdevUSBDevices function, make it focus on
adding usb device to activeUsbHostdevs after check. After that,
the usb hotplug function qemuDomainAttachHostDevice also could use
it.
expand qemuPrepareHostUSBDevices to perform the usb search,
rollback on failure.
---
src/qemu/qemu_hostdev.c | 118 ++++++++++++++++++++++++++++++-----------------
src/qemu/qemu_hostdev.h | 3 +-
2 files changed, 76 insertions(+), 45 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 8594fb2..a3d4ad4 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -565,13 +565,51 @@ qemuPrepareHostPCIDevices(struct qemud_driver *driver,
int
qemuPrepareHostdevUSBDevices(struct qemud_driver *driver,
const char *name,
- virDomainHostdevDefPtr *hostdevs,
- int nhostdevs)
+ usbDeviceList *list)
{
- int ret = -1;
int i;
+ usbDevice *tmp;
+
+ for (i = 0; i< usbDeviceListCount(list); i++) {
+ usbDevice *usb = usbDeviceListGet(list, i);
+ if ((tmp = usbDeviceListFind(driver->activeUsbHostdevs, usb))) {
+ const char *other_name = usbDeviceGetUsedBy(tmp);
+
+ if (other_name)
+ qemuReportError(VIR_ERR_OPERATION_INVALID,
+ _("USB device %s is in use by domain %s"),
+ usbDeviceGetName(tmp), other_name);
+ else
+ qemuReportError(VIR_ERR_OPERATION_INVALID,
+ _("USB device %s is already in use"),
+ usbDeviceGetName(tmp));
+ return -1;
+ }
+
+ usbDeviceSetUsedBy(usb, name);
+ VIR_DEBUG("Adding %03d.%03d dom=%s to activeUsbHostdevs",
+ usbDeviceGetBus(usb), usbDeviceGetDevno(usb), name);
+ /*
+ * The caller is responsible to steal the list of usb
+ * from the usbDeviceList that passed in on success,
s/steal the list of usb/usb devices which are used by domain/.
+ * perform rollback on failure.
+ */
+ if (usbDeviceListAdd(driver->activeUsbHostdevs, usb)< 0)
+ return -1;
+
+ }
+ return 0;
+}
+
+static int
+qemuPrepareHostUSBDevices(struct qemud_driver *driver,
+ virDomainDefPtr def)
It's confused with qemuPrepareHostdevUSBDevices, the idea to
have a general function (new qemuPrepareHostdevUSBDevices) for
reusing is good. But we should have another name for it instead,
and keep "qemuPrepareHostdevUSBDevices" here.
+{
+ int i, ret = -1;
usbDeviceList *list;
usbDevice *tmp;
+ virDomainHostdevDefPtr *hostdevs = def->hostdevs;
+ int nhostdevs = def->nhostdevs;
/* To prevent situation where USB device is assigned to two domains
* we need to keep a list of currently assigned USB devices.
@@ -586,64 +624,65 @@ qemuPrepareHostdevUSBDevices(struct qemud_driver *driver,
*/
for (i = 0 ; i< nhostdevs ; i++) {
virDomainHostdevDefPtr hostdev = hostdevs[i];
+ usbDevice *usb = NULL;
if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
continue;
if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB)
continue;
- /* Resolve a vendor/product to bus/device */
- if (hostdev->source.subsys.u.usb.vendor) {
- usbDevice *usb
- = usbFindDevice(hostdev->source.subsys.u.usb.vendor,
- hostdev->source.subsys.u.usb.product);
+ unsigned vendor = hostdev->source.subsys.u.usb.vendor;
+ unsigned product = hostdev->source.subsys.u.usb.product;
+ unsigned bus = hostdev->source.subsys.u.usb.bus;
+ unsigned device = hostdev->source.subsys.u.usb.device;
+ if (vendor&& bus) {
+ usb = usbFindDevice(vendor, product, bus, device);
if (!usb)
goto cleanup;
Given there is a check of 'usb' after the "if...else if..." clauses.
This should be removed. see [1]
- if ((tmp = usbDeviceListFind(driver->activeUsbHostdevs, usb))) {
- const char *other_name = usbDeviceGetUsedBy(tmp);
-
- if (other_name)
- qemuReportError(VIR_ERR_OPERATION_INVALID,
- _("USB device %s is in use by domain
%s"),
- usbDeviceGetName(tmp), other_name);
- else
- qemuReportError(VIR_ERR_OPERATION_INVALID,
- _("USB device %s is already in use"),
- usbDeviceGetName(tmp));
- usbFreeDevice(usb);
+ } else if (vendor&& !bus) {
+ usbDeviceList *devs = usbFindDevByVendor(vendor, product);
+ if (!devs)
+ goto cleanup;
likewise.
+
+ if (usbDeviceListCount(devs)> 1) {
+ qemuReportError(VIR_ERR_XML_ERROR,
+ _("multiple USB deivces %x:%x, "
+ "use<address> to specify one"),
vendor, product);
The place to fix the error type, and have a more clear message.
+ usbDeviceListFree(devs);
goto cleanup;
}
+ usb = usbDeviceListGet(devs, 0);
+ usbDeviceListSteal(devs, usb);
+ usbDeviceListFree(devs);
hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb);
hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb);
- if (usbDeviceListAdd(list, usb)< 0) {
- usbFreeDevice(usb);
+ } else if (!vendor&& bus) {
+ usb = usbFindDevByBus(bus, device);
+ if (!usb)
goto cleanup;
Should be removed.
- }
+ }
+ if (!usb)
+ goto cleanup;
[1] The check is here.
+
+ if (usbDeviceListAdd(list, usb)< 0) {
+ usbFreeDevice(usb);
+ goto cleanup;
}
}
- /* Loop 2: Mark devices in temporary list as used by @name
+ /* Mark devices in temporary list as used by @name
* and add them do driver list. However, if something goes
* wrong, perform rollback.
*/
- for (i = 0; i< usbDeviceListCount(list); i++) {
- tmp = usbDeviceListGet(list, i);
- usbDeviceSetUsedBy(tmp, name);
+ if (qemuPrepareHostdevUSBDevices(driver, def->name, list)< 0)
+ goto inactivedevs;
- VIR_DEBUG("Adding %03d.%03d dom=%s to activeUsbHostdevs",
- usbDeviceGetBus(tmp), usbDeviceGetDevno(tmp), name);
- if (usbDeviceListAdd(driver->activeUsbHostdevs, tmp)< 0) {
- usbFreeDevice(tmp);
- goto inactivedevs;
- }
- }
-
- /* Loop 3: Temporary list was successfully merged with
+ /* Loop 2: Temporary list was successfully merged with
* driver list, so steal all items to avoid freeing them
* in cleanup label.
*/
@@ -669,13 +708,6 @@ cleanup:
return ret;
}
-static int
-qemuPrepareHostUSBDevices(struct qemud_driver *driver,
- virDomainDefPtr def)
-{
- return qemuPrepareHostdevUSBDevices(driver, def->name, def->hostdevs,
def->nhostdevs);
-}
-
int qemuPrepareHostDevices(struct qemud_driver *driver,
virDomainDefPtr def)
{
diff --git a/src/qemu/qemu_hostdev.h b/src/qemu/qemu_hostdev.h
index 371630a..a8acccf 100644
--- a/src/qemu/qemu_hostdev.h
+++ b/src/qemu/qemu_hostdev.h
@@ -38,8 +38,7 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver,
int nhostdevs);
int qemuPrepareHostdevUSBDevices(struct qemud_driver *driver,
const char *name,
- virDomainHostdevDefPtr *hostdevs,
- int nhostdevs);
+ usbDeviceList *list);
int qemuPrepareHostDevices(struct qemud_driver *driver,
virDomainDefPtr def);
void qemuReattachPciDevice(pciDevice *dev, struct qemud_driver *driver);
3:51 a.m.
New subject: [libvirt] [PATCH v2 2/3] qemu: make use of usb search function to initialize usb devices
On 05/03/2012 01:45 AM, Osier Yang wrote:
On 2012年05月01日 16:16, Guannan Ren wrote:
> refactor qemuPrepareHostdevUSBDevices function, make it focus on
> adding usb device to activeUsbHostdevs after check. After that,
> the usb hotplug function qemuDomainAttachHostDevice also could use
> it.
>
> expand qemuPrepareHostUSBDevices to perform the usb search,
> rollback on failure.
> ---
> src/qemu/qemu_hostdev.c | 118
> ++++++++++++++++++++++++++++++-----------------
> src/qemu/qemu_hostdev.h | 3 +-
> 2 files changed, 76 insertions(+), 45 deletions(-)
>
> diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
> index 8594fb2..a3d4ad4 100644
> --- a/src/qemu/qemu_hostdev.c
> +++ b/src/qemu/qemu_hostdev.c
> @@ -565,13 +565,51 @@ qemuPrepareHostPCIDevices(struct qemud_driver
> *driver,
> int
> qemuPrepareHostdevUSBDevices(struct qemud_driver *driver,
> const char *name,
> - virDomainHostdevDefPtr *hostdevs,
> - int nhostdevs)
> + usbDeviceList *list)
> {
> - int ret = -1;
> int i;
> + usbDevice *tmp;
> +
> + for (i = 0; i< usbDeviceListCount(list); i++) {
> + usbDevice *usb = usbDeviceListGet(list, i);
> + if ((tmp = usbDeviceListFind(driver->activeUsbHostdevs,
> usb))) {
> + const char *other_name = usbDeviceGetUsedBy(tmp);
> +
> + if (other_name)
> + qemuReportError(VIR_ERR_OPERATION_INVALID,
> + _("USB device %s is in use by domain
> %s"),
> + usbDeviceGetName(tmp), other_name);
> + else
> + qemuReportError(VIR_ERR_OPERATION_INVALID,
> + _("USB device %s is already in use"),
> + usbDeviceGetName(tmp));
> + return -1;
> + }
> +
> + usbDeviceSetUsedBy(usb, name);
> + VIR_DEBUG("Adding %03d.%03d dom=%s to activeUsbHostdevs",
> + usbDeviceGetBus(usb), usbDeviceGetDevno(usb), name);
> + /*
> + * The caller is responsible to steal the list of usb
> + * from the usbDeviceList that passed in on success,
s/steal the list of usb/usb devices which are used by domain/.
we add usb devices into driver->activeUsbHostdevs from the list
that passed in.
we need to steal these usb devices from arguments list in order
not to be freed
s/steal the list of usb/steal the usb devices/
> + * perform rollback on failure.
> + */
> + if (usbDeviceListAdd(driver->activeUsbHostdevs, usb)< 0)
> + return -1;
> +
> + }
> + return 0;
> +}
> +
> +static int
> +qemuPrepareHostUSBDevices(struct qemud_driver *driver,
> + virDomainDefPtr def)
It's confused with qemuPrepareHostdevUSBDevices, the idea to
have a general function (new qemuPrepareHostdevUSBDevices) for
reusing is good. But we should have another name for it instead,
and keep "qemuPrepareHostdevUSBDevices" here.
The purpose of "qemuPrepareHostdevUSBDevices" is add usb devices
into qemu driver which is not changed.
I just move the action of usb device search up, original
function did two job
search and add, currently, it only has "add".
The code has been fixed according to the rest of comments.
Thanks for the review.
Guannan Ren
3:16 a.m.
New subject: [libvirt] [PATCH v2 3/3] qemu: search usb device accurately to improve usb device hotplug
One usb device could be allowed to hotplug in at a time. If user
give a xml as follows. Probably there are two usb devices avaiable
but with different value of "bus, device"
we give a error to let user use <address> to specify the desired one.
<hostdev mode='subsystem' type='usb' managed='yes'>
<source>
<vendor id='0x15e1'/>
<product id='0x2007'/>
</source>
</hostdev>
---
src/qemu/qemu_hotplug.c | 68 +++++++++++++++++++++++++++++++++++++---------
1 files changed, 54 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 7cf7b90..15693a0 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1121,6 +1121,9 @@ int qemuDomainAttachHostDevice(struct qemud_driver *driver,
virDomainObjPtr vm,
virDomainHostdevDefPtr hostdev)
{
+ usbDeviceList *list;
+ usbDevice * usb = NULL;
+
if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("hostdev mode '%s' not supported"),
@@ -1128,29 +1131,62 @@ int qemuDomainAttachHostDevice(struct qemud_driver *driver,
return -1;
}
- /* Resolve USB product/vendor to bus/device */
- if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB &&
- hostdev->source.subsys.u.usb.vendor) {
- if (qemuPrepareHostdevUSBDevices(driver, vm->def->name, &hostdev, 1)
< 0)
- goto error;
+ if (!(list = usbDeviceListNew()))
+ goto cleanup;
+
+ if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) {
+ unsigned vendor = hostdev->source.subsys.u.usb.vendor;
+ unsigned product = hostdev->source.subsys.u.usb.product;
+ unsigned bus = hostdev->source.subsys.u.usb.bus;
+ unsigned device = hostdev->source.subsys.u.usb.device;
+
+ if (vendor && bus) {
+ usb = usbFindDevice(vendor, product, bus, device);
+ if (!usb)
+ goto cleanup;
+
+ } else if (vendor && !bus) {
+ usbDeviceList *devs = usbFindDevByVendor(vendor, product);
+ if (!devs)
+ goto cleanup;
+
+ if (usbDeviceListCount(devs) > 1) {
+ qemuReportError(VIR_ERR_XML_ERROR,
+ _("multiple USB deivces %x:%x, "
+ "use <address> to specify one"),
vendor, product);
+ usbDeviceListFree(devs);
+ goto cleanup;
+ }
+ usb = usbDeviceListGet(devs, 0);
+ usbDeviceListSteal(devs, usb);
+ usbDeviceListFree(devs);
- usbDevice *usb
- = usbFindDevice(hostdev->source.subsys.u.usb.vendor,
- hostdev->source.subsys.u.usb.product);
+ hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb);
+ hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb);
+
+ } else if (!vendor && bus) {
+ usb = usbFindDevByBus(bus, device);
+ if (!usb)
+ goto cleanup;
+ }
if (!usb)
- return -1;
+ goto cleanup;
- hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb);
- hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb);
+ if (usbDeviceListAdd(list, usb) < 0) {
+ usbFreeDevice(usb);
+ goto cleanup;
+ }
- usbFreeDevice(usb);
- }
+ if (qemuPrepareHostdevUSBDevices(driver, vm->def->name, list) < 0)
+ goto cleanup;
+ usbDeviceListSteal(list, usb);
+ }
if (virSecurityManagerSetHostdevLabel(driver->securityManager,
vm->def, hostdev) < 0)
- return -1;
+ goto cleanup;
switch (hostdev->source.subsys.type) {
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
@@ -1172,6 +1208,7 @@ int qemuDomainAttachHostDevice(struct qemud_driver *driver,
goto error;
}
+ usbDeviceListFree(list);
return 0;
error:
@@ -1179,6 +1216,9 @@ error:
vm->def, hostdev) < 0)
VIR_WARN("Unable to restore host device labelling on hotplug fail");
+cleanup:
+ usbDeviceListFree(list);
+ usbDeviceListSteal(driver->activeUsbHostdevs, usb);
return -1;
}
--
1.7.7.5
10:30 a.m.
New subject: [libvirt] [PATCH v2 3/3] qemu: search usb device accurately to improve usb device hotplug
On 05/01/2012 10:16 AM, Guannan Ren wrote:
One usb device could be allowed to hotplug in at a time. If user
give a xml as follows. Probably there are two usb devices avaiable
but with different value of "bus, device"
we give a error to let user use <address> to specify the desired one.
<hostdev mode='subsystem' type='usb' managed='yes'>
<source>
<vendor id='0x15e1'/>
<product id='0x2007'/>
</source>
</hostdev>
---
src/qemu/qemu_hotplug.c | 68 +++++++++++++++++++++++++++++++++++++---------
1 files changed, 54 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 7cf7b90..15693a0 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1121,6 +1121,9 @@ int qemuDomainAttachHostDevice(struct qemud_driver *driver,
virDomainObjPtr vm,
virDomainHostdevDefPtr hostdev)
{
+ usbDeviceList *list;
+ usbDevice * usb = NULL;
+
if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("hostdev mode '%s' not supported"),
@@ -1128,29 +1131,62 @@ int qemuDomainAttachHostDevice(struct qemud_driver *driver,
return -1;
}
- /* Resolve USB product/vendor to bus/device */
- if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB &&
- hostdev->source.subsys.u.usb.vendor) {
- if (qemuPrepareHostdevUSBDevices(driver, vm->def->name, &hostdev, 1)
< 0)
- goto error;
+ if (!(list = usbDeviceListNew()))
+ goto cleanup;
+
+ if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) {
+ unsigned vendor = hostdev->source.subsys.u.usb.vendor;
+ unsigned product = hostdev->source.subsys.u.usb.product;
+ unsigned bus = hostdev->source.subsys.u.usb.bus;
+ unsigned device = hostdev->source.subsys.u.usb.device;
+
+ if (vendor && bus) {
+ usb = usbFindDevice(vendor, product, bus, device);
+ if (!usb)
+ goto cleanup;
+
+ } else if (vendor && !bus) {
+ usbDeviceList *devs = usbFindDevByVendor(vendor, product);
+ if (!devs)
+ goto cleanup;
+
+ if (usbDeviceListCount(devs) > 1) {
+ qemuReportError(VIR_ERR_XML_ERROR,
+ _("multiple USB deivces %x:%x, "
+ "use <address> to specify one"),
vendor, product);
+ usbDeviceListFree(devs);
+ goto cleanup;
+ }
+ usb = usbDeviceListGet(devs, 0);
+ usbDeviceListSteal(devs, usb);
+ usbDeviceListFree(devs);
- usbDevice *usb
- = usbFindDevice(hostdev->source.subsys.u.usb.vendor,
- hostdev->source.subsys.u.usb.product);
+ hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb);
+ hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb);
+
+ } else if (!vendor && bus) {
+ usb = usbFindDevByBus(bus, device);
+ if (!usb)
+ goto cleanup;
+ }
if (!usb)
- return -1;
+ goto cleanup;
- hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb);
- hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb);
+ if (usbDeviceListAdd(list, usb) < 0) {
+ usbFreeDevice(usb);
+ goto cleanup;
+ }
- usbFreeDevice(usb);
- }
+ if (qemuPrepareHostdevUSBDevices(driver, vm->def->name, list) < 0)
+ goto cleanup;
+ usbDeviceListSteal(list, usb);
+ }
if (virSecurityManagerSetHostdevLabel(driver->securityManager,
vm->def, hostdev) < 0)
- return -1;
+ goto cleanup;
switch (hostdev->source.subsys.type) {
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
@@ -1172,6 +1208,7 @@ int qemuDomainAttachHostDevice(struct qemud_driver *driver,
goto error;
}
+ usbDeviceListFree(list);
return 0;
error:
@@ -1179,6 +1216,9 @@ error:
vm->def, hostdev) < 0)
VIR_WARN("Unable to restore host device labelling on hotplug fail");
+cleanup:
+ usbDeviceListFree(list);
+ usbDeviceListSteal(driver->activeUsbHostdevs, usb);
return -1;
}
Seems all is well for me, ACK.
Martin
12:31 p.m.
New subject: [libvirt] [PATCH v2 3/3] qemu: search usb device accurately to improve usb device hotplug
On 05/01/2012 02:16 AM, Guannan Ren wrote:
One usb device could be allowed to hotplug in at a time. If user
give a xml as follows. Probably there are two usb devices avaiable
s/avaiable/available/
but with different value of "bus, device"
we give a error to let user use <address> to specify the desired one.
<hostdev mode='subsystem' type='usb' managed='yes'>
<source>
<vendor id='0x15e1'/>
<product id='0x2007'/>
</source>
</hostdev>
---
src/qemu/qemu_hotplug.c | 68 +++++++++++++++++++++++++++++++++++++---------
1 files changed, 54 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 7cf7b90..15693a0 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1121,6 +1121,9 @@ int qemuDomainAttachHostDevice(struct qemud_driver *driver,
virDomainObjPtr vm,
virDomainHostdevDefPtr hostdev)
{
+ usbDeviceList *list;
+ usbDevice * usb = NULL;
Style: no space after *.
+ if (!(list = usbDeviceListNew()))
+ goto cleanup;
virReportOOMError()
+
+ if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) {
+ unsigned vendor = hostdev->source.subsys.u.usb.vendor;
+ unsigned product = hostdev->source.subsys.u.usb.product;
+ unsigned bus = hostdev->source.subsys.u.usb.bus;
+ unsigned device = hostdev->source.subsys.u.usb.device;
+
+ if (vendor && bus) {
Same question as in 2/3 about valid values for bus.
+ if (usbDeviceListCount(devs) > 1) {
+ qemuReportError(VIR_ERR_XML_ERROR,
+ _("multiple USB deivces %x:%x, "
s/deivces/devices/
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
3:08 a.m.
New subject: [libvirt] [PATCH v2 3/3] qemu: search usb device accurately to improve usb device hotplug
On 05/03/2012 01:31 AM, Eric Blake wrote:
On 05/01/2012 02:16 AM, Guannan Ren wrote:
> One usb device could be allowed to hotplug in at a time. If user
> give a xml as follows. Probably there are two usb devices avaiable
s/avaiable/available/
> but with different value of "bus, device"
>
> we give a error to let user use<address> to specify the desired one.
>
> <hostdev mode='subsystem' type='usb' managed='yes'>
> <source>
> <vendor id='0x15e1'/>
> <product id='0x2007'/>
> </source>
> </hostdev>
> ---
> src/qemu/qemu_hotplug.c | 68 +++++++++++++++++++++++++++++++++++++---------
> 1 files changed, 54 insertions(+), 14 deletions(-)
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 7cf7b90..15693a0 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1121,6 +1121,9 @@ int qemuDomainAttachHostDevice(struct qemud_driver *driver,
> virDomainObjPtr vm,
> virDomainHostdevDefPtr hostdev)
> {
> + usbDeviceList *list;
> + usbDevice * usb = NULL;
Style: no space after *.
> + if (!(list = usbDeviceListNew()))
> + goto cleanup;
virReportOOMError()
OOM error will be raised in usbDeviceListNew()
we get 'NULL' value in that case.
> +
> + if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) {
> + unsigned vendor = hostdev->source.subsys.u.usb.vendor;
> + unsigned product = hostdev->source.subsys.u.usb.product;
> + unsigned bus = hostdev->source.subsys.u.usb.bus;
> + unsigned device = hostdev->source.subsys.u.usb.device;
> +
> + if (vendor&& bus) {
Same question as in 2/3 about valid values for bus.
> + if (usbDeviceListCount(devs)> 1) {
> + qemuReportError(VIR_ERR_XML_ERROR,
> + _("multiple USB deivces %x:%x, "
s/deivces/devices/
1:06 p.m.
New subject: [libvirt] [PATCH v2 3/3] qemu: search usb device accurately to improve usb device hotplug
On 2012年05月01日 16:16, Guannan Ren wrote:
One usb device could be allowed to hotplug in at a time. If user
give a xml as follows.
s/give/gives/
Probably there are two usb devices avaiable
s/avaiable/available/
but with different value of "bus, device"
we give a error to let user use<address> to specify the desired one.
<hostdev mode='subsystem' type='usb' managed='yes'>
<source>
<vendor id='0x15e1'/>
<product id='0x2007'/>
</source>
</hostdev>
---
src/qemu/qemu_hotplug.c | 68 +++++++++++++++++++++++++++++++++++++---------
1 files changed, 54 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 7cf7b90..15693a0 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1121,6 +1121,9 @@ int qemuDomainAttachHostDevice(struct qemud_driver *driver,
virDomainObjPtr vm,
virDomainHostdevDefPtr hostdev)
{
+ usbDeviceList *list;
+ usbDevice * usb = NULL;
s/usbDevice * usb/usbDevice *usb/
+
if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("hostdev mode '%s' not supported"),
@@ -1128,29 +1131,62 @@ int qemuDomainAttachHostDevice(struct qemud_driver *driver,
return -1;
}
- /* Resolve USB product/vendor to bus/device */
- if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB&&
- hostdev->source.subsys.u.usb.vendor) {
- if (qemuPrepareHostdevUSBDevices(driver, vm->def->name,&hostdev,
1)< 0)
- goto error;
+ if (!(list = usbDeviceListNew()))
virReportOOMError()
+ goto cleanup;
+
+ if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) {
+ unsigned vendor = hostdev->source.subsys.u.usb.vendor;
+ unsigned product = hostdev->source.subsys.u.usb.product;
+ unsigned bus = hostdev->source.subsys.u.usb.bus;
+ unsigned device = hostdev->source.subsys.u.usb.device;
+
+ if (vendor&& bus) {
+ usb = usbFindDevice(vendor, product, bus, device);
+ if (!usb)
+ goto cleanup;
Should be removed. See [1]
+
+ } else if (vendor&& !bus) {
+ usbDeviceList *devs = usbFindDevByVendor(vendor, product);
+ if (!devs)
+ goto cleanup;
+
+ if (usbDeviceListCount(devs)> 1) {
+ qemuReportError(VIR_ERR_XML_ERROR,
+ _("multiple USB deivces %x:%x, "
+ "use<address> to specify one"),
vendor, product);
+ usbDeviceListFree(devs);
+ goto cleanup;
+ }
+ usb = usbDeviceListGet(devs, 0);
+ usbDeviceListSteal(devs, usb);
+ usbDeviceListFree(devs);
- usbDevice *usb
- = usbFindDevice(hostdev->source.subsys.u.usb.vendor,
- hostdev->source.subsys.u.usb.product);
+ hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb);
+ hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb);
+
+ } else if (!vendor&& bus) {
+ usb = usbFindDevByBus(bus, device);
+ if (!usb)
+ goto cleanup;
Should be removed. see [1]
+ }
if (!usb)
- return -1;
+ goto cleanup;
[1] There is a duplicate check here.
- hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb);
- hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb);
+ if (usbDeviceListAdd(list, usb)< 0) {
+ usbFreeDevice(usb);
+ goto cleanup;
+ }
- usbFreeDevice(usb);
- }
+ if (qemuPrepareHostdevUSBDevices(driver, vm->def->name, list)< 0)
+ goto cleanup;
Do we really need a list here? there is only found usb device is , I guess
the only purpose is to take use of the new helper function
qemuPrepareHostdevUSBDevices, I.e. to use the new helper function,
it create a new list, and the only one usb device is inserted to
and removed from the list. IMO it's a waste.
+ usbDeviceListSteal(list, usb);
+ }
if (virSecurityManagerSetHostdevLabel(driver->securityManager,
vm->def, hostdev)< 0)
- return -1;
+ goto cleanup;
switch (hostdev->source.subsys.type) {
case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
@@ -1172,6 +1208,7 @@ int qemuDomainAttachHostDevice(struct qemud_driver *driver,
goto error;
}
+ usbDeviceListFree(list);
return 0;
error:
@@ -1179,6 +1216,9 @@ error:
vm->def, hostdev)< 0)
VIR_WARN("Unable to restore host device labelling on hotplug fail");
+cleanup:
+ usbDeviceListFree(list);
+ usbDeviceListSteal(driver->activeUsbHostdevs, usb);
return -1;
}
1:11 p.m.
New subject: [libvirt] [PATCH v2 3/3] qemu: search usb device accurately to improve usb device hotplug
On 2012年05月03日 02:06, Osier Yang wrote:
On 2012年05月01日 16:16, Guannan Ren wrote:
> One usb device could be allowed to hotplug in at a time. If user
> give a xml as follows.
s/give/gives/
Probably there are two usb devices avaiable
s/avaiable/available/
> but with different value of "bus, device"
>
> we give a error to let user use<address> to specify the desired one.
>
> <hostdev mode='subsystem' type='usb' managed='yes'>
> <source>
> <vendor id='0x15e1'/>
> <product id='0x2007'/>
> </source>
> </hostdev>
> ---
> src/qemu/qemu_hotplug.c | 68
> +++++++++++++++++++++++++++++++++++++---------
> 1 files changed, 54 insertions(+), 14 deletions(-)
>
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 7cf7b90..15693a0 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -1121,6 +1121,9 @@ int qemuDomainAttachHostDevice(struct
> qemud_driver *driver,
> virDomainObjPtr vm,
> virDomainHostdevDefPtr hostdev)
> {
> + usbDeviceList *list;
> + usbDevice * usb = NULL;
s/usbDevice * usb/usbDevice *usb/
> +
> if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) {
> qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("hostdev mode '%s' not supported"),
> @@ -1128,29 +1131,62 @@ int qemuDomainAttachHostDevice(struct
> qemud_driver *driver,
> return -1;
> }
>
> - /* Resolve USB product/vendor to bus/device */
> - if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB&&
> - hostdev->source.subsys.u.usb.vendor) {
> - if (qemuPrepareHostdevUSBDevices(driver, vm->def->name,&hostdev, 1)<
0)
> - goto error;
> + if (!(list = usbDeviceListNew()))
virReportOOMError()
> + goto cleanup;
> +
> + if (hostdev->source.subsys.type ==
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) {
> + unsigned vendor = hostdev->source.subsys.u.usb.vendor;
> + unsigned product = hostdev->source.subsys.u.usb.product;
> + unsigned bus = hostdev->source.subsys.u.usb.bus;
> + unsigned device = hostdev->source.subsys.u.usb.device;
> +
> + if (vendor&& bus) {
> + usb = usbFindDevice(vendor, product, bus, device);
> + if (!usb)
> + goto cleanup;
Should be removed. See [1]
> +
> + } else if (vendor&& !bus) {
> + usbDeviceList *devs = usbFindDevByVendor(vendor, product);
> + if (!devs)
> + goto cleanup;
> +
> + if (usbDeviceListCount(devs)> 1) {
> + qemuReportError(VIR_ERR_XML_ERROR,
> + _("multiple USB deivces %x:%x, "
> + "use<address> to specify one"), vendor, product);
> + usbDeviceListFree(devs);
> + goto cleanup;
> + }
> + usb = usbDeviceListGet(devs, 0);
> + usbDeviceListSteal(devs, usb);
> + usbDeviceListFree(devs);
>
> - usbDevice *usb
> - = usbFindDevice(hostdev->source.subsys.u.usb.vendor,
> - hostdev->source.subsys.u.usb.product);
> + hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb);
> + hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb);
> +
> + } else if (!vendor&& bus) {
> + usb = usbFindDevByBus(bus, device);
> + if (!usb)
> + goto cleanup;
Should be removed. see [1]
> + }
>
> if (!usb)
> - return -1;
> + goto cleanup;
[1] There is a duplicate check here.
>
> - hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb);
> - hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb);
> + if (usbDeviceListAdd(list, usb)< 0) {
> + usbFreeDevice(usb);
> + goto cleanup;
> + }
>
> - usbFreeDevice(usb);
> - }
> + if (qemuPrepareHostdevUSBDevices(driver, vm->def->name, list)< 0)
> + goto cleanup;
Do we really need a list here? there is only found usb device is , I guess
the only purpose is to take use of the new helper function
qemuPrepareHostdevUSBDevices, I.e. to use the new helper function,
it create a new list, and the only one usb device is inserted to
and removed from the list. IMO it's a waste.
I.e. Given the name of the new helper function is confused with
"qemuPrepareHostUSBDevices", and it's actually no need to use it
here, IMO the new helper function is no need.
>
> + usbDeviceListSteal(list, usb);
> + }
>
> if (virSecurityManagerSetHostdevLabel(driver->securityManager,
> vm->def, hostdev)< 0)
> - return -1;
> + goto cleanup;
>
> switch (hostdev->source.subsys.type) {
> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
> @@ -1172,6 +1208,7 @@ int qemuDomainAttachHostDevice(struct
> qemud_driver *driver,
> goto error;
> }
>
> + usbDeviceListFree(list);
> return 0;
>
> error:
> @@ -1179,6 +1216,9 @@ error:
> vm->def, hostdev)< 0)
> VIR_WARN("Unable to restore host device labelling on hotplug fail");
>
> +cleanup:
> + usbDeviceListFree(list);
> + usbDeviceListSteal(driver->activeUsbHostdevs, usb);
> return -1;
> }
>
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
4:03 a.m.
New subject: [libvirt] [PATCH v2 3/3] qemu: search usb device accurately to improve usb device hotplug
On 05/03/2012 02:06 AM, Osier Yang wrote:
On 2012年05月01日 16:16, Guannan Ren wrote:
> One usb device could be allowed to hotplug in at a time. If user
> give a xml as follows.
s/give/gives/
Probably there are two usb devices avaiable
s/avaiable/available/
> but with different value of "bus, device"
>
> we give a error to let user use<address> to specify the desired one.
>
> <hostdev mode='subsystem' type='usb' managed='yes'>
> <source>
> <vendor id='0x15e1'/>
> <product id='0x2007'/>
> </source>
> </hostdev>
> ---
>
> - hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb);
> - hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb);
> + if (usbDeviceListAdd(list, usb)< 0) {
> + usbFreeDevice(usb);
> + goto cleanup;
> + }
>
> - usbFreeDevice(usb);
> - }
> + if (qemuPrepareHostdevUSBDevices(driver, vm->def->name,
> list)< 0)
> + goto cleanup;
Do we really need a list here? there is only found usb device is , I
guess
the only purpose is to take use of the new helper function
qemuPrepareHostdevUSBDevices, I.e. to use the new helper function,
it create a new list, and the only one usb device is inserted to
and removed from the list. IMO it's a waste.
qemuPrepareHostUSBDevices probably have multiple devices to add
to qemu driver
Usb hotplug shares the function for the job, that's it.
Guannan Ren
11:26 a.m.
On 2012年05月01日 16:16, Guannan Ren wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=815755
The set of patch tries to fix the issue when multiple usb devices with
same idVendor, idProduct are availible on host, the usb device with
lowest bus:device will be attached to guest if usb xml file is given like
this:
<hostdev mode='subsystem' type='usb' managed='yes'>
<source>
<vendor id='0x15e1'/>
<product id='0x2007'/>
</source>
</hostdev>
The reason is that the usb hotplug function searchs usb device in system files
to match vendor and product id, the file with lowest number is always found
first.
After fix, in this case, libvirt will report an error as follows:
At the same time, the usb part of domain initilization is also update in patch 2/3
# virsh attach-device rhel6u1 /tmp/usb.xml
error: Failed to attach device from /tmp/usb.xml
error: XML error: multiple USB deivces 15e1:2007, use<address> to specify one.
The error could be more clear, e.g.
multiple USB deivces for vendor='15e1',
product='2007', use <address> to specify one.
And it's not "XML error", the XML is just fine. The XML with
only 'vendor' and 'product' specified could still work well
if no mulitple devices with the same vendor and product.
Perhaps VIR_ERR_INTERNAL_ERROR is a good choice.
Regards,
Osier
11:35 a.m.
On 05/02/2012 10:26 AM, Osier Yang wrote:
The error could be more clear, e.g.
multiple USB deivces for vendor='15e1',
product='2007', use <address> to specify one.
And it's not "XML error", the XML is just fine. The XML with
only 'vendor' and 'product' specified could still work well
if no mulitple devices with the same vendor and product.
Perhaps VIR_ERR_INTERNAL_ERROR is a good choice.
No, because it's not an internal libvirt problem, but an ambiguous user
input problem. I'd recommend VIR_ERR_INVALID_ARG (invalid because it
was ambiguous) or VIR_ERR_OPERATION_FAILED (failed because we didn't
have enough information).
--
Eric Blake eblake(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
11:37 a.m.
On 2012年05月03日 00:35, Eric Blake wrote:
On 05/02/2012 10:26 AM, Osier Yang wrote:
> The error could be more clear, e.g.
>
> multiple USB deivces for vendor='15e1',
> product='2007', use<address> to specify one.
>
> And it's not "XML error", the XML is just fine. The XML with
> only 'vendor' and 'product' specified could still work well
> if no mulitple devices with the same vendor and product.
>
> Perhaps VIR_ERR_INTERNAL_ERROR is a good choice.
No, because it's not an internal libvirt problem, but an ambiguous user
input problem. I'd recommend VIR_ERR_INVALID_ARG (invalid because it
was ambiguous) or VIR_ERR_OPERATION_FAILED (failed because we didn't
have enough information).
I considered VIR_ERR_OPERATION_FAILED too, so +1 for it, :-)
Osier
4586
days inactive
4588
days old
21 comments
4 participants
participants (4)
-
Eric Blake -
Guannan Ren -
Martin Kletzander -
Osier Yang