[libvirt] [PATCH v4 0/3] usb devices with same vendorID, productID hotplug support
v4 updates: make each patch not break build make usbDeviceSearch flags filtering code simpler 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/qemu/qemu_hostdev.c | 14 +++- src/qemu/qemu_hotplug.c | 14 +++- src/util/hostusb.c | 202 ++++++++++++++++++++++++++++++++------------- src/util/hostusb.h | 22 ++++-- 5 files changed, 181 insertions(+), 73 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 88f8a21..a4670d3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1088,6 +1088,8 @@ usbDeviceListNew; usbDeviceListSteal; usbDeviceSetUsedBy; usbFindDevice; +usbFindDeviceByBus; +usbFindDeviceByVendor; usbFreeDevice; usbGetDevice; diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 8594fb2..b98fd8f 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -594,13 +594,19 @@ qemuPrepareHostdevUSBDevices(struct qemud_driver *driver, /* 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); + usbDevice *usb; + usbDeviceList *devs; - if (!usb) + devs = usbFindDeviceByVendor(hostdev->source.subsys.u.usb.vendor, + hostdev->source.subsys.u.usb.product); + + if (!devs) goto cleanup; + usb = usbDeviceListGet(devs, 0); + usbDeviceListSteal(devs, usb); + usbDeviceListFree(devs); + if ((tmp = usbDeviceListFind(driver->activeUsbHostdevs, usb))) { const char *other_name = usbDeviceGetUsedBy(tmp); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7cf7b90..0f5fed1 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1131,16 +1131,22 @@ int qemuDomainAttachHostDevice(struct qemud_driver *driver, /* Resolve USB product/vendor to bus/device */ if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB && hostdev->source.subsys.u.usb.vendor) { + usbDevice *usb; + usbDeviceList *list; + if (qemuPrepareHostdevUSBDevices(driver, vm->def->name, &hostdev, 1) < 0) goto error; - usbDevice *usb - = usbFindDevice(hostdev->source.subsys.u.usb.vendor, - hostdev->source.subsys.u.usb.product); + list = usbFindDeviceByVendor(hostdev->source.subsys.u.usb.vendor, + hostdev->source.subsys.u.usb.product); - if (!usb) + if (!list) return -1; + usb = usbDeviceListGet(list, 0); + usbDeviceListSteal(list, usb); + usbDeviceListFree(list); + hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb); hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb); diff --git a/src/util/hostusb.c b/src/util/hostusb.c index 92f52a2..63b10ef 100644 --- a/src/util/hostusb.c +++ b/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 @@ -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 << 0, + USB_DEVICE_FIND_BY_BUS = 1 << 1, +} 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,61 +126,145 @@ 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; + + if ((flags & USB_DEVICE_FIND_BY_VENDOR) && + (found_prod != product || found_vend != vendor)) + continue; - if (STRPREFIX(de->d_name, "usb")) - tmpstr += 3; + if (flags & USB_DEVICE_FIND_BY_BUS) { + if (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) { int saved_errno = errno; - closedir (dir); + 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 +306,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 +331,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..27e07dc 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/04/2012 02:25 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/qemu/qemu_hostdev.c | 14 +++- src/qemu/qemu_hotplug.c | 14 +++- src/util/hostusb.c | 202 ++++++++++++++++++++++++++++++++------------- src/util/hostusb.h | 22 ++++-- 5 files changed, 181 insertions(+), 73 deletions(-)
ACK. Looks better now. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/05/2012 03:16 AM, Eric Blake wrote:
On 05/04/2012 02:25 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/qemu/qemu_hostdev.c | 14 +++- src/qemu/qemu_hotplug.c | 14 +++- src/util/hostusb.c | 202 ++++++++++++++++++++++++++++++++------------- src/util/hostusb.h | 22 ++++-- 5 files changed, 181 insertions(+), 73 deletions(-)
ACK. Looks better now.
Thanks and pushed. 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 | 117 +++++++++++++++++++++++++++------------------- src/qemu/qemu_hostdev.h | 3 +- 2 files changed, 70 insertions(+), 50 deletions(-) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index b98fd8f..0cb89a6 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -565,13 +565,50 @@ 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,70 +623,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; - usbDeviceList *devs; + 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; - devs = usbFindDeviceByVendor(hostdev->source.subsys.u.usb.vendor, - hostdev->source.subsys.u.usb.product); + if (vendor && bus) { + usb = usbFindDevice(vendor, product, bus, device); + } else if (vendor && !bus) { + usbDeviceList *devs = usbFindDeviceByVendor(vendor, product); if (!devs) goto cleanup; + 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); - 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); - 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; - } + } 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); - - VIR_DEBUG("Adding %03d.%03d dom=%s to activeUsbHostdevs", - usbDeviceGetBus(tmp), usbDeviceGetDevno(tmp), name); - if (usbDeviceListAdd(driver->activeUsbHostdevs, tmp) < 0) { - usbFreeDevice(tmp); - goto inactivedevs; - } - } + if (qemuPrepareHostdevUSBDevices(driver, def->name, list) < 0) + 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. */ @@ -675,13 +703,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
On 05/04/2012 02:25 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 | 117 +++++++++++++++++++++++++++------------------- src/qemu/qemu_hostdev.h | 3 +- 2 files changed, 70 insertions(+), 50 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index b98fd8f..0cb89a6 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -565,13 +565,50 @@ 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++) {
Thankfully, usbDeviceListCount() is O(1); if it were O(n) then you would have a quadratic loop. Still, it is slightly faster to factor the function call out of the for loop. On the other hand, right now we aren't using gcc's attribute((pure/const)) because the warnings in gcc 4.6 were buggy; if we were to use that, then usbDeviceListCount() would already be marked and there would be nothing to factor out. So, this is fine as is, and we can save the optimization for a future day when we rely on gcc 4.7 to help us mark the attributes where needed. Unfortunately, this failed to compile on its own: qemu/qemu_hotplug.c: In function 'qemuDomainAttachHostDevice': qemu/qemu_hotplug.c:1137:9: error: passing argument 3 of 'qemuPrepareHostdevUSBDevices' from incompatible pointer type [-Werror] qemu/qemu_hostdev.h:39:5: note: expected 'struct usbDeviceList *' but argument is of type 'struct virDomainHostdevDef **' qemu/qemu_hotplug.c:1137:9: error: too many arguments to function 'qemuPrepareHostdevUSBDevices' qemu/qemu_hostdev.h:39:5: note: declared here But since applying patch 3/3 fixes the failure: ACK if you squash 2/3 and 3/3 into a single patch, so that the total series is only 2 patches instead of 3. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
src/qemu/qemu_hostdev.c: 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_hotplug.c: If there are multiple usb devices available with same vendorID and productID, but with different value of "bus, device", we give an error to let user use <address> to specify the desired one. --- src/qemu/qemu_hostdev.c | 120 ++++++++++++++++++++++++++++------------------- src/qemu/qemu_hostdev.h | 3 +- src/qemu/qemu_hotplug.c | 69 +++++++++++++++++++-------- 3 files changed, 122 insertions(+), 70 deletions(-) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index b98fd8f..b649ae0 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -565,13 +565,53 @@ 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; + unsigned int count; + usbDevice *tmp; + + count = usbDeviceListCount(list); + + for (i = 0; i < count; 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,70 +626,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; - usbDeviceList *devs; + 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; - devs = usbFindDeviceByVendor(hostdev->source.subsys.u.usb.vendor, - hostdev->source.subsys.u.usb.product); + if (vendor && bus) { + usb = usbFindDevice(vendor, product, bus, device); + } else if (vendor && !bus) { + usbDeviceList *devs = usbFindDeviceByVendor(vendor, product); if (!devs) goto cleanup; + 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); - 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); - 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; - } + } 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); - - VIR_DEBUG("Adding %03d.%03d dom=%s to activeUsbHostdevs", - usbDeviceGetBus(tmp), usbDeviceGetDevno(tmp), name); - if (usbDeviceListAdd(driver->activeUsbHostdevs, tmp) < 0) { - usbFreeDevice(tmp); - goto inactivedevs; - } - } + if (qemuPrepareHostdevUSBDevices(driver, def->name, list) < 0) + 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. */ @@ -675,13 +706,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); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 0f5fed1..ad31eba 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1116,11 +1116,13 @@ error: return -1; } - 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,35 +1130,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) { - usbDevice *usb; - usbDeviceList *list; + if (!(list = usbDeviceListNew())) + goto cleanup; - if (qemuPrepareHostdevUSBDevices(driver, vm->def->name, &hostdev, 1) < 0) - goto error; + 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; - list = usbFindDeviceByVendor(hostdev->source.subsys.u.usb.vendor, - hostdev->source.subsys.u.usb.product); + if (vendor && bus) { + usb = usbFindDevice(vendor, product, bus, device); - if (!list) - return -1; + } else if (vendor && !bus) { + usbDeviceList *devs = usbFindDeviceByVendor(vendor, product); + if (!devs) + goto cleanup; - usb = usbDeviceListGet(list, 0); - usbDeviceListSteal(list, usb); - usbDeviceListFree(list); + 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); + hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb); + hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb); - usbFreeDevice(usb); - } + } else if (!vendor && bus) { + usb = usbFindDeviceByBus(bus, device); + } + if (!usb) + goto cleanup; + + if (usbDeviceListAdd(list, usb) < 0) { + usbFreeDevice(usb); + goto cleanup; + } + + 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: @@ -1178,6 +1203,7 @@ int qemuDomainAttachHostDevice(struct qemud_driver *driver, goto error; } + usbDeviceListFree(list); return 0; error: @@ -1185,6 +1211,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
On 05/06/2012 08:55 AM, Guannan Ren wrote:
src/qemu/qemu_hostdev.c: 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_hotplug.c: If there are multiple usb devices available with same vendorID and productID, but with different value of "bus, device", we give an error to let user use <address> to specify the desired one. --- src/qemu/qemu_hostdev.c | 120 ++++++++++++++++++++++++++++------------------- src/qemu/qemu_hostdev.h | 3 +- src/qemu/qemu_hotplug.c | 69 +++++++++++++++++++-------- 3 files changed, 122 insertions(+), 70 deletions(-)
ACK; this fixes my concerns. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/07/2012 11:32 PM, Eric Blake wrote:
On 05/06/2012 08:55 AM, Guannan Ren wrote:
src/qemu/qemu_hostdev.c: 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_hotplug.c: If there are multiple usb devices available with same vendorID and productID, but with different value of "bus, device", we give an error to let user use<address> to specify the desired one. --- src/qemu/qemu_hostdev.c | 120 ++++++++++++++++++++++++++++------------------- src/qemu/qemu_hostdev.h | 3 +- src/qemu/qemu_hotplug.c | 69 +++++++++++++++++++-------- 3 files changed, 122 insertions(+), 70 deletions(-) ACK; this fixes my concerns.
Thanks for the review. Pushed. Guannan Ren
On 05/05/2012 03:36 AM, Eric Blake wrote:
On 05/04/2012 02:25 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 | 117 +++++++++++++++++++++++++++------------------- src/qemu/qemu_hostdev.h | 3 +- 2 files changed, 70 insertions(+), 50 deletions(-) diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index b98fd8f..0cb89a6 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -565,13 +565,50 @@ 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++) { Thankfully, usbDeviceListCount() is O(1); if it were O(n) then you would have a quadratic loop. Still, it is slightly faster to factor the function call out of the for loop. On the other hand, right now we aren't using gcc's attribute((pure/const)) because the warnings in gcc 4.6 were buggy; if we were to use that, then usbDeviceListCount() would already be marked and there would be nothing to factor out. So, this is fine as is, and we can save the optimization for a future day when we rely on gcc 4.7 to help us mark the attributes where needed.
I combine the two patches 2/3, 3/3, and made a small modification on invoking usbDeviceListCount() on V5. ... + unsigned int count; + usbDevice *tmp; + + count = usbDeviceListCount(list); + + for (i = 0; i< count; i++) { ...
Unfortunately, this failed to compile on its own:
qemu/qemu_hotplug.c: In function 'qemuDomainAttachHostDevice': qemu/qemu_hotplug.c:1137:9: error: passing argument 3 of 'qemuPrepareHostdevUSBDevices' from incompatible pointer type [-Werror] qemu/qemu_hostdev.h:39:5: note: expected 'struct usbDeviceList *' but argument is of type 'struct virDomainHostdevDef **' qemu/qemu_hotplug.c:1137:9: error: too many arguments to function 'qemuPrepareHostdevUSBDevices' qemu/qemu_hostdev.h:39:5: note: declared here
But since applying patch 3/3 fixes the failure:
ACK if you squash 2/3 and 3/3 into a single patch, so that the total series is only 2 patches instead of 3.
Thanks for these reviews and comments. Guannan Ren
On 04.05.2012 10:25, 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 | 117 +++++++++++++++++++++++++++------------------- src/qemu/qemu_hostdev.h | 3 +- 2 files changed, 70 insertions(+), 50 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index b98fd8f..0cb89a6 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -565,13 +565,50 @@ 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.
Shame the context didn't catch the rest of this comment, but as written there, we *can't* join these 3 steps into 1 big step which is what you are doing.
@@ -586,70 +623,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; - usbDeviceList *devs; + 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;
- devs = usbFindDeviceByVendor(hostdev->source.subsys.u.usb.vendor, - hostdev->source.subsys.u.usb.product); + if (vendor && bus) { + usb = usbFindDevice(vendor, product, bus, device);
+ } else if (vendor && !bus) { + usbDeviceList *devs = usbFindDeviceByVendor(vendor, product); if (!devs) goto cleanup;
+ 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);
- 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); - 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; - } + } 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); - - VIR_DEBUG("Adding %03d.%03d dom=%s to activeUsbHostdevs", - usbDeviceGetBus(tmp), usbDeviceGetDevno(tmp), name); - if (usbDeviceListAdd(driver->activeUsbHostdevs, tmp) < 0) { - usbFreeDevice(tmp); - goto inactivedevs; - } - } + if (qemuPrepareHostdevUSBDevices(driver, def->name, list) < 0) + goto inactivedevs;
This is wrong and causing a regression. Because if a domain has an USB device which is used by another domain, this will return -1 ... (again missing context) ... and under inactivedevs label the USB device is removed from the activeUsbHostdevs list (the list of currently used USB devices). Which is wrong. My approach was: 1. loop - build the list of USB devices to be used by domain A, while building check if any device is in activeUsbHostdevs list. 2. set devices in temporary list as UsedBy(A) and add them to activeUsbHostdevs. If anything fails, goto inactivedevs. 3. free temp list. return 0; inactivedevs: remove temporary list items from activeUsbHostdevs. return -1; The advantage is - if we find out a device is used (step 1) we won't mess up activeUsbHostdevs list; However, if we join 1+2 into one step, we can't perform rollback. Well, with this impl at least; We need to differentiate if qemuPrepareHostdevUSBDevices failed because a device is already taken (= fail in step 1); or adding to activeUsbHostdevs failed (= fail in step 2). Sorry for not catching this earlier. Michal
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 | 69 +++++++++++++++++++++++++++++++++------------- 1 files changed, 49 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 0f5fed1..ad31eba 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1116,11 +1116,13 @@ error: return -1; } - 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,35 +1130,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) { - usbDevice *usb; - usbDeviceList *list; + if (!(list = usbDeviceListNew())) + goto cleanup; - if (qemuPrepareHostdevUSBDevices(driver, vm->def->name, &hostdev, 1) < 0) - goto error; + 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; - list = usbFindDeviceByVendor(hostdev->source.subsys.u.usb.vendor, - hostdev->source.subsys.u.usb.product); + if (vendor && bus) { + usb = usbFindDevice(vendor, product, bus, device); - if (!list) - return -1; + } else if (vendor && !bus) { + usbDeviceList *devs = usbFindDeviceByVendor(vendor, product); + if (!devs) + goto cleanup; - usb = usbDeviceListGet(list, 0); - usbDeviceListSteal(list, usb); - usbDeviceListFree(list); + 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); + hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb); + hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb); - usbFreeDevice(usb); - } + } else if (!vendor && bus) { + usb = usbFindDeviceByBus(bus, device); + } + if (!usb) + goto cleanup; + + if (usbDeviceListAdd(list, usb) < 0) { + usbFreeDevice(usb); + goto cleanup; + } + + 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: @@ -1178,6 +1203,7 @@ int qemuDomainAttachHostDevice(struct qemud_driver *driver, goto error; } + usbDeviceListFree(list); return 0; error: @@ -1185,6 +1211,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 -
Michal Privoznik