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

[Laine Stump]

On 12/09/2015 09:36 AM, John Ferlan wrote:
>
> 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

  'git rebase -i -x "make -j8 check && make -j8 syntax-check" master' is 
my friend :-)

>
>> 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
>   */

Actually, more like this:

/**
  * functionName:
  *
  * @arg1: description
  * @argn: description
  *
  * Function description
  *
  * Returns description
  */

I changed it accordingly.

>> +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...


This is only called if there are no other USB controllers in the config, 
so no that could never happen.


>
>> +    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?

No, it should be a part of patch 5. Must have gotten mixed up during a 
rebase. I moved it.

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