[libvirt] [PATCHv4 5/9] Add functions for adding USB hubs to addrs

[John Ferlan]

On 07/01/2016 11:38 AM, Ján Tomko wrote:
> Walk through all the usb hubs in the domain definition
> that have a USB address specified, create the
> corresponding structures in the virDomainUSBAddressSet
> and mark the port it occupies as used.
> ---
>  src/conf/domain_addr.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 115 insertions(+)
> 
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index 82fe295..2c511ba 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -1437,6 +1437,109 @@ virDomainUSBAddressSetAddController(virDomainUSBAddressSetPtr addrs,
>  }
>  
>  
> +static ssize_t
> +virDomainUSBAddressGetLastIdx(virDomainDeviceInfoPtr info)
> +{
> +    ssize_t i;
> +    for (i = VIR_DOMAIN_DEVICE_USB_MAX_PORT_DEPTH - 1; i > 0; i--) {
> +        if (info->addr.usb.port[i] != 0)
> +            break;
> +    }
> +    return i;
> +}

I would say the one thing that concerns me if some code calls
virDomainUSBAddressSetAddHub or virDomainUSBAddressFindPort without
first checking virDomainUSBAddressPortIsValid

> +
> +
> +/* Find the USBAddressHub structure representing the hub/controller
> + * that corresponds to the bus/port path specified by info.
> + * Returns the index of the requested port in targetIdx.
> + */
> +static virDomainUSBAddressHubPtr
> +virDomainUSBAddressFindPort(virDomainUSBAddressSetPtr addrs,
> +                            virDomainDeviceInfoPtr info,
> +                            int *targetIdx,
> +                            const char *portStr)
> +{
> +    virDomainUSBAddressHubPtr hub = NULL;
> +    ssize_t i, lastIdx;
> +
> +    if (info->addr.usb.bus >= addrs->nbuses ||
> +        !addrs->buses[info->addr.usb.bus]) {
> +        virReportError(VIR_ERR_XML_ERROR, _("Missing USB bus %u"),
> +                       info->addr.usb.bus);
> +        return NULL;
> +    }
> +    hub = addrs->buses[info->addr.usb.bus];
> +
> +    lastIdx = virDomainUSBAddressGetLastIdx(info);

Again to the same point if lastIdx == 0, then someone didn't check
virDomainUSBAddressPortIsValid before calling...  and there's some
internal error, but I'm not sure it's worth checking... on the fence for
now I guess.

> +
> +    for (i = 0; i < lastIdx; i++) {
> +        /* ports are numbered from 1 */
> +        int portIdx = info->addr.usb.port[i] - 1;
> +
> +        if (hub->nports <= portIdx) {
> +            virReportError(VIR_ERR_XML_ERROR,
> +                           _("port %u out of range in USB address bus: %u port: %s"),
> +                           info->addr.usb.port[i],
> +                           info->addr.usb.bus,
> +                           portStr);
> +            return NULL;
> +        }
> +        hub = hub->hubs[portIdx];
> +    }
> +
> +    *targetIdx = info->addr.usb.port[lastIdx] - 1;

It almost feels like the caller should do this since it's the only one
that cares, but that means the caller has not know about the
addr.usb.port... It's a coin flip.  Since you designed it this way, I'm
OK with it as is.

> +    return hub;
> +}
> +
> +
> +#define VIR_DOMAIN_USB_HUB_PORTS 8
> +
> +static int
> +virDomainUSBAddressSetAddHub(virDomainUSBAddressSetPtr addrs,
> +                             virDomainHubDefPtr hub)
> +{
> +    virDomainUSBAddressHubPtr targetHub = NULL, newHub = NULL;
> +    int ret = -1;
> +    int targetPort;
> +    char *portStr = NULL;
> +
> +    if (hub->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) {

Oy - hub->type and hub->info.type... Seems there should be some other
check somewhere in the code that would ensure that the <address> used
for a <hub> was USB <sigh>

> +        virReportError(VIR_ERR_XML_ERROR, "%s",
> +                       _("Wrong address type for USB hub"));
> +        goto cleanup;
> +    }
> +
> +    if (!(portStr = virDomainUSBAddressPortFormat(hub->info.addr.usb.port)))
> +        goto cleanup;
> +
> +    VIR_DEBUG("Adding a USB hub with 8 ports on bus=%u port=%s",
> +              hub->info.addr.usb.bus, portStr);
> +
> +    if (!(newHub = virDomainUSBAddressHubNew(VIR_DOMAIN_USB_HUB_PORTS)))
> +        goto cleanup;
> +
> +    if (!(targetHub = virDomainUSBAddressFindPort(addrs, &(hub->info), &targetPort,
> +                                                  portStr)))
> +        goto cleanup;
> +
> +    if (targetHub->hubs[targetPort]) {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("Dupicate USB hub on bus %u port %s"),
                             ^^^^^^^^
Duplicate

It could be a USB hub or another USB device, right?  IOW should we say
that the port is already in use by some other USB device.

> +                       hub->info.addr.usb.bus, portStr);
> +        goto cleanup;
> +    }
> +    ignore_value(virBitmapSetBit(targetHub->ports, targetPort));
> +    targetHub->hubs[targetPort] = newHub;
> +    newHub = NULL;
> +
> +    ret = 0;
> + cleanup:
> +    virDomainUSBAddressHubFree(newHub);
> +    VIR_FREE(portStr);
> +    return ret;
> +}
> +
> +
>  int virDomainUSBAddressSetAddControllers(virDomainUSBAddressSetPtr addrs,
>                                           virDomainDefPtr def)
>  {
> @@ -1449,5 +1552,17 @@ int virDomainUSBAddressSetAddControllers(virDomainUSBAddressSetPtr addrs,
>                  return -1;
>          }
>      }
> +
> +    for (i = 0; i < def->nhubs; i++) {
> +        virDomainHubDefPtr hub = def->hubs[i];
> +        if (hub->type == VIR_DOMAIN_HUB_TYPE_USB &&
> +            hub->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB &&
> +            virDomainUSBAddressPortIsValid(hub->info.addr.usb.port)) {
^^^
This line ensures we have a port value from the XML; however, as pointed
out above I'm hoping no other future caller will need to also check
this.  I'd almost say we should move that check inside the SetAddHub.
Your call though as you know where this is headed (I'm thinking are
there going to be issues with hotplug and/or migration).


ACK to what's here as long as the error message is adjusted.

John

FYI: I'll continue with the rest tomorrow - although at least 8 won't
git am -3 for me as I guess things have diverged enough.


> +            /* USB hubs that do not yet have an USB address have to be
> +             * dealt with later */
> +            if (virDomainUSBAddressSetAddHub(addrs, hub) < 0)
> +                return -1;
> +        }
> +    }
>      return 0;
>  }
>