[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