[libvirt] [PATCH v3 0/3] usb devices with same vendorID, productID hotplug support
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 like: # virsh attach-device rhel6u1 /tmp/usb.xml error: Failed to attach device from /tmp/usb.xml error: operation failed: multiple USB devices for 15e1:2007, use <address> to specify one At the same time, the usb part of domain initilization is also updated in patch 2/3 These patches 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='7'/> </source> </hostdev> # virsh attach-device rhel6u12nd /tmp/usb_by_bus.xml error: Failed to attach device from /tmp/usb_by_bus.xml error: Requested operation is not valid: USB device 006:007 is in use by domain rhel6u1
usbFindDevice():get usb device according to idVendor, idProduct, bus, device it is the exact match of the four parameters usbFindDeviceByBus():get usb device according to bus, device it returns only one usb device same as usbFindDevice usbFindDeviceByVendor():get usb device according to idVendor,idProduct it probably returns multiple usb devices. usbDeviceSearch(): a helper function to do the actual search --- src/libvirt_private.syms | 2 + src/util/hostusb.c | 209 +++++++++++++++++++++++++++++++++------------- src/util/hostusb.h | 22 ++++-- 3 files changed, 170 insertions(+), 63 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 025816a..b9f9015 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1084,6 +1084,8 @@ usbDeviceListGet; usbDeviceListNew; usbDeviceListSteal; usbDeviceSetUsedBy; +usbFindDeviceByBus; +usbFindDeviceByVendor; usbFindDevice; usbFreeDevice; usbGetDevice; diff --git a/src/util/hostusb.c b/src/util/hostusb.c index 92f52a2..cad0a6c 100644 --- a/src/util/hostusb.c +++ b/src/util/hostusb.c @@ -42,9 +42,16 @@ #define USB_ID_LEN 10 /* "1234 5678" */ #define USB_ADDR_LEN 8 /* "123:456" */ +/* For virReportOOMError() and virReportSystemError() */ +#define VIR_FROM_THIS VIR_FROM_NONE + +#define usbReportError(code, ...) \ + virReportErrorHelper(VIR_FROM_NONE, code, __FILE__, \ + __FUNCTION__, __LINE__, __VA_ARGS__) + struct _usbDevice { - unsigned bus; - unsigned dev; + unsigned int bus; + unsigned int dev; char name[USB_ADDR_LEN]; /* domain:bus:slot.function */ char id[USB_ID_LEN]; /* product vendor */ @@ -57,15 +64,14 @@ struct _usbDeviceList { usbDevice **devs; }; -/* For virReportOOMError() and virReportSystemError() */ -#define VIR_FROM_THIS VIR_FROM_NONE - -#define usbReportError(code, ...) \ - virReportErrorHelper(VIR_FROM_NONE, code, __FILE__, \ - __FUNCTION__, __LINE__, __VA_ARGS__) +typedef enum { + USB_DEVICE_ALL = 0, + USB_DEVICE_FIND_BY_VENDOR = 1, + USB_DEVICE_FIND_BY_BUS = 2, +} usbDeviceFindFlags; static int usbSysReadFile(const char *f_name, const char *d_name, - int base, unsigned *value) + int base, unsigned int *value) { int ret = -1, tmp; char *buf = NULL; @@ -94,13 +100,22 @@ cleanup: return ret; } -static int usbFindBusByVendor(unsigned vendor, unsigned product, - unsigned *bus, unsigned *devno) +static usbDeviceList * +usbDeviceSearch(unsigned int vendor, + unsigned int product, + unsigned int bus, + unsigned int devno, + unsigned int flags) { DIR *dir = NULL; - int ret = -1, found = 0; + bool found = false; 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 +126,71 @@ static int usbFindBusByVendor(unsigned vendor, unsigned product, } while ((de = readdir(dir))) { - unsigned found_prod, found_vend; + unsigned int 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 (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; + + /* + * Don't set found to true in order to continue the loop + * to find multiple USB devices with same idVendor and idProduct + */ + if (flags && !(flags & ~USB_DEVICE_FIND_BY_VENDOR)) + if (found_prod != product || found_vend != vendor) + continue; + + if (flags && !(flags & ~USB_DEVICE_FIND_BY_BUS)) { + if (found_bus != bus || found_devno != devno) + continue; + found = true; + } - if (STRPREFIX(de->d_name, "usb")) - tmpstr += 3; + if ((flags & USB_DEVICE_FIND_BY_BUS) + && (flags & USB_DEVICE_FIND_BY_VENDOR)) { + if (found_prod != product || + found_vend != vendor || + found_bus != bus || + found_devno != devno) + continue; + found = true; + } - 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; - } + usb = usbGetDevice(found_bus, found_devno); + if (!usb) + goto cleanup; - if (usbSysReadFile("devnum", de->d_name, - 10, &found_addr) < 0) - goto cleanup; + if (usbDeviceListAdd(list, usb) < 0) { + usbFreeDevice(usb); + goto cleanup; + } - *bus = found_bus; - *devno = found_addr; - found = 1; + if (found) break; - } } - - if (!found) - usbReportError(VIR_ERR_INTERNAL_ERROR, - _("Did not find USB device %x:%x"), vendor, product); - else - ret = 0; + ret = list; cleanup: if (dir) { @@ -160,12 +198,84 @@ cleanup: closedir (dir); errno = saved_errno; } + + if (!ret) + usbDeviceListFree(list); return ret; } +usbDeviceList * +usbFindDeviceByVendor(unsigned int 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 * -usbGetDevice(unsigned bus, - unsigned devno) +usbFindDeviceByBus(unsigned int bus, unsigned devno) +{ + usbDevice *usb; + 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; + } + + usb = usbDeviceListGet(list, 0); + usbDeviceListSteal(list, usb); + usbDeviceListFree(list); + + return usb; +} + +usbDevice * +usbFindDevice(unsigned int vendor, + unsigned int product, + unsigned int bus, + unsigned int devno) +{ + usbDevice *usb; + usbDeviceList *list; + + unsigned int flags = USB_DEVICE_FIND_BY_VENDOR|USB_DEVICE_FIND_BY_BUS; + if (!(list = usbDeviceSearch(vendor, product, bus, devno, flags))) + 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; + } + + usb = usbDeviceListGet(list, 0); + usbDeviceListSteal(list, usb); + usbDeviceListFree(list); + + return usb; +} + +usbDevice * +usbGetDevice(unsigned int bus, + unsigned int devno) { usbDevice *dev; @@ -207,21 +317,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) { @@ -247,13 +342,13 @@ const char *usbDeviceGetName(usbDevice *dev) return dev->name; } -unsigned usbDeviceGetBus(usbDevice *dev) +unsigned int usbDeviceGetBus(usbDevice *dev) { return dev->bus; } -unsigned usbDeviceGetDevno(usbDevice *dev) +unsigned int usbDeviceGetDevno(usbDevice *dev) { return dev->dev; } diff --git a/src/util/hostusb.h b/src/util/hostusb.h index afaa32f..0c43e3f 100644 --- a/src/util/hostusb.h +++ b/src/util/hostusb.h @@ -28,17 +28,27 @@ typedef struct _usbDevice usbDevice; typedef struct _usbDeviceList usbDeviceList; -usbDevice *usbGetDevice(unsigned bus, - unsigned devno); -usbDevice *usbFindDevice(unsigned vendor, - unsigned product); +usbDevice *usbGetDevice(unsigned int bus, + unsigned int devno); + +usbDevice * usbFindDeviceByBus(unsigned int bus, + unsigned int devno); + +usbDeviceList * usbFindDeviceByVendor(unsigned int vendor, + unsigned int product); + +usbDevice * usbFindDevice(unsigned int vendor, + unsigned int product, + unsigned int bus, + unsigned int devno); + void usbFreeDevice (usbDevice *dev); void usbDeviceSetUsedBy(usbDevice *dev, const char *name); const char *usbDeviceGetUsedBy(usbDevice *dev); const char *usbDeviceGetName(usbDevice *dev); -unsigned usbDeviceGetBus(usbDevice *dev); -unsigned usbDeviceGetDevno(usbDevice *dev); +unsigned int usbDeviceGetBus(usbDevice *dev); +unsigned int usbDeviceGetDevno(usbDevice *dev); /* * Callback that will be invoked once for each file -- 1.7.7.5
On 05/03/2012 04:51 AM, Guannan Ren wrote:
usbFindDevice():get usb device according to idVendor, idProduct, bus, device it is the exact match of the four parameters
usbFindDeviceByBus():get usb device according to bus, device it returns only one usb device same as usbFindDevice
usbFindDeviceByVendor():get usb device according to idVendor,idProduct it probably returns multiple usb devices.
usbDeviceSearch(): a helper function to do the actual search --- src/libvirt_private.syms | 2 + src/util/hostusb.c | 209 +++++++++++++++++++++++++++++++++------------- src/util/hostusb.h | 22 ++++-- 3 files changed, 170 insertions(+), 63 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 025816a..b9f9015 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1084,6 +1084,8 @@ usbDeviceListGet; usbDeviceListNew; usbDeviceListSteal; usbDeviceSetUsedBy; +usbFindDeviceByBus; +usbFindDeviceByVendor; usbFindDevice;
Nit: Prefix sorts before the longer name.
+ + /* + * Don't set found to true in order to continue the loop + * to find multiple USB devices with same idVendor and idProduct + */ + if (flags && !(flags & ~USB_DEVICE_FIND_BY_VENDOR))
Huh? Read literally, that says: If I requested filtering (flags is nonzero), and the filtering that I requested was any bit other than BY_VENDOR, then filter by vendor. But that's not what you want.
+ if (found_prod != product || found_vend != vendor) + continue;
This if condition is filtering - it says, if my vendor doesn't match, then resume the loop on the next element. So you _really_ want: if ((flags & USB_DEVICE_FIND_BY_VENDOR)) && (found_prod != product || found_vend != vendor)) continue;
+ + if (flags && !(flags & ~USB_DEVICE_FIND_BY_BUS)) { + if (found_bus != bus || found_devno != devno) + continue;
Likewise, this should be: if ((flags & USB_DEVICE_FIND_BY_BUS) && (found_bus != bus || found_devno != devno)) continue;
+ found = true; + }
At which point, if you get this far in one iteration of the loop, none of your filtering requests rejected the current device, so you can set found to true.
- if (STRPREFIX(de->d_name, "usb")) - tmpstr += 3; + if ((flags & USB_DEVICE_FIND_BY_BUS) + && (flags & USB_DEVICE_FIND_BY_VENDOR)) { + if (found_prod != product || + found_vend != vendor || + found_bus != bus || + found_devno != devno) + continue; + found = true; + }
And you don't need this clause at all, because you already took care of the filtering up above.
+usbDevice * +usbFindDevice(unsigned int vendor, + unsigned int product, + unsigned int bus, + unsigned int devno) +{ + usbDevice *usb; + usbDeviceList *list; + + unsigned int flags = USB_DEVICE_FIND_BY_VENDOR|USB_DEVICE_FIND_BY_BUS; + if (!(list = usbDeviceSearch(vendor, product, bus, devno, flags)))
This code is then correct if you fix usbDeviceSearch.
+ +usbDevice * usbFindDeviceByBus(unsigned int bus,
No space after * when returning a pointer type.
+ unsigned int devno); + +usbDeviceList * usbFindDeviceByVendor(unsigned int vendor,
Again
+ unsigned int product); + +usbDevice * usbFindDevice(unsigned int vendor,
Again -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/03/2012 03:21 PM, Eric Blake wrote:
On 05/03/2012 04:51 AM, Guannan Ren wrote:
usbFindDevice():get usb device according to idVendor, idProduct, bus, device it is the exact match of the four parameters
usbFindDeviceByBus():get usb device according to bus, device it returns only one usb device same as usbFindDevice
usbFindDeviceByVendor():get usb device according to idVendor,idProduct it probably returns multiple usb devices.
usbDeviceSearch(): a helper function to do the actual search --- src/libvirt_private.syms | 2 + src/util/hostusb.c | 209 +++++++++++++++++++++++++++++++++------------- src/util/hostusb.h | 22 ++++-- 3 files changed, 170 insertions(+), 63 deletions(-)
This patch doesn't even compile! qemu/qemu_hostdev.c: In function 'qemuPrepareHostdevUSBDevices': qemu/qemu_hostdev.c:599:33: error: too few arguments to function 'usbFindDevice' ../src/util/hostusb.h:40:13: note: declared here You can't change the signature of usbFindDevice unless you update all callers in the same patch. By the way, here are some suggestions I have for your patch: diff --git i/src/util/hostusb.c w/src/util/hostusb.c index cad0a6c..533b9c7 100644 --- i/src/util/hostusb.c +++ w/src/util/hostusb.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2011 Red Hat, Inc. + * Copyright (C) 2009-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -65,9 +65,10 @@ struct _usbDeviceList { }; typedef enum { - USB_DEVICE_ALL = 0, - USB_DEVICE_FIND_BY_VENDOR = 1, - USB_DEVICE_FIND_BY_BUS = 2, + USB_DEVICE_ALL = 0, /* no filtering */ + USB_DEVICE_FIND_BY_VENDOR = 1 << 0, /* filter to vendor matches */ + USB_DEVICE_FIND_BY_BUS = 1 << 1, /* filter to bus matches */ + USB_DEVICE_FIND_ONE = 1 << 2, /* filter to first match */ } usbDeviceFindFlags; static int usbSysReadFile(const char *f_name, const char *d_name, @@ -108,7 +109,6 @@ usbDeviceSearch(unsigned int vendor, unsigned int flags) { DIR *dir = NULL; - bool found = false; char *ignore = NULL; struct dirent *de; usbDeviceList *list = NULL, *ret = NULL; @@ -154,29 +154,13 @@ usbDeviceSearch(unsigned int vendor, 10, &found_devno) < 0) goto cleanup; - /* - * Don't set found to true in order to continue the loop - * to find multiple USB devices with same idVendor and idProduct - */ - if (flags && !(flags & ~USB_DEVICE_FIND_BY_VENDOR)) - if (found_prod != product || found_vend != vendor) - continue; - - if (flags && !(flags & ~USB_DEVICE_FIND_BY_BUS)) { - if (found_bus != bus || found_devno != devno) - continue; - found = true; - } + if ((flags & USB_DEVICE_FIND_BY_VENDOR) && + (found_prod != product || found_vend != vendor)) + continue; - if ((flags & USB_DEVICE_FIND_BY_BUS) - && (flags & USB_DEVICE_FIND_BY_VENDOR)) { - if (found_prod != product || - found_vend != vendor || - found_bus != bus || - found_devno != devno) - continue; - found = true; - } + if ((flags & USB_DEVICE_FIND_BY_BUS) && + (found_bus != bus || found_devno != devno)) + continue; usb = usbGetDevice(found_bus, found_devno); if (!usb) @@ -187,7 +171,7 @@ usbDeviceSearch(unsigned int vendor, goto cleanup; } - if (found) + if (flags & USB_DEVICE_FIND_ONE) break; } ret = list; @@ -195,7 +179,7 @@ usbDeviceSearch(unsigned int vendor, cleanup: if (dir) { int saved_errno = errno; - closedir (dir); + closedir(dir); errno = saved_errno; } @@ -209,7 +193,8 @@ usbFindDeviceByVendor(unsigned int vendor, unsigned product) { usbDeviceList *list; - if (!(list = usbDeviceSearch(vendor, product, 0 , 0, USB_DEVICE_FIND_BY_VENDOR))) + if (!(list = usbDeviceSearch(vendor, product, 0 , 0, + USB_DEVICE_FIND_BY_VENDOR))) return NULL; if (list->count == 0) { @@ -228,12 +213,14 @@ usbFindDeviceByBus(unsigned int bus, unsigned devno) usbDevice *usb; usbDeviceList *list; - if (!(list = usbDeviceSearch(0, 0, bus, devno, USB_DEVICE_FIND_BY_BUS))) + if (!(list = usbDeviceSearch(0, 0, bus, devno, + USB_DEVICE_FIND_BY_BUS | USB_DEVICE_FIND_ONE))) return NULL; if (list->count == 0) { usbReportError(VIR_ERR_INTERNAL_ERROR, - _("Did not find USB device bus:%u device:%u"), bus, devno); + _("Did not find USB device bus:%u device:%u"), + bus, devno); usbDeviceListFree(list); return NULL; } @@ -254,7 +241,9 @@ usbFindDevice(unsigned int vendor, usbDevice *usb; usbDeviceList *list; - unsigned int flags = USB_DEVICE_FIND_BY_VENDOR|USB_DEVICE_FIND_BY_BUS; + unsigned int flags = (USB_DEVICE_FIND_BY_VENDOR | + USB_DEVICE_FIND_BY_BUS | + USB_DEVICE_FIND_ONE); if (!(list = usbDeviceSearch(vendor, product, bus, devno, flags))) return NULL; -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/04/2012 05:38 AM, Eric Blake wrote:
On 05/03/2012 03:21 PM, Eric Blake wrote:
On 05/03/2012 04:51 AM, Guannan Ren wrote:
usbFindDevice():get usb device according to idVendor, idProduct, bus, device it is the exact match of the four parameters
usbFindDeviceByBus():get usb device according to bus, device it returns only one usb device same as usbFindDevice
usbFindDeviceByVendor():get usb device according to idVendor,idProduct it probably returns multiple usb devices.
usbDeviceSearch(): a helper function to do the actual search --- src/libvirt_private.syms | 2 + src/util/hostusb.c | 209 +++++++++++++++++++++++++++++++++------------- src/util/hostusb.h | 22 ++++-- 3 files changed, 170 insertions(+), 63 deletions(-) This patch doesn't even compile!
qemu/qemu_hostdev.c: In function 'qemuPrepareHostdevUSBDevices': qemu/qemu_hostdev.c:599:33: error: too few arguments to function 'usbFindDevice' ../src/util/hostusb.h:40:13: note: declared here
You can't change the signature of usbFindDevice unless you update all callers in the same patch.
I compiled it and tested it , then sent it out. I think the changes on usbFindDevice caller is in PATCH2/3 and PATCH 3/3 "usb = usbFindDevice(vendor, product, bus, device);" are there.
By the way, here are some suggestions I have for your patch:
diff --git i/src/util/hostusb.c w/src/util/hostusb.c index cad0a6c..533b9c7 100644 --- i/src/util/hostusb.c +++ w/src/util/hostusb.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2009-2011 Red Hat, Inc. + * Copyright (C) 2009-2012 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -65,9 +65,10 @@ struct _usbDeviceList { };
typedef enum { - USB_DEVICE_ALL = 0, - USB_DEVICE_FIND_BY_VENDOR = 1, - USB_DEVICE_FIND_BY_BUS = 2, + USB_DEVICE_ALL = 0, /* no filtering */ + USB_DEVICE_FIND_BY_VENDOR = 1<< 0, /* filter to vendor matches */ + USB_DEVICE_FIND_BY_BUS = 1<< 1, /* filter to bus matches */ + USB_DEVICE_FIND_ONE = 1<< 2, /* filter to first match */
This patch is great, but "USB_DEVICE_FIND_ONE" is not very useful. The bus, devno number could identify usb device enough, the bool 'found' just stop the loop because we are sure that the rest of usb devices are no need to search.
} usbDeviceFindFlags;
static int usbSysReadFile(const char *f_name, const char *d_name, @@ -108,7 +109,6 @@ usbDeviceSearch(unsigned int vendor, unsigned int flags) { DIR *dir = NULL; - bool found = false; char *ignore = NULL; struct dirent *de; usbDeviceList *list = NULL, *ret = NULL; @@ -154,29 +154,13 @@ usbDeviceSearch(unsigned int vendor, 10,&found_devno)< 0) goto cleanup;
- /* - * Don't set found to true in order to continue the loop - * to find multiple USB devices with same idVendor and idProduct - */ - if (flags&& !(flags& ~USB_DEVICE_FIND_BY_VENDOR)) - if (found_prod != product || found_vend != vendor) - continue; - - if (flags&& !(flags& ~USB_DEVICE_FIND_BY_BUS)) { - if (found_bus != bus || found_devno != devno) - continue; - found = true; - } + if ((flags& USB_DEVICE_FIND_BY_VENDOR)&& + (found_prod != product || found_vend != vendor)) + continue;
- if ((flags& USB_DEVICE_FIND_BY_BUS) -&& (flags& USB_DEVICE_FIND_BY_VENDOR)) { - if (found_prod != product || - found_vend != vendor || - found_bus != bus || - found_devno != devno) - continue; - found = true; - } + if ((flags& USB_DEVICE_FIND_BY_BUS)&& + (found_bus != bus || found_devno != devno)) + continue;
usb = usbGetDevice(found_bus, found_devno); if (!usb) @@ -187,7 +171,7 @@ usbDeviceSearch(unsigned int vendor, goto cleanup; }
- if (found) + if (flags& USB_DEVICE_FIND_ONE) break; } ret = list; @@ -195,7 +179,7 @@ usbDeviceSearch(unsigned int vendor, cleanup: if (dir) { int saved_errno = errno; - closedir (dir); + closedir(dir); errno = saved_errno; }
@@ -209,7 +193,8 @@ usbFindDeviceByVendor(unsigned int vendor, unsigned product) {
usbDeviceList *list; - if (!(list = usbDeviceSearch(vendor, product, 0 , 0, USB_DEVICE_FIND_BY_VENDOR))) + if (!(list = usbDeviceSearch(vendor, product, 0 , 0, + USB_DEVICE_FIND_BY_VENDOR))) return NULL;
if (list->count == 0) { @@ -228,12 +213,14 @@ usbFindDeviceByBus(unsigned int bus, unsigned devno) usbDevice *usb; usbDeviceList *list;
- if (!(list = usbDeviceSearch(0, 0, bus, devno, USB_DEVICE_FIND_BY_BUS))) + if (!(list = usbDeviceSearch(0, 0, bus, devno, + USB_DEVICE_FIND_BY_BUS | USB_DEVICE_FIND_ONE))) return NULL;
if (list->count == 0) { usbReportError(VIR_ERR_INTERNAL_ERROR, - _("Did not find USB device bus:%u device:%u"), bus, devno); + _("Did not find USB device bus:%u device:%u"), + bus, devno); usbDeviceListFree(list); return NULL; } @@ -254,7 +241,9 @@ usbFindDevice(unsigned int vendor, usbDevice *usb; usbDeviceList *list;
- unsigned int flags = USB_DEVICE_FIND_BY_VENDOR|USB_DEVICE_FIND_BY_BUS; + unsigned int flags = (USB_DEVICE_FIND_BY_VENDOR | + USB_DEVICE_FIND_BY_BUS | + USB_DEVICE_FIND_ONE); if (!(list = usbDeviceSearch(vendor, product, bus, devno, flags))) return NULL;
On 2012年05月04日 12:39, Guannan Ren wrote:
On 05/04/2012 05:38 AM, Eric Blake wrote:
On 05/03/2012 03:21 PM, Eric Blake wrote:
On 05/03/2012 04:51 AM, Guannan Ren wrote:
usbFindDevice():get usb device according to idVendor, idProduct, bus, device it is the exact match of the four parameters
usbFindDeviceByBus():get usb device according to bus, device it returns only one usb device same as usbFindDevice
usbFindDeviceByVendor():get usb device according to idVendor,idProduct it probably returns multiple usb devices.
usbDeviceSearch(): a helper function to do the actual search --- src/libvirt_private.syms | 2 + src/util/hostusb.c | 209 +++++++++++++++++++++++++++++++++------------- src/util/hostusb.h | 22 ++++-- 3 files changed, 170 insertions(+), 63 deletions(-) This patch doesn't even compile!
qemu/qemu_hostdev.c: In function 'qemuPrepareHostdevUSBDevices': qemu/qemu_hostdev.c:599:33: error: too few arguments to function 'usbFindDevice' ../src/util/hostusb.h:40:13: note: declared here
You can't change the signature of usbFindDevice unless you update all callers in the same patch.
I compiled it and tested it , then sent it out. I think the changes on usbFindDevice caller is in PATCH2/3 and PATCH 3/3 "usb = usbFindDevice(vendor, product, bus, device);" are there.
No, every patch shouldn't break the build. You have to update all callers of usbFindDevice together with this patch. Regards, Osier
On 05/04/2012 01:48 PM, Osier Yang wrote:
On 2012年05月04日 12:39, Guannan Ren wrote:
On 05/04/2012 05:38 AM, Eric Blake wrote:
On 05/03/2012 03:21 PM, Eric Blake wrote:
On 05/03/2012 04:51 AM, Guannan Ren wrote:
usbFindDevice():get usb device according to idVendor, idProduct, bus, device it is the exact match of the four parameters
usbFindDeviceByBus():get usb device according to bus, device it returns only one usb device same as usbFindDevice
usbFindDeviceByVendor():get usb device according to idVendor,idProduct it probably returns multiple usb devices.
usbDeviceSearch(): a helper function to do the actual search --- src/libvirt_private.syms | 2 + src/util/hostusb.c | 209 +++++++++++++++++++++++++++++++++------------- src/util/hostusb.h | 22 ++++-- 3 files changed, 170 insertions(+), 63 deletions(-) This patch doesn't even compile!
qemu/qemu_hostdev.c: In function 'qemuPrepareHostdevUSBDevices': qemu/qemu_hostdev.c:599:33: error: too few arguments to function 'usbFindDevice' ../src/util/hostusb.h:40:13: note: declared here
You can't change the signature of usbFindDevice unless you update all callers in the same patch.
I compiled it and tested it , then sent it out. I think the changes on usbFindDevice caller is in PATCH2/3 and PATCH 3/3 "usb = usbFindDevice(vendor, product, bus, device);" are there.
No, every patch shouldn't break the build. You have to update all callers of usbFindDevice together with this patch.
Regards, Osier
Eric, Osier, Thanks, I really don't know about this. The v4 fixed the building break problem, each patch is standalone. Guannan Ren
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, 74 insertions(+), 47 deletions(-) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 8594fb2..8eef6d3 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 these usb devices + * 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,61 @@ 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) + } else if (vendor && !bus) { + usbDeviceList *devs = usbFindDeviceByVendor(vendor, product); + if (!devs) 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); + if (usbDeviceListCount(devs) > 1) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("multiple USB devices for %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); - goto cleanup; - } + } else if (!vendor && bus) { + usb = usbFindDeviceByBus(bus, device); + } + 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 +704,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
One usb device could be allowed to hotplug in at a time. If user gives a xml as follows. Probably there are two usb devices available but with different value of "bus, device" we give an 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 | 64 ++++++++++++++++++++++++++++++++++++---------- 1 files changed, 50 insertions(+), 14 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7cf7b90..c59ae74 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,58 @@ 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); + + } else if (vendor && !bus) { + usbDeviceList *devs = usbFindDeviceByVendor(vendor, product); + if (!devs) + goto cleanup; - usbDevice *usb - = usbFindDevice(hostdev->source.subsys.u.usb.vendor, - hostdev->source.subsys.u.usb.product); + if (usbDeviceListCount(devs) > 1) { + qemuReportError(VIR_ERR_OPERATION_FAILED, + _("multiple USB devices for %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); + + } else if (!vendor && bus) { + usb = usbFindDeviceByBus(bus, device); + } 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 +1204,7 @@ int qemuDomainAttachHostDevice(struct qemud_driver *driver, goto error; } + usbDeviceListFree(list); return 0; error: @@ -1179,6 +1212,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
participants (3)
-
Eric Blake -
Guannan Ren -
Osier Yang