[libvirt] [PATCH 2/3] qemu: make use of usb search function to initialize usb devices

[Michal Privoznik]

On 04.05.2012 10:25, 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 |  117 +++++++++++++++++++++++++++-------------------
>  src/qemu/qemu_hostdev.h |    3 +-
>  2 files changed, 70 insertions(+), 50 deletions(-)
> 
> diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
> index b98fd8f..0cb89a6 100644
> --- a/src/qemu/qemu_hostdev.c
> +++ b/src/qemu/qemu_hostdev.c
> @@ -565,13 +565,50 @@ 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 these usb devices
> +         * from the usbDeviceList that passed in on success,
> +         * perform rollback on failure.
> +         */
> +        if (usbDeviceListAdd(driver->activeUsbHostdevs, usb) < 0)
> +            return -1;
> +    }
> +    return 0;
> +}
> +
> +static int
> +qemuPrepareHostUSBDevices(struct qemud_driver *driver,
> +                          virDomainDefPtr def)
> +{
> +    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.

Shame the context didn't catch the rest of this comment, but as written
there, we *can't* join these 3 steps into 1 big step which is what you
are doing.
> @@ -586,70 +623,61 @@ 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;
> -            usbDeviceList *devs;
> +        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;
>  
> -            devs = usbFindDeviceByVendor(hostdev->source.subsys.u.usb.vendor,
> -                                         hostdev->source.subsys.u.usb.product);
> +        if (vendor && bus) {
> +            usb = usbFindDevice(vendor, product, bus, device);
>  
> +        } else if (vendor && !bus) {
> +            usbDeviceList *devs = usbFindDeviceByVendor(vendor, product);
>              if (!devs)
>                  goto cleanup;
>  
> +            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);
>  
> -            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);
> -                goto cleanup;
> -            }
> -
>              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;
> -            }
> +        } else if (!vendor && bus) {
> +            usb = usbFindDeviceByBus(bus, device);
> +        }
> +
> +        if (!usb)
> +            goto cleanup;
>  
> +        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);
> -
> -        VIR_DEBUG("Adding %03d.%03d dom=%s to activeUsbHostdevs",
> -                  usbDeviceGetBus(tmp), usbDeviceGetDevno(tmp), name);
> -        if (usbDeviceListAdd(driver->activeUsbHostdevs, tmp) < 0) {
> -            usbFreeDevice(tmp);
> -            goto inactivedevs;
> -        }
> -    }
> +    if (qemuPrepareHostdevUSBDevices(driver, def->name, list) < 0)
> +        goto inactivedevs;

This is wrong and causing a regression. Because if a domain has an USB
device which is used by another domain, this will return -1 ...
(again missing context)
... and under inactivedevs label the USB device is removed from the
activeUsbHostdevs list (the list of currently used USB devices).
Which is wrong.

My approach was:
1. loop - build the list of USB devices to be used by domain A,
     while building check if any device is in activeUsbHostdevs list.

2. set devices in temporary list as UsedBy(A) and add them to
activeUsbHostdevs. If anything fails, goto inactivedevs.

3. free temp list.

   return 0;
inactivedevs:
   remove temporary list items from activeUsbHostdevs.
   return -1;

The advantage is - if we find out a device is used (step 1) we won't
mess up activeUsbHostdevs list; However, if we join 1+2 into one step,
we can't perform rollback. Well, with this impl at least; We need to
differentiate if qemuPrepareHostdevUSBDevices failed because a device is
already taken (= fail in step 1); or adding to activeUsbHostdevs failed
(= fail in step 2).

Sorry for not catching this earlier.

Michal