On 11/19/2015 01:25 PM, Laine Stump wrote:
> This new function will add a single controller of the given model,
> except the case of ich9-usb-ehci1 (the master controller for a USB2
> controller set) in which case a set of related controllers will be
> added (EHCI1, UHCI1, UHCI2, UHCI3). These controllers will not be
> given PCI addresses, but should be otherwise ready to use.
>
> "-1" is allowed for controller model, and means "default for this
> machinetype". This matches the existing practice in
> qemuDomainDefPostParse(), which always adds the default controller
> with model = -1, and relies on the commandline builder to set a model
> (that is wrong, but will be fixed later).
> ---
> src/conf/domain_conf.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> src/conf/domain_conf.h | 2 ++
> src/libvirt_private.syms | 1 +
> tests/qemuxml2argvtest.c | 1 +
> 4 files changed, 52 insertions(+)
>
Wasn't able to "git am -3" this patch - so I assume you have some merge
conflicts coming... Safe to assume from your side that I wasn't able to
compile this - so I'll further assume this and the next patch will have
gone through check/check-syntax processing
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index ab05e7f..63888b1 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -14410,6 +14410,54 @@ virDomainDefAddController(virDomainDefPtr def, int type, int idx, int model)
> }
>
>
> +/*
> + * virDomainDefAddUSBController() - add a USB controller of the
> + * specified model. If model is ich9-usb-ehci, also add companion
> + * uhci1, uhci2, and uhci3 controllers at the same index. Note that a
> + * model of "-1" is allowed for backward compatibility, and means
> + * "default USB controller for this machinetype".
> + */
Nice! comments, but the format I've seen lately has been:
/* functionName
* @arg1: description
* @argn: description
*
* Function description
*
* Returns description
*/
> +int
> +virDomainDefAddUSBController(virDomainDefPtr def, int idx, int model)
> +{
> + virDomainControllerDefPtr cont; /* this is a *copy* of the DefPtr */
> +
> + cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB,
> + idx, model);
> + if (!cont)
> + return -1;
> +
> + if (model != VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1)
> + return 0;
> +
> + /* When the initial controller is ich9-usb-ehci, also add the
> + * companion controllers
> + */
> +
> + idx = cont->idx; /* in case original request was "-1" */
> +
And the operating assumption being that none of the following exist?
IOW: Would it be possible for someone to add these, but not the above?
Not that anyone (qa) would do that...
> + if (!(cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB,
> + idx, VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1)))
> + return -1;
> + cont->info.mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB;
> + cont->info.master.usb.startport = 0;
> +
> + if (!(cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB,
> + idx, VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2)))
> + return -1;
> + cont->info.mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB;
> + cont->info.master.usb.startport = 2;
> +
> + if (!(cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_USB,
> + idx, VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3)))
> + return -1;
> + cont->info.mastertype = VIR_DOMAIN_CONTROLLER_MASTER_USB;
> + cont->info.master.usb.startport = 4;
> +
> + return 0;
> +}
> +
> +
> int
> virDomainDefMaybeAddController(virDomainDefPtr def,
> int type,
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 8d43ee6..c34bfd0 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -3169,6 +3169,8 @@ int virDomainObjListConvert(virDomainObjListPtr domlist,
> bool skip_missing);
>
> int
> +virDomainDefAddUSBController(virDomainDefPtr def, int idx, int model);
> +int
> virDomainDefMaybeAddController(virDomainDefPtr def,
> int type,
> int idx,
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 7e60d87..b7008e0 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -200,6 +200,7 @@ virDomainControllerTypeToString;
> virDomainCpuPlacementModeTypeFromString;
> virDomainCpuPlacementModeTypeToString;
> virDomainDefAddImplicitControllers;
> +virDomainDefAddUSBController;
> virDomainDefCheckABIStability;
> virDomainDefCheckDuplicateDiskInfo;
> virDomainDefCheckUnsupportedMemoryHotplug;
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 5fe52b0..25ffbea 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -1474,6 +1474,7 @@ mymain(void)
> QEMU_CAPS_DEVICE, QEMU_CAPS_DEVICE_PCI_BRIDGE,
> QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE,
> QEMU_CAPS_ICH9_AHCI,
> + QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_ICH9_USB_EHCI1,
Should this perhaps have been merged into patch 2?
Just seems out of place here.
ACK - seems reasonable to me.
John
> QEMU_CAPS_DEVICE_VIDEO_PRIMARY,
> QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE_QXL);
> DO_TEST("q35-usb2",
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list