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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list