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: 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(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 05/05/2012 03:36 AM, Eric Blake wrote: 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++) { ... Thanks for these reviews and comments. Guannan Ren

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