On 05/01/2012 10:16 AM, Guannan Ren wrote:

usbFindDevice():get usb device according to idVendor, idProduct, bus, device it is the most strict search

usbFindDevByBus():get usb device according to bus, device it returns only one usb device same as usbFindDevice

usbFindDevByVendor():get usb device according to idVendor,idProduct it probably returns multiple usb devices.

usbDeviceSearch(): a helper function to do the actual search --- src/util/hostusb.c | 162 ++++++++++++++++++++++++++++++++++++++------------- src/util/hostusb.h | 20 ++++++- 2 files changed, 138 insertions(+), 44 deletions(-)

diff --git a/src/util/hostusb.c b/src/util/hostusb.c index 92f52a2..73dc959 100644 --- a/src/util/hostusb.c +++ b/src/util/hostusb.c @@ -94,13 +94,22 @@ cleanup: return ret; }

-static int usbFindBusByVendor(unsigned vendor, unsigned product, - unsigned *bus, unsigned *devno) +static usbDeviceList * +usbDeviceSearch(unsigned vendor, + unsigned product, + unsigned bus, + unsigned devno, + unsigned flags) { DIR *dir = NULL; - int ret = -1, found = 0; + int found = 0; char *ignore = NULL; struct dirent *de; + usbDeviceList *list = NULL, *ret = NULL; + usbDevice *usb; + + if (!(list = usbDeviceListNew())) + goto cleanup;

dir = opendir(USB_SYSFS "/devices"); if (!dir) { @@ -111,48 +120,72 @@ static int usbFindBusByVendor(unsigned vendor, unsigned product, }

while ((de = readdir(dir))) { - unsigned found_prod, found_vend; + unsigned found_prod, found_vend, found_bus, found_devno; + char *tmpstr = de->d_name; + if (de->d_name[0] == '.' || strchr(de->d_name, ':')) continue;

if (usbSysReadFile("idVendor", de->d_name, 16, &found_vend) < 0) goto cleanup; + if (usbSysReadFile("idProduct", de->d_name, 16, &found_prod) < 0) goto cleanup;

- if (found_prod == product && found_vend == vendor) { - /* Lookup bus.addr info */ - char *tmpstr = de->d_name; - unsigned found_bus, found_addr; + if (STRPREFIX(de->d_name, "usb")) + tmpstr += 3;

- if (STRPREFIX(de->d_name, "usb")) - tmpstr += 3; + if (virStrToLong_ui(tmpstr, &ignore, 10, &found_bus) < 0) { + usbReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to parse dir name '%s'"), + de->d_name); + goto cleanup; + } + + if (usbSysReadFile("devnum", de->d_name, + 10, &found_devno) < 0) + goto cleanup;

- if (virStrToLong_ui(tmpstr, &ignore, 10, &found_bus) < 0) { - usbReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to parse dir name '%s'"), - de->d_name); - goto cleanup; - } + switch (flags) { + /* + * Don't set found to 1 in order to continue the loop + * to find multiple USB devices with same idVendor and idProduct + */ + case USB_DEVICE_FIND_BY_VENDOR: + if (found_prod != product || found_vend != vendor) + continue; + break;

- if (usbSysReadFile("devnum", de->d_name, - 10, &found_addr) < 0) - goto cleanup; + case USB_DEVICE_FIND_BY_BUS: + if (found_bus != bus || found_devno != devno) + continue; + found = 1; + break;

- *bus = found_bus; - *devno = found_addr; + case USB_DEVICE_FIND_BY_ALL: + if (found_prod != product + || found_vend != vendor + || found_bus != bus + || found_devno != devno) + continue; found = 1; break; } - }

- if (!found) - usbReportError(VIR_ERR_INTERNAL_ERROR, - _("Did not find USB device %x:%x"), vendor, product); - else - ret = 0; + usb = usbGetDevice(found_bus, found_devno); + if (!usb) + goto cleanup; + + if (usbDeviceListAdd(list, usb) < 0) { + usbFreeDevice(usb); + goto cleanup; + } + + if (found) break; + } + ret = list;

cleanup: if (dir) { @@ -160,9 +193,69 @@ cleanup: closedir (dir); errno = saved_errno; } + + if (!ret) + usbDeviceListFree(list); return ret; }

+usbDeviceList * +usbFindDevByVendor(unsigned vendor, unsigned product) +{ + + usbDeviceList *list; + if (!(list = usbDeviceSearch(vendor, product, 0 , 0, USB_DEVICE_FIND_BY_VENDOR))) + return NULL; + + if (list->count == 0) { + usbReportError(VIR_ERR_INTERNAL_ERROR, + _("Did not find USB device %x:%x"), vendor, product); + usbDeviceListFree(list); + return NULL; + } + + return list; +} + +usbDevice * +usbFindDevByBus(unsigned bus, unsigned devno) +{ + usbDeviceList *list; + if (!(list = usbDeviceSearch(0, 0, bus, devno, USB_DEVICE_FIND_BY_BUS))) + return NULL; + + if (list->count == 0) { + usbReportError(VIR_ERR_INTERNAL_ERROR, + _("Did not find USB device bus:%u device:%u"), bus, devno); + usbDeviceListFree(list); + return NULL; + } + + return usbDeviceListGet(list, 0); +} + +usbDevice * +usbFindDevice(unsigned vendor, + unsigned product, + unsigned bus, + unsigned devno) +{ + usbDeviceList *list; + if (!(list = usbDeviceSearch(vendor, product, + bus, devno, USB_DEVICE_FIND_BY_ALL))) + return NULL; + + if (list->count == 0) { + usbReportError(VIR_ERR_INTERNAL_ERROR, + _("Did not find USB device %x:%x bus:%u device:%u"), + vendor, product, bus, devno); + usbDeviceListFree(list); + return NULL; + } + + return usbDeviceListGet(list, 0); +} + usbDevice * usbGetDevice(unsigned bus, unsigned devno) @@ -207,21 +300,6 @@ usbGetDevice(unsigned bus, return dev; }

- -usbDevice * -usbFindDevice(unsigned vendor, - unsigned product) -{ - unsigned bus = 0, devno = 0; - - if (usbFindBusByVendor(vendor, product, &bus, &devno) < 0) { - return NULL; - } - - return usbGetDevice(bus, devno); -} - - void usbFreeDevice(usbDevice *dev) { diff --git a/src/util/hostusb.h b/src/util/hostusb.h index afaa32f..7f18bce 100644 --- a/src/util/hostusb.h +++ b/src/util/hostusb.h @@ -28,10 +28,26 @@ typedef struct _usbDevice usbDevice; typedef struct _usbDeviceList usbDeviceList;

+typedef enum { + USB_DEVICE_FIND_BY_VENDOR = 0, + USB_DEVICE_FIND_BY_BUS = 1 << 0, + USB_DEVICE_FIND_BY_ALL = 1 << 1, +} usbDeviceFindFlags; + usbDevice *usbGetDevice(unsigned bus, unsigned devno); -usbDevice *usbFindDevice(unsigned vendor, - unsigned product); + +usbDevice * usbFindDevByBus(unsigned bus, + unsigned devno); + +usbDeviceList * usbFindDevByVendor(unsigned vendor, + unsigned product); + +usbDevice * usbFindDevice(unsigned vendor, + unsigned product, + unsigned bus, + unsigned devno); + void usbFreeDevice (usbDevice *dev); void usbDeviceSetUsedBy(usbDevice *dev, const char *name); const char *usbDeviceGetUsedBy(usbDevice *dev); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 025816a..290dad7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1085,6 +1085,8 @@ usbDeviceListNew; usbDeviceListSteal; usbDeviceSetUsedBy; usbFindDevice; +usbFindDevByBus; +usbFindDevByVendor; usbFreeDevice; usbGetDevice;

This looks good to me, but I'd prefer one more ACK with mine. I'll go through the rest of the series, but the ACK assurance will be the same as I'm not very familiar with this still. BTW I had problems applying this patch to current head, I had to add a newline in the middle of: @@ -1085,6 +1085,8 @@ usbDeviceListNew; and make it: @@ -1085,6 +1085,8 @@ usbDeviceListNew; Don't know how that happened, though, other patches apply fine :) Martin

On 2012年05月01日 16:16, Guannan Ren wrote:

usbFindDevice():get usb device according to idVendor, idProduct, bus, device it is the most strict search

usbFindDevByBus():get usb device according to bus, device

Should we name it as usbFindDevByAddress? Given that both 'bus' and 'device' are used to find the device actually.

it returns only one usb device same as usbFindDevice

usbFindDevByVendor():get usb device according to idVendor,idProduct it probably returns multiple usb devices.

usbDeviceSearch(): a helper function to do the actual search --- src/util/hostusb.c | 162 ++++++++++++++++++++++++++++++++++++++------------- src/util/hostusb.h | 20 ++++++- 2 files changed, 138 insertions(+), 44 deletions(-)

diff --git a/src/util/hostusb.c b/src/util/hostusb.c index 92f52a2..73dc959 100644 --- a/src/util/hostusb.c +++ b/src/util/hostusb.c @@ -94,13 +94,22 @@ cleanup: return ret; }

-static int usbFindBusByVendor(unsigned vendor, unsigned product, - unsigned *bus, unsigned *devno) +static usbDeviceList * +usbDeviceSearch(unsigned vendor, + unsigned product, + unsigned bus, + unsigned devno, + unsigned flags)

Though no difference between 'unsigned' and 'unsigned int', expect more typing, should we keep consistent across the project? I.e. Don't introduce new 'unsigned', and substitute it with 'unsigned int' as a follow up patch.

{ DIR *dir = NULL; - int ret = -1, found = 0; + int found = 0; char *ignore = NULL; struct dirent *de; + usbDeviceList *list = NULL, *ret = NULL; + usbDevice *usb; + + if (!(list = usbDeviceListNew()))

virReportOOMError();

+ goto cleanup;

dir = opendir(USB_SYSFS "/devices"); if (!dir) { @@ -111,48 +120,72 @@ static int usbFindBusByVendor(unsigned vendor, unsigned product, }

while ((de = readdir(dir))) { - unsigned found_prod, found_vend; + unsigned found_prod, found_vend, found_bus, found_devno; + char *tmpstr = de->d_name; + if (de->d_name[0] == '.' || strchr(de->d_name, ':')) continue;

if (usbSysReadFile("idVendor", de->d_name, 16,&found_vend)< 0) goto cleanup; + if (usbSysReadFile("idProduct", de->d_name, 16,&found_prod)< 0) goto cleanup;

- if (found_prod == product&& found_vend == vendor) { - /* Lookup bus.addr info */ - char *tmpstr = de->d_name; - unsigned found_bus, found_addr; + if (STRPREFIX(de->d_name, "usb")) + tmpstr += 3;

- if (STRPREFIX(de->d_name, "usb")) - tmpstr += 3; + if (virStrToLong_ui(tmpstr,&ignore, 10,&found_bus)< 0) { + usbReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to parse dir name '%s'"), + de->d_name); + goto cleanup; + } + + if (usbSysReadFile("devnum", de->d_name, + 10,&found_devno)< 0) + goto cleanup;

- if (virStrToLong_ui(tmpstr,&ignore, 10,&found_bus)< 0) { - usbReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to parse dir name '%s'"), - de->d_name); - goto cleanup; - } + switch (flags) { + /* + * Don't set found to 1 in order to continue the loop + * to find multiple USB devices with same idVendor and idProduct + */ + case USB_DEVICE_FIND_BY_VENDOR: + if (found_prod != product || found_vend != vendor) + continue; + break;

- if (usbSysReadFile("devnum", de->d_name, - 10,&found_addr)< 0) - goto cleanup; + case USB_DEVICE_FIND_BY_BUS: + if (found_bus != bus || found_devno != devno) + continue; + found = 1; + break;

- *bus = found_bus; - *devno = found_addr; + case USB_DEVICE_FIND_BY_ALL: + if (found_prod != product + || found_vend != vendor + || found_bus != bus + || found_devno != devno)

As a habit: if (found_prod != product || found_vend != vendor || found_bus != bus || found_devno != devno)

+ continue; found = 1;

Given 'found' can only be '0' and '1', why not boolean?

break; } - }

- if (!found) - usbReportError(VIR_ERR_INTERNAL_ERROR, - _("Did not find USB device %x:%x"), vendor, product); - else - ret = 0; + usb = usbGetDevice(found_bus, found_devno); + if (!usb) + goto cleanup; + + if (usbDeviceListAdd(list, usb)< 0) { + usbFreeDevice(usb); + goto cleanup; + } + + if (found) break;

It's desired to be: if (found) break;

+ } + ret = list;

cleanup: if (dir) { @@ -160,9 +193,69 @@ cleanup: closedir (dir); errno = saved_errno; } + + if (!ret) + usbDeviceListFree(list); return ret; }

+usbDeviceList * +usbFindDevByVendor(unsigned vendor, unsigned product) +{ + + usbDeviceList *list; + if (!(list = usbDeviceSearch(vendor, product, 0 , 0, USB_DEVICE_FIND_BY_VENDOR))) + return NULL; + + if (list->count == 0) { + usbReportError(VIR_ERR_INTERNAL_ERROR, + _("Did not find USB device %x:%x"), vendor, product); + usbDeviceListFree(list); + return NULL; + } + + return list; +} + +usbDevice * +usbFindDevByBus(unsigned bus, unsigned devno) +{ + usbDeviceList *list; + if (!(list = usbDeviceSearch(0, 0, bus, devno, USB_DEVICE_FIND_BY_BUS))) + return NULL; + + if (list->count == 0) { + usbReportError(VIR_ERR_INTERNAL_ERROR, + _("Did not find USB device bus:%u device:%u"), bus, devno); + usbDeviceListFree(list); + return NULL; + } + + return usbDeviceListGet(list, 0);

The left items in the list are leaked. (In case of there are multiple devices found)

+} + +usbDevice * +usbFindDevice(unsigned vendor, + unsigned product, + unsigned bus, + unsigned devno) +{ + usbDeviceList *list; + if (!(list = usbDeviceSearch(vendor, product, + bus, devno, USB_DEVICE_FIND_BY_ALL))) + return NULL; + + if (list->count == 0) { + usbReportError(VIR_ERR_INTERNAL_ERROR, + _("Did not find USB device %x:%x bus:%u device:%u"), + vendor, product, bus, devno); + usbDeviceListFree(list); + return NULL; + } + + return usbDeviceListGet(list, 0);

Likewise.

+} + usbDevice * usbGetDevice(unsigned bus, unsigned devno) @@ -207,21 +300,6 @@ usbGetDevice(unsigned bus, return dev; }

- -usbDevice * -usbFindDevice(unsigned vendor, - unsigned product) -{ - unsigned bus = 0, devno = 0; - - if (usbFindBusByVendor(vendor, product,&bus,&devno)< 0) { - return NULL; - } - - return usbGetDevice(bus, devno); -} - - void usbFreeDevice(usbDevice *dev) { diff --git a/src/util/hostusb.h b/src/util/hostusb.h index afaa32f..7f18bce 100644 --- a/src/util/hostusb.h +++ b/src/util/hostusb.h @@ -28,10 +28,26 @@ typedef struct _usbDevice usbDevice; typedef struct _usbDeviceList usbDeviceList;

+typedef enum { + USB_DEVICE_FIND_BY_VENDOR = 0, + USB_DEVICE_FIND_BY_BUS = 1<< 0, + USB_DEVICE_FIND_BY_ALL = 1<< 1, +} usbDeviceFindFlags; + usbDevice *usbGetDevice(unsigned bus, unsigned devno); -usbDevice *usbFindDevice(unsigned vendor, - unsigned product); + +usbDevice * usbFindDevByBus(unsigned bus, + unsigned devno); + +usbDeviceList * usbFindDevByVendor(unsigned vendor, + unsigned product); + +usbDevice * usbFindDevice(unsigned vendor, + unsigned product, + unsigned bus, + unsigned devno); + void usbFreeDevice (usbDevice *dev); void usbDeviceSetUsedBy(usbDevice *dev, const char *name); const char *usbDeviceGetUsedBy(usbDevice *dev); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 025816a..290dad7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1085,6 +1085,8 @@ usbDeviceListNew; usbDeviceListSteal; usbDeviceSetUsedBy; usbFindDevice; +usbFindDevByBus; +usbFindDevByVendor; usbFreeDevice; usbGetDevice;

On 2012年05月01日 16:16, Guannan Ren wrote:

refactor qemuPrepareHostdevUSBDevices function, make it focus on adding usb device to activeUsbHostdevs after check. After that, the usb hotplug function qemuDomainAttachHostDevice also could use it.

expand qemuPrepareHostUSBDevices to perform the usb search, rollback on failure. --- src/qemu/qemu_hostdev.c | 118 ++++++++++++++++++++++++++++++----------------- src/qemu/qemu_hostdev.h | 3 +- 2 files changed, 76 insertions(+), 45 deletions(-)

diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index 8594fb2..a3d4ad4 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -565,13 +565,51 @@ qemuPrepareHostPCIDevices(struct qemud_driver *driver, int qemuPrepareHostdevUSBDevices(struct qemud_driver *driver, const char *name, - virDomainHostdevDefPtr *hostdevs, - int nhostdevs) + usbDeviceList *list) { - int ret = -1; int i; + usbDevice *tmp; + + for (i = 0; i< usbDeviceListCount(list); i++) { + usbDevice *usb = usbDeviceListGet(list, i); + if ((tmp = usbDeviceListFind(driver->activeUsbHostdevs, usb))) { + const char *other_name = usbDeviceGetUsedBy(tmp); + + if (other_name) + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("USB device %s is in use by domain %s"), + usbDeviceGetName(tmp), other_name); + else + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("USB device %s is already in use"), + usbDeviceGetName(tmp)); + return -1; + } + + usbDeviceSetUsedBy(usb, name); + VIR_DEBUG("Adding %03d.%03d dom=%s to activeUsbHostdevs", + usbDeviceGetBus(usb), usbDeviceGetDevno(usb), name); + /* + * The caller is responsible to steal the list of usb + * from the usbDeviceList that passed in on success,

s/steal the list of usb/usb devices which are used by domain/.

+ * perform rollback on failure. + */ + if (usbDeviceListAdd(driver->activeUsbHostdevs, usb)< 0) + return -1; + + } + return 0; +} + +static int +qemuPrepareHostUSBDevices(struct qemud_driver *driver, + virDomainDefPtr def)

It's confused with qemuPrepareHostdevUSBDevices, the idea to have a general function (new qemuPrepareHostdevUSBDevices) for reusing is good. But we should have another name for it instead, and keep "qemuPrepareHostdevUSBDevices" here.

+{ + int i, ret = -1; usbDeviceList *list; usbDevice *tmp; + virDomainHostdevDefPtr *hostdevs = def->hostdevs; + int nhostdevs = def->nhostdevs;

/* To prevent situation where USB device is assigned to two domains * we need to keep a list of currently assigned USB devices. @@ -586,64 +624,65 @@ qemuPrepareHostdevUSBDevices(struct qemud_driver *driver, */ for (i = 0 ; i< nhostdevs ; i++) { virDomainHostdevDefPtr hostdev = hostdevs[i]; + usbDevice *usb = NULL;

if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) continue; if (hostdev->source.subsys.type != VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) continue;

- /* Resolve a vendor/product to bus/device */ - if (hostdev->source.subsys.u.usb.vendor) { - usbDevice *usb - = usbFindDevice(hostdev->source.subsys.u.usb.vendor, - hostdev->source.subsys.u.usb.product); + unsigned vendor = hostdev->source.subsys.u.usb.vendor; + unsigned product = hostdev->source.subsys.u.usb.product; + unsigned bus = hostdev->source.subsys.u.usb.bus; + unsigned device = hostdev->source.subsys.u.usb.device;

+ if (vendor&& bus) { + usb = usbFindDevice(vendor, product, bus, device); if (!usb) goto cleanup;

Given there is a check of 'usb' after the "if...else if..." clauses. This should be removed. see [1]

- if ((tmp = usbDeviceListFind(driver->activeUsbHostdevs, usb))) { - const char *other_name = usbDeviceGetUsedBy(tmp); - - if (other_name) - qemuReportError(VIR_ERR_OPERATION_INVALID, - _("USB device %s is in use by domain %s"), - usbDeviceGetName(tmp), other_name); - else - qemuReportError(VIR_ERR_OPERATION_INVALID, - _("USB device %s is already in use"), - usbDeviceGetName(tmp)); - usbFreeDevice(usb); + } else if (vendor&& !bus) { + usbDeviceList *devs = usbFindDevByVendor(vendor, product); + if (!devs) + goto cleanup;

likewise.

+ + if (usbDeviceListCount(devs)> 1) { + qemuReportError(VIR_ERR_XML_ERROR, + _("multiple USB deivces %x:%x, " + "use<address> to specify one"), vendor, product);

The place to fix the error type, and have a more clear message.

+ usbDeviceListFree(devs); goto cleanup; } + usb = usbDeviceListGet(devs, 0); + usbDeviceListSteal(devs, usb); + usbDeviceListFree(devs);

hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb); hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb);

- if (usbDeviceListAdd(list, usb)< 0) { - usbFreeDevice(usb); + } else if (!vendor&& bus) { + usb = usbFindDevByBus(bus, device); + if (!usb) goto cleanup;

Should be removed.

- } + }

+ if (!usb) + goto cleanup;

[1] The check is here.

+ + if (usbDeviceListAdd(list, usb)< 0) { + usbFreeDevice(usb); + goto cleanup; } }

- /* Loop 2: Mark devices in temporary list as used by @name + /* Mark devices in temporary list as used by @name * and add them do driver list. However, if something goes * wrong, perform rollback. */ - for (i = 0; i< usbDeviceListCount(list); i++) { - tmp = usbDeviceListGet(list, i); - usbDeviceSetUsedBy(tmp, name); + if (qemuPrepareHostdevUSBDevices(driver, def->name, list)< 0) + goto inactivedevs;

- VIR_DEBUG("Adding %03d.%03d dom=%s to activeUsbHostdevs", - usbDeviceGetBus(tmp), usbDeviceGetDevno(tmp), name); - if (usbDeviceListAdd(driver->activeUsbHostdevs, tmp)< 0) { - usbFreeDevice(tmp); - goto inactivedevs; - } - } - - /* Loop 3: Temporary list was successfully merged with + /* Loop 2: Temporary list was successfully merged with * driver list, so steal all items to avoid freeing them * in cleanup label. */ @@ -669,13 +708,6 @@ cleanup: return ret; }

-static int -qemuPrepareHostUSBDevices(struct qemud_driver *driver, - virDomainDefPtr def) -{ - return qemuPrepareHostdevUSBDevices(driver, def->name, def->hostdevs, def->nhostdevs); -} - int qemuPrepareHostDevices(struct qemud_driver *driver, virDomainDefPtr def) { diff --git a/src/qemu/qemu_hostdev.h b/src/qemu/qemu_hostdev.h index 371630a..a8acccf 100644 --- a/src/qemu/qemu_hostdev.h +++ b/src/qemu/qemu_hostdev.h @@ -38,8 +38,7 @@ int qemuPrepareHostdevPCIDevices(struct qemud_driver *driver, int nhostdevs); int qemuPrepareHostdevUSBDevices(struct qemud_driver *driver, const char *name, - virDomainHostdevDefPtr *hostdevs, - int nhostdevs); + usbDeviceList *list); int qemuPrepareHostDevices(struct qemud_driver *driver, virDomainDefPtr def); void qemuReattachPciDevice(pciDevice *dev, struct qemud_driver *driver);

On 05/01/2012 02:16 AM, Guannan Ren wrote:

One usb device could be allowed to hotplug in at a time. If user give a xml as follows. Probably there are two usb devices avaiable

s/avaiable/available/

but with different value of "bus, device"

we give a error to let user use <address> to specify the desired one.

<hostdev mode='subsystem' type='usb' managed='yes'> <source> <vendor id='0x15e1'/> <product id='0x2007'/> </source> </hostdev> --- src/qemu/qemu_hotplug.c | 68 +++++++++++++++++++++++++++++++++++++--------- 1 files changed, 54 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7cf7b90..15693a0 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1121,6 +1121,9 @@ int qemuDomainAttachHostDevice(struct qemud_driver *driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { + usbDeviceList *list; + usbDevice * usb = NULL;

Style: no space after *.

+ if (!(list = usbDeviceListNew())) + goto cleanup;

virReportOOMError()

+ + if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) { + unsigned vendor = hostdev->source.subsys.u.usb.vendor; + unsigned product = hostdev->source.subsys.u.usb.product; + unsigned bus = hostdev->source.subsys.u.usb.bus; + unsigned device = hostdev->source.subsys.u.usb.device; + + if (vendor && bus) {

Same question as in 2/3 about valid values for bus.

+ if (usbDeviceListCount(devs) > 1) { + qemuReportError(VIR_ERR_XML_ERROR, + _("multiple USB deivces %x:%x, "

s/deivces/devices/ -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 2012年05月01日 16:16, Guannan Ren wrote:

One usb device could be allowed to hotplug in at a time. If user give a xml as follows.

s/give/gives/ Probably there are two usb devices avaiable s/avaiable/available/

but with different value of "bus, device"

we give a error to let user use<address> to specify the desired one.

<hostdev mode='subsystem' type='usb' managed='yes'> <source> <vendor id='0x15e1'/> <product id='0x2007'/> </source> </hostdev> --- src/qemu/qemu_hotplug.c | 68 +++++++++++++++++++++++++++++++++++++--------- 1 files changed, 54 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7cf7b90..15693a0 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1121,6 +1121,9 @@ int qemuDomainAttachHostDevice(struct qemud_driver *driver, virDomainObjPtr vm, virDomainHostdevDefPtr hostdev) { + usbDeviceList *list; + usbDevice * usb = NULL;

s/usbDevice * usb/usbDevice *usb/

+ if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("hostdev mode '%s' not supported"), @@ -1128,29 +1131,62 @@ int qemuDomainAttachHostDevice(struct qemud_driver *driver, return -1; }

- /* Resolve USB product/vendor to bus/device */ - if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB&& - hostdev->source.subsys.u.usb.vendor) { - if (qemuPrepareHostdevUSBDevices(driver, vm->def->name,&hostdev, 1)< 0) - goto error; + if (!(list = usbDeviceListNew()))

virReportOOMError()

+ goto cleanup; + + if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) { + unsigned vendor = hostdev->source.subsys.u.usb.vendor; + unsigned product = hostdev->source.subsys.u.usb.product; + unsigned bus = hostdev->source.subsys.u.usb.bus; + unsigned device = hostdev->source.subsys.u.usb.device; + + if (vendor&& bus) { + usb = usbFindDevice(vendor, product, bus, device); + if (!usb) + goto cleanup;

Should be removed. See [1]

+ + } else if (vendor&& !bus) { + usbDeviceList *devs = usbFindDevByVendor(vendor, product); + if (!devs) + goto cleanup; + + if (usbDeviceListCount(devs)> 1) { + qemuReportError(VIR_ERR_XML_ERROR, + _("multiple USB deivces %x:%x, " + "use<address> to specify one"), vendor, product); + usbDeviceListFree(devs); + goto cleanup; + } + usb = usbDeviceListGet(devs, 0); + usbDeviceListSteal(devs, usb); + usbDeviceListFree(devs);

- usbDevice *usb - = usbFindDevice(hostdev->source.subsys.u.usb.vendor, - hostdev->source.subsys.u.usb.product); + hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb); + hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb); + + } else if (!vendor&& bus) { + usb = usbFindDevByBus(bus, device); + if (!usb) + goto cleanup;

Should be removed. see [1]

+ }

if (!usb) - return -1; + goto cleanup;

[1] There is a duplicate check here.

- hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb); - hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb); + if (usbDeviceListAdd(list, usb)< 0) { + usbFreeDevice(usb); + goto cleanup; + }

- usbFreeDevice(usb); - } + if (qemuPrepareHostdevUSBDevices(driver, vm->def->name, list)< 0) + goto cleanup;

Do we really need a list here? there is only found usb device is , I guess the only purpose is to take use of the new helper function qemuPrepareHostdevUSBDevices, I.e. to use the new helper function, it create a new list, and the only one usb device is inserted to and removed from the list. IMO it's a waste.

+ usbDeviceListSteal(list, usb); + }

if (virSecurityManagerSetHostdevLabel(driver->securityManager, vm->def, hostdev)< 0) - return -1; + goto cleanup;

switch (hostdev->source.subsys.type) { case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: @@ -1172,6 +1208,7 @@ int qemuDomainAttachHostDevice(struct qemud_driver *driver, goto error; }

+ usbDeviceListFree(list); return 0;

error: @@ -1179,6 +1216,9 @@ error: vm->def, hostdev)< 0) VIR_WARN("Unable to restore host device labelling on hotplug fail");

+cleanup: + usbDeviceListFree(list); + usbDeviceListSteal(driver->activeUsbHostdevs, usb); return -1; }