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

Osier Yang jyang at redhat.com
Wed May 2 17:45:51 UTC 2012


On 2012年05月01日 16:16, 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,

s/steal the list of usb/usb devices which are used by domain/.

> +         * perform rollback on failure.
> +         */
> +        if (usbDeviceListAdd(driver->activeUsbHostdevs, usb)<  0)
> +            return -1;
> +
> +    }
> +    return 0;
> +}
> +
> +static int
> +qemuPrepareHostUSBDevices(struct qemud_driver *driver,
> +                          virDomainDefPtr def)

It's confused with qemuPrepareHostdevUSBDevices, the idea to
have a general function (new qemuPrepareHostdevUSBDevices) for
reusing is good. But we should have another name for it instead,
and keep "qemuPrepareHostdevUSBDevices" here.

> +{
> +    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;

Given there is a check of 'usb' after the "if...else if..." clauses.
This should be removed. see [1]

>
> -            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;

likewise.

> +
> +            if (usbDeviceListCount(devs)>  1) {
> +                qemuReportError(VIR_ERR_XML_ERROR,
> +                                _("multiple USB deivces %x:%x, "
> +                                  "use<address>  to specify one"), vendor, product);

The place to fix the error type, and have a more clear message.

> +                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;

Should be removed.

> -            }
> +        }
>
> +        if (!usb)
> +            goto cleanup;

[1] The check is here.

> +
> +        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);




More information about the libvir-list mailing list