[libvirt] [PATCH 6/9] Add functions for adding usb controllers to addrs

[John Ferlan]

Perhaps needs a bit more meat/description for the commit message...

On 08/12/2015 10:52 AM, Ján Tomko wrote:
> ---
>  src/conf/domain_addr.c   | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/conf/domain_addr.h   |  4 +++
>  src/libvirt_private.syms |  1 +
>  3 files changed, 93 insertions(+)
> 
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index 3962357..024d47b 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -1239,3 +1239,91 @@ void virDomainUSBAddressSetFree(virDomainUSBAddressSetPtr addrs)
>      VIR_FREE(addrs->buses);
>      VIR_FREE(addrs);
>  }
> +
> +
> +static int virDomainUSBAddressSetAddController(virDomainUSBAddressSetPtr addrs,
> +                                               virDomainControllerDefPtr cont)

static int
virDomain*

> +{
> +    virDomainUSBAddressHubPtr hub = NULL;
> +    size_t ports = 0;

Perhaps easier if use 'nports'

> +    int ret = -1;
> +    int model = cont->model;
> +
> +    VIR_DEBUG("addrs=%p controller type=%d model=%d",
> +              addrs, cont->type, cont->model);
> +
> +    if (VIR_ALLOC(hub) < 0)
> +        goto cleanup;

This should be paired this with the ports allocation and have a separate
API to also be used by next patch

> +
> +    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:
> +        ports = 2;
> +        break;
> +
> +    case VIR_DOMAIN_CONTROLLER_MODEL_USB_EHCI:
> +    case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1:
> +        ports = 6;
> +        break;
> +
> +    case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1:
> +    case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2:
> +    case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3:
> +        return 0;
> +        /* FIXME * check the companions? */
> +        ports = 2;
> +        break;

This needs to be fixed/resolved.

> +
> +    case VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI:
> +        ports = 3;
> +        break;
> +
> +    case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI:
> +        ports = 15;
> +
> +    case VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE:
> +    case VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST:
> +        ret = 0;
> +        goto cleanup;
> +    }
> +
> +    if (cont->idx >= addrs->nbuses) {
> +        if (VIR_EXPAND_N(addrs->buses, addrs->nbuses, cont->idx - addrs->nbuses + 1) < 0)
> +            goto cleanup;
> +    }

So if I read this correctly, someone defining a single controller in
their XML with an index of 5 (or 10 or 100 or anything up to 0xfff -
which is supposed to be the bus# max) will be creating that many
AddressSets. There's no requirement that one numbers their controllers
starting at '0' - just that the index is used as the order to be loaded.
Perhaps not what is desired.  Perhaps need some sort of number of
controllers of type 'model' found API rather than a straight 'idx' to
'nbuses' comparison.

Once it's figured out which controller index is being used, that could
be set in 'addrs->ctlr_idx'.  Could also create a companion search API
or two that could take the 'ctlr_idx' and "find" the controller knowing
the eventual USB device type.

Since there's no guarantee of having cont->idx start at 0 and run
through some maximum, should this use a VIR_INSERT_ELEMENT type
algorithm to keep things ordered?  Using ctlr_idx for ordering (if it
even matters).


> +
> +    if (VIR_ALLOC_N(hub->ports, ports + 1) < 0)
> +        goto cleanup;
> +    hub->nports = ports + 1;
> +

Mental note #1 - why are we allocating an extra port?  Answered later on
- using port#0 is not valid (nor is using 0 in dotted notation - eg
1.0.1 would equate to just 1).  Since this isn't obvious - it might be
beneficial to add a note as to why we're adding 1.

Mental note #2 - this is what I'll refer to later as the root usb hub...

The VIR_ALLOC(hub) and this VIR_ALLOC_N should have a separate API to be
shared in next patch as well. It can take a parameter of 'nports'.

What's not fully clear (and something I note in the next patch) is
whether a root hub port can contain a device or if it has to contain
another hub.  I would think physically the answer would be a device
could live on the root hub, but dealing with the physical world is not
something I know all that well..


> +    VIR_DEBUG("Added %zu ports on hub %p", hub->nports - 1, hub);
> +
> +    /* FIXME: is there a bus already? */
> +    addrs->buses[cont->idx] = hub;

Wouldn't this be solved with VIR_INSERT_ELEMENT?  If buses[cont->idx] !=
NULL before this line what does that say - something's wrong with the
algorithm. At the very least you'll leak memory and lose at least 1
device and possibly more.

> +    hub = NULL;
> +
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(hub);
> +    return ret;
> +}
> +
> +
> +int virDomainUSBAddressSetAddControllers(virDomainUSBAddressSetPtr addrs,
> +                                         virDomainDefPtr def)

int
virDomain*

John

> +{
> +    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 dcf86d4..64d35e9 100644
> --- a/src/conf/domain_addr.h
> +++ b/src/conf/domain_addr.h
> @@ -263,6 +263,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 a628d00..7db4839 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -108,6 +108,7 @@ virDomainPCIAddressSlotInUse;
>  virDomainPCIAddressValidate;
>  virDomainUSBAddressGetPortBuf;
>  virDomainUSBAddressGetPortString;
> +virDomainUSBAddressSetAddControllers;
>  virDomainUSBAddressSetCreate;
>  virDomainUSBAddressSetFree;
>  virDomainVirtioSerialAddrAssign;
>