[libvirt] [PATCH 4/5] qemu: define virDomainDevAddUSBController()

[John Ferlan]

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",
>