[libvirt] [PATCH 5/9] Introduce virDomainUSBAddressSet

[John Ferlan]

Picking up where I left off (more or less) before KVM Forum...  I think
I need a virtual whiteboard... I tried reviewing patches 5 -> 7 together
- going back and forth. Hopefully I haven't left (m)any disjoint
thoughts... I have left some thoughts on the way through - some just to
make sure I'm following what's being done...  others because I found
something not quite right.

On 08/12/2015 10:52 AM, Ján Tomko wrote:
> A new type to track USB addresses.
> 
> The buses in virDomainUSBAddressSet correspond to USB controllers.
> They are represented by the virDomainUSBAddressHub type, having nports
> USB ports. These can contain nested hubs (nports != 0), or they can be
> occupied by other USB devices (with nports = 0).

Recalling earlier reviews - nested ports are the "n.n", "n.n.n", and
"n.n.n.n" dotted octet port numbers... What's still not clear in my mind
is whether it's possible to have "port='1'" and "port='1.1'" in
hardware. Based on the algorithm - I assume not, but without digging in
it wasn't obvious. Maybe this is something we can document to make it
clearer.

Based on your description and the recursive virDomainUSBAddressHubFree,
pictorally this is what I envision:

AddressSet
+---------+       AddressHub
| buses[] |----> +--------+
| nbuses  |      | ports[]|----> +---------+      AddressHub
+---------+      | nports |      | ports[0]|----> +--------+
                 +--------+      | ports[1]|      | ports  + ...
                  nports != 0    +---------+      | nports |
                                                  +--------+
                                                  nports != 0

After reading further, I think AddressSet may need to define which
controller index is being used rather than allocate an array of buses
based on the controller index found.

It also took me a while to find where nports == 0 (the code wasn't self
commenting ;-))

It seems that AddressSet is an array of buses (numbered 0...0xfff
according to formatdomain.html.in - something that could be checked) and
that AddressHub is either another hub (eg, an array of 'nports') or a
device. An array entry can contain either another array or a device.
There can be up to 4 nested octets.

> ---
>  src/conf/domain_addr.c   | 41 +++++++++++++++++++++++++++++++++++++++++
>  src/conf/domain_addr.h   | 17 +++++++++++++++++
>  src/libvirt_private.syms |  2 ++
>  3 files changed, 60 insertions(+)
> 
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index a5d142d..3962357 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -1198,3 +1198,44 @@ virDomainUSBAddressGetPortString(unsigned int *port)
>          return NULL;
>      return virBufferContentAndReset(&buf);
>  }
> +
> +
> +virDomainUSBAddressSetPtr
> +virDomainUSBAddressSetCreate(void)
> +{
> +    virDomainUSBAddressSetPtr addrs;
> +
> +    if (VIR_ALLOC(addrs) < 0)
> +        return NULL;
> +

I think we may need an 'addrs->ctlr_idx = -1;'

> +    return addrs;
> +}
> +
> +
> +static void
> +virDomainUSBAddressHubFree(virDomainUSBAddressHubPtr hub)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < hub->nports; i++) {
> +        if (hub->ports[i])
> +            virDomainUSBAddressHubFree(hub->ports[i]);
> +    }
> +    VIR_FREE(hub->ports);
> +    VIR_FREE(hub);
> +}
> +

Need extra LF (others had 2 this one had 1)

> +void virDomainUSBAddressSetFree(virDomainUSBAddressSetPtr addrs)

void
virDomain*

> +{
> +    size_t i;
> +
> +    if (!addrs)
> +        return;
> +
> +    for (i = 0; i < addrs->nbuses; i++) {
> +        if (addrs->buses[i])
> +            virDomainUSBAddressHubFree(addrs->buses[i]);
> +    }
> +    VIR_FREE(addrs->buses);
> +    VIR_FREE(addrs);
> +}
> diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
> index c6d8da7..dcf86d4 100644
> --- a/src/conf/domain_addr.h
> +++ b/src/conf/domain_addr.h
> @@ -248,4 +248,21 @@ char *
>  virDomainUSBAddressGetPortString(unsigned int *port)
>      ATTRIBUTE_NONNULL(1);
>  
> +typedef struct _virDomainUSBAddressHub virDomainUSBAddressHub;
> +typedef virDomainUSBAddressHub *virDomainUSBAddressHubPtr;
> +struct _virDomainUSBAddressHub {
> +    virDomainUSBAddressHubPtr *ports;
> +    size_t nports;
> +};
> +
> +struct _virDomainUSBAddressSet {
> +    virDomainUSBAddressHubPtr *buses;
> +    size_t nbuses;

This may need an:

    int ctlr_idx;


John
> +};
> +typedef struct _virDomainUSBAddressSet virDomainUSBAddressSet;
> +typedef virDomainUSBAddressSet *virDomainUSBAddressSetPtr;
> +
> +virDomainUSBAddressSetPtr virDomainUSBAddressSetCreate(void);
> +void virDomainUSBAddressSetFree(virDomainUSBAddressSetPtr addrs);
> +
>  #endif /* __DOMAIN_ADDR_H__ */
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 5168230..a628d00 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -108,6 +108,8 @@ virDomainPCIAddressSlotInUse;
>  virDomainPCIAddressValidate;
>  virDomainUSBAddressGetPortBuf;
>  virDomainUSBAddressGetPortString;
> +virDomainUSBAddressSetCreate;
> +virDomainUSBAddressSetFree;
>  virDomainVirtioSerialAddrAssign;
>  virDomainVirtioSerialAddrAutoAssign;
>  virDomainVirtioSerialAddrIsComplete;
>