[libvirt] [PATCHv4 4/9] Add functions for adding USB controllers to addrs

[John Ferlan]

On 07/01/2016 11:38 AM, Ján Tomko wrote:
> Walk through all the usb controllers in the domain definition
> and create the corresponding structures in the virDomainUSBAddressSet.
> ---
>  src/conf/domain_addr.c   | 121 +++++++++++++++++++++++++++++++++++++++++++++++
>  src/conf/domain_addr.h   |   4 ++
>  src/libvirt_private.syms |   1 +
>  3 files changed, 126 insertions(+)
> 
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index 5e84751..82fe295 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -1330,3 +1330,124 @@ virDomainUSBAddressSetFree(virDomainUSBAddressSetPtr addrs)
>      VIR_FREE(addrs->buses);
>      VIR_FREE(addrs);
>  }
> +
> +
> +static size_t
> +virDomainUSBAddressControllerModelToPorts(virDomainControllerDefPtr cont)
> +{
> +    int model = cont->model;
> +
> +    if (model == -1)
> +        model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
> +
> +    switch ((virDomainControllerModelUSB) model) {
> +    case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI:
> +    case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX4_UHCI:
> +    case VIR_DOMAIN_CONTROLLER_MODEL_USB_VT82C686B_UHCI:
> +        return 2;
> +
> +    case VIR_DOMAIN_CONTROLLER_MODEL_USB_EHCI:
> +    case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1:
> +        return 6;
> +
> +    case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1:
> +    case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2:
> +    case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3:
> +        /* These have two ports each and are used to provide USB1.1
> +         * ports while ICH9_EHCI1 provides 6 USB2.0 ports.
> +         * Ignore these since we will add the EHCI1 too. */
> +        return 0;
> +
> +    case VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI:
> +        return 3;
> +
> +    case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI:
> +        if (cont->opts.usbopts.ports != -1)
> +            return cont->opts.usbopts.ports;
> +        return 4;
> +
> +    case VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE:
> +    case VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST:
> +        /* yoda */

Prior review by Erik asked for this to be removed...

> +        break;
> +    }
> +    return 0;
> +}
> +
> +
> +static virDomainUSBAddressHubPtr
> +virDomainUSBAddressHubNew(size_t nports)
> +{
> +    virDomainUSBAddressHubPtr hub = NULL, ret = NULL;
> +
> +    if (VIR_ALLOC(hub) < 0)
> +        goto cleanup;
> +
> +    if (!(hub->ports = virBitmapNew(nports)))
> +        goto cleanup;
> +
> +    if (VIR_ALLOC_N(hub->hubs, nports) < 0)
> +        goto cleanup;
> +    hub->nports = nports;
> +
> +    ret = hub;
> +    hub = NULL;
> + cleanup:
> +    virDomainUSBAddressHubFree(hub);
> +    return ret;
> +}
> +
> +
> +static int
> +virDomainUSBAddressSetAddController(virDomainUSBAddressSetPtr addrs,
> +                                    virDomainControllerDefPtr cont)
> +{
> +    size_t nports = virDomainUSBAddressControllerModelToPorts(cont);
> +    virDomainUSBAddressHubPtr hub = NULL;
> +    int ret = -1;
> +
> +    VIR_DEBUG("Adding a USB controller model=%s with %zu ports",
> +              virDomainControllerModelUSBTypeToString(cont->model),
> +              nports);
> +
> +    /* Skip UHCI{1,2,3} companions; only add the EHCI1 */
> +    if (nports == 0)
> +        return 0;
> +
> +    if (addrs->nbuses <= cont->idx) {
> +        if (VIR_EXPAND_N(addrs->buses, addrs->nbuses, cont->idx - addrs->nbuses + 1) < 0)
> +            goto cleanup;


So if "someone" decides to be cute and have 1 USB controller, but number
it like 1000, then you're allocating and wasting a lot of memory. I
understand it makes the access a lot faster, but is that what we want?
Someone would have to be "malicious" and equally powerful as yoda to
adjust their XML thusly - of course in the alternate reality 1000000
would take a bigger hit.

Unless someone else has severe heartburn over this (and it wasn't
mentioned by Eric for his ACK), I'm OK with it...


> +    } else if (addrs->buses[cont->idx]) {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("Duplicate USB controllers with index %u"),
> +                       cont->idx);
> +        goto cleanup;
> +    }
> +
> +    if (!(hub = virDomainUSBAddressHubNew(nports)))
> +        goto cleanup;
> +
> +    addrs->buses[cont->idx] = hub;
> +    hub = NULL;
> +
> +    ret = 0;
> + cleanup:
> +    virDomainUSBAddressHubFree(hub);
> +    return ret;
> +}
> +
> +
> +int virDomainUSBAddressSetAddControllers(virDomainUSBAddressSetPtr addrs,
> +                                         virDomainDefPtr def)

This one almost escaped attention (multiline the function)

ACK w/ at least this adjustment

John

I also didn't go through any practice uses under the environment like
Erik seemed to do in his review of the previous version.

> +{
> +    size_t i;
> +
> +    for (i = 0; i < def->ncontrollers; i++) {
> +        virDomainControllerDefPtr cont = def->controllers[i];
> +        if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) {
> +            if (virDomainUSBAddressSetAddController(addrs, cont) < 0)
> +                return -1;
> +        }
> +    }
> +    return 0;
> +}
> diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
> index c62ff6a..4cb0212 100644
> --- a/src/conf/domain_addr.h
> +++ b/src/conf/domain_addr.h
> @@ -269,6 +269,10 @@ typedef struct _virDomainUSBAddressSet virDomainUSBAddressSet;
>  typedef virDomainUSBAddressSet *virDomainUSBAddressSetPtr;
>  
>  virDomainUSBAddressSetPtr virDomainUSBAddressSetCreate(void);
> +
> +int virDomainUSBAddressSetAddControllers(virDomainUSBAddressSetPtr addrs,
> +                                         virDomainDefPtr def)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>  void virDomainUSBAddressSetFree(virDomainUSBAddressSetPtr addrs);
>  
>  #endif /* __DOMAIN_ADDR_H__ */
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 49f8d6c..f0fed8e 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -110,6 +110,7 @@ virDomainPCIControllerModelToConnectType;
>  virDomainUSBAddressPortFormat;
>  virDomainUSBAddressPortFormatBuf;
>  virDomainUSBAddressPortIsValid;
> +virDomainUSBAddressSetAddControllers;
>  virDomainUSBAddressSetCreate;
>  virDomainUSBAddressSetFree;
>  virDomainVirtioSerialAddrAssign;
>