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

Ján Tomko jtomko at redhat.com
Mon Jul 18 15:55:54 UTC 2016


On Wed, Jul 13, 2016 at 06:43:51PM -0400, John Ferlan wrote:
>
>
>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.
>

I think checking it would be paranoid.

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

All the callers care.

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

Currently, usb is the only possible hub type.

Other address types get ignored by the condition below and are
overwritten by a freshly assigned USB address later.

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

At this point, only USB hubs have been added to the structure.

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

The other usage will be adding a hub we've just created with an address
we've generating.

Jan




More information about the libvir-list mailing list