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

[Martin Kletzander]

On 05/01/2012 10:16 AM, 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,
> +         * 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.
> @@ -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;
>  
> -            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;
> +
> +            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);
>  
>              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;
> -            }
> +        }
>  
> +        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);
> +    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);

This looks good also, ACK.

Martin