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

[Osier Yang]

On 2012年05月03日 02:06, Osier Yang wrote:
> 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.

I.e. Given the name of the new helper function is confused with
"qemuPrepareHostUSBDevices", and it's actually no need to use it
here, IMO the new helper function is no need.

>
>>
>> + 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list