On 07/01/2016 11:38 AM, Ján Tomko wrote:
> Walk through all the usb controllers in the domain definition
> and create the corresponding structures in the virDomainUSBAddressSet.
> ---
> src/conf/domain_addr.c | 121 +++++++++++++++++++++++++++++++++++++++++++++++
> src/conf/domain_addr.h | 4 ++
> src/libvirt_private.syms | 1 +
> 3 files changed, 126 insertions(+)
>
> diff --git a/src/conf/domain_addr.c b/src/conf/domain_addr.c
> index 5e84751..82fe295 100644
> --- a/src/conf/domain_addr.c
> +++ b/src/conf/domain_addr.c
> @@ -1330,3 +1330,124 @@ virDomainUSBAddressSetFree(virDomainUSBAddressSetPtr addrs)
> VIR_FREE(addrs->buses);
> VIR_FREE(addrs);
> }
> +
> +
> +static size_t
> +virDomainUSBAddressControllerModelToPorts(virDomainControllerDefPtr cont)
> +{
> + int model = cont->model;
> +
> + if (model == -1)
> + model = VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI;
> +
> + switch ((virDomainControllerModelUSB) model) {
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI:
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX4_UHCI:
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_VT82C686B_UHCI:
> + return 2;
> +
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_EHCI:
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1:
> + return 6;
> +
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1:
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2:
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3:
> + /* These have two ports each and are used to provide USB1.1
> + * ports while ICH9_EHCI1 provides 6 USB2.0 ports.
> + * Ignore these since we will add the EHCI1 too. */
> + return 0;
> +
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI:
> + return 3;
> +
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI:
> + if (cont->opts.usbopts.ports != -1)
> + return cont->opts.usbopts.ports;
> + return 4;
> +
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_NONE:
> + case VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST:
> + /* yoda */
Prior review by Erik asked for this to be removed...
> + break;
> + }
> + return 0;
> +}
> +
> +
> +static virDomainUSBAddressHubPtr
> +virDomainUSBAddressHubNew(size_t nports)
> +{
> + virDomainUSBAddressHubPtr hub = NULL, ret = NULL;
> +
> + if (VIR_ALLOC(hub) < 0)
> + goto cleanup;
> +
> + if (!(hub->ports = virBitmapNew(nports)))
> + goto cleanup;
> +
> + if (VIR_ALLOC_N(hub->hubs, nports) < 0)
> + goto cleanup;
> + hub->nports = nports;
> +
> + ret = hub;
> + hub = NULL;
> + cleanup:
> + virDomainUSBAddressHubFree(hub);
> + return ret;
> +}
> +
> +
> +static int
> +virDomainUSBAddressSetAddController(virDomainUSBAddressSetPtr addrs,
> + virDomainControllerDefPtr cont)
> +{
> + size_t nports = virDomainUSBAddressControllerModelToPorts(cont);
> + virDomainUSBAddressHubPtr hub = NULL;
> + int ret = -1;
> +
> + VIR_DEBUG("Adding a USB controller model=%s with %zu ports",
> + virDomainControllerModelUSBTypeToString(cont->model),
> + nports);
> +
> + /* Skip UHCI{1,2,3} companions; only add the EHCI1 */
> + if (nports == 0)
> + return 0;
> +
> + if (addrs->nbuses <= cont->idx) {
> + if (VIR_EXPAND_N(addrs->buses, addrs->nbuses, cont->idx - addrs->nbuses + 1) < 0)
> + goto cleanup;
So if "someone" decides to be cute and have 1 USB controller, but number
it like 1000, then you're allocating and wasting a lot of memory. I
understand it makes the access a lot faster, but is that what we want?
Someone would have to be "malicious" and equally powerful as yoda to
adjust their XML thusly - of course in the alternate reality 1000000
would take a bigger hit.
Unless someone else has severe heartburn over this (and it wasn't
mentioned by Eric for his ACK), I'm OK with it...
> + } else if (addrs->buses[cont->idx]) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Duplicate USB controllers with index %u"),
> + cont->idx);
> + goto cleanup;
> + }
> +
> + if (!(hub = virDomainUSBAddressHubNew(nports)))
> + goto cleanup;
> +
> + addrs->buses[cont->idx] = hub;
> + hub = NULL;
> +
> + ret = 0;
> + cleanup:
> + virDomainUSBAddressHubFree(hub);
> + return ret;
> +}
> +
> +
> +int virDomainUSBAddressSetAddControllers(virDomainUSBAddressSetPtr addrs,
> + virDomainDefPtr def)
This one almost escaped attention (multiline the function)
ACK w/ at least this adjustment
John
I also didn't go through any practice uses under the environment like
Erik seemed to do in his review of the previous version.
> +{
> + size_t i;
> +
> + for (i = 0; i < def->ncontrollers; i++) {
> + virDomainControllerDefPtr cont = def->controllers[i];
> + if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) {
> + if (virDomainUSBAddressSetAddController(addrs, cont) < 0)
> + return -1;
> + }
> + }
> + return 0;
> +}
> diff --git a/src/conf/domain_addr.h b/src/conf/domain_addr.h
> index c62ff6a..4cb0212 100644
> --- a/src/conf/domain_addr.h
> +++ b/src/conf/domain_addr.h
> @@ -269,6 +269,10 @@ typedef struct _virDomainUSBAddressSet virDomainUSBAddressSet;
> typedef virDomainUSBAddressSet *virDomainUSBAddressSetPtr;
>
> virDomainUSBAddressSetPtr virDomainUSBAddressSetCreate(void);
> +
> +int virDomainUSBAddressSetAddControllers(virDomainUSBAddressSetPtr addrs,
> + virDomainDefPtr def)
> + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> void virDomainUSBAddressSetFree(virDomainUSBAddressSetPtr addrs);
>
> #endif /* __DOMAIN_ADDR_H__ */
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 49f8d6c..f0fed8e 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -110,6 +110,7 @@ virDomainPCIControllerModelToConnectType;
> virDomainUSBAddressPortFormat;
> virDomainUSBAddressPortFormatBuf;
> virDomainUSBAddressPortIsValid;
> +virDomainUSBAddressSetAddControllers;
> virDomainUSBAddressSetCreate;
> virDomainUSBAddressSetFree;
> virDomainVirtioSerialAddrAssign;
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list