Re: [libvirt] [PATCH v2 3/3] qemu: search usb device accurately to improve usb device hotplug

On 2012年05月01日 16:16, Guannan Ren wrote: > One usb device could be allowed to hotplug in at a time. If user > give a xml as follows. s/give/gives/ Probably there are two usb devices avaiable s/avaiable/available/ > but with different value of "bus, device" > > we give a error to let user use<address> to specify the desired one. > > <hostdev mode='subsystem' type='usb' managed='yes'> > <source> > <vendor id='0x15e1'/> > <product id='0x2007'/> > </source> > </hostdev> > --- > src/qemu/qemu_hotplug.c | 68 > +++++++++++++++++++++++++++++++++++++--------- > 1 files changed, 54 insertions(+), 14 deletions(-) > > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index 7cf7b90..15693a0 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -1121,6 +1121,9 @@ int qemuDomainAttachHostDevice(struct > qemud_driver *driver, > virDomainObjPtr vm, > virDomainHostdevDefPtr hostdev) > { > + usbDeviceList *list; > + usbDevice * usb = NULL; s/usbDevice * usb/usbDevice *usb/ > + > if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS) { > qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, > _("hostdev mode '%s' not supported"), > @@ -1128,29 +1131,62 @@ int qemuDomainAttachHostDevice(struct > qemud_driver *driver, > return -1; > } > > - /* Resolve USB product/vendor to bus/device */ > - if (hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB&& > - hostdev->source.subsys.u.usb.vendor) { > - if (qemuPrepareHostdevUSBDevices(driver, vm->def->name,&hostdev, 1)< 0) > - goto error; > + if (!(list = usbDeviceListNew())) virReportOOMError() > + goto cleanup; > + > + if (hostdev->source.subsys.type == > VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) { > + unsigned vendor = hostdev->source.subsys.u.usb.vendor; > + unsigned product = hostdev->source.subsys.u.usb.product; > + unsigned bus = hostdev->source.subsys.u.usb.bus; > + unsigned device = hostdev->source.subsys.u.usb.device; > + > + if (vendor&& bus) { > + usb = usbFindDevice(vendor, product, bus, device); > + if (!usb) > + goto cleanup; Should be removed. See [1] > + > + } else if (vendor&& !bus) { > + usbDeviceList *devs = usbFindDevByVendor(vendor, product); > + if (!devs) > + goto cleanup; > + > + if (usbDeviceListCount(devs)> 1) { > + qemuReportError(VIR_ERR_XML_ERROR, > + _("multiple USB deivces %x:%x, " > + "use<address> to specify one"), vendor, product); > + usbDeviceListFree(devs); > + goto cleanup; > + } > + usb = usbDeviceListGet(devs, 0); > + usbDeviceListSteal(devs, usb); > + usbDeviceListFree(devs); > > - usbDevice *usb > - = usbFindDevice(hostdev->source.subsys.u.usb.vendor, > - hostdev->source.subsys.u.usb.product); > + hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb); > + hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb); > + > + } else if (!vendor&& bus) { > + usb = usbFindDevByBus(bus, device); > + if (!usb) > + goto cleanup; Should be removed. see [1] > + } > > if (!usb) > - return -1; > + goto cleanup; [1] There is a duplicate check here. > > - hostdev->source.subsys.u.usb.bus = usbDeviceGetBus(usb); > - hostdev->source.subsys.u.usb.device = usbDeviceGetDevno(usb); > + if (usbDeviceListAdd(list, usb)< 0) { > + usbFreeDevice(usb); > + goto cleanup; > + } > > - usbFreeDevice(usb); > - } > + if (qemuPrepareHostdevUSBDevices(driver, vm->def->name, list)< 0) > + goto cleanup; Do we really need a list here? there is only found usb device is , I guess the only purpose is to take use of the new helper function qemuPrepareHostdevUSBDevices, I.e. to use the new helper function, it create a new list, and the only one usb device is inserted to and removed from the list. IMO it's a waste.

> > + usbDeviceListSteal(list, usb); > + } > > if (virSecurityManagerSetHostdevLabel(driver->securityManager, > vm->def, hostdev)< 0) > - return -1; > + goto cleanup; > > switch (hostdev->source.subsys.type) { > case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI: > @@ -1172,6 +1208,7 @@ int qemuDomainAttachHostDevice(struct > qemud_driver *driver, > goto error; > } > > + usbDeviceListFree(list); > return 0; > > error: > @@ -1179,6 +1216,9 @@ error: > vm->def, hostdev)< 0) > VIR_WARN("Unable to restore host device labelling on hotplug fail"); > > +cleanup: > + usbDeviceListFree(list); > + usbDeviceListSteal(driver->activeUsbHostdevs, usb); > return -1; > } > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list