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

[Osier Yang <jyang@xxxxxxxxxx>]

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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list