10:55 a.m.
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