[libvirt] [PATCHv4 9/9] Auto-add one hub if there are too many USB devices

[Ján Tomko]

On Thu, Jul 14, 2016 at 01:00:09PM -0400, John Ferlan wrote:
>
>
>On 07/01/2016 11:38 AM, Ján Tomko wrote:
>> When parsing a command line with USB devices that have
>> no address specified, QEMU automatically adds a USB hub
>> if the device would fill up all the available USB ports.
>>
>> To help most of the users, add one hub if there are more
>> USB devices than available ports. For wilder configurations,
>> expect the user to provide us with more hubs and/or controllers.
>> ---
>>  src/conf/domain_addr.c                             | 20 +++++++++
>>  src/conf/domain_addr.h                             |  2 +
>>  src/libvirt_private.syms                           |  1 +
>>  src/qemu/qemu_domain_address.c                     | 48 ++++++++++++++++++++++
>>  .../qemuxml2argv-usb-hub-autoadd.args              | 28 +++++++++++++
>>  .../qemuxml2argv-usb-hub-autoadd.xml               | 23 +++++++++++
>>  tests/qemuxml2argvtest.c                           |  3 ++
>>  7 files changed, 125 insertions(+)
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-hub-autoadd.args
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-usb-hub-autoadd.xml
>>
>
>> +static int
>> +qemuDomainUSBAddressAddHubs(virDomainDefPtr def)
>> +{
>> +    struct qemuAssignUSBIteratorInfo data = { .count = 0 };
>> +    virDomainHubDefPtr hub = NULL;
>> +    size_t available_ports;
>> +    int ret = -1;
>> +
>> +    available_ports = virDomainUSBAddressCountAvailablePorts(def);
>> +    ignore_value(virDomainUSBDeviceDefForeach(def,
>> +                                              qemuDomainAssignUSBPortsCounter,
>> +                                              &data,
>> +                                              false));
>> +    VIR_DEBUG("Found %zu USB devices and %zu provided USB ports",
>> +              data.count, available_ports);
>> +
>> +    /* Add one hub if there are more devices than ports
>> +     * otherwise it's up to the user to specify more hubs/controllers */
>
>What if one hub isn't enough? Wouldn't we need to keep adding until we
>have enough?   I'm sure a test could be created for it...
>

For that many devices, most of the users would probably be better off
with more USB controllers than chaining hubs.

The aim of this code is to cover most of the cases, not all of them.

>> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-usb-hub-autoadd.xml b/tests/qemuxml2argvdata/qemuxml2argv-usb-hub-autoadd.xml
>> new file mode 100644
>> index 0000000..43e0f1f
>> --- /dev/null
>> +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-hub-autoadd.xml
>> @@ -0,0 +1,23 @@
>> +<domain type='qemu'>
>> +  <name>QEMUGuest1</name>
>> +  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
>> +  <memory unit='KiB'>219136</memory>
>> +  <currentMemory unit='KiB'>219136</currentMemory>
>> +  <vcpu placement='static'>1</vcpu>
>> +  <os>
>> +    <type arch='i686' machine='pc'>hvm</type>
>> +    <boot dev='hd'/>
>> +  </os>
>> +  <devices>
>> +    <emulator>/usr/bin/qemu</emulator>
>> +    <controller type='usb' index='0'/>
>
>So to try and answer something I was thinking about earlier... If I just
>change index from 0 to 1, then I get an error:
>
>libvirt: QEMU Driver error : unsupported configuration: Multiple legacy
>USB controllers are not supported
>
>If I then adjust the qemuxml2argvtest to add QEMU_CAPS_PIIX3_USB_UHCI,
>QEMU_CAPS_PCI_MULTIFUNCTION, then I get the failure:
>
>error : virDomainUSBAddressReserve:1724 : XML error: Duplicate USB
>address bus 0 port 1
>

I could not reproduce this error. Maybe I fixed it along the way?

Jan

>So is it a "valid" test to have one defined for index='1' and allow the
>code to create index='0'?
>
>
>John