[libvirt] [PATCH v2 1/6] tests: qemuxml2argv: remove some QEMU_CAPS_DEVICE problem cases

[Martin Kletzander]

On Tue, Feb 09, 2016 at 02:42:53PM -0500, Cole Robinson wrote:
>On 02/09/2016 02:28 PM, Laine Stump wrote:
>> On 02/09/2016 01:46 PM, Cole Robinson wrote:
>>> On 02/09/2016 01:31 PM, Laine Stump wrote:
>>>> On 02/09/2016 10:58 AM, Cole Robinson wrote:
>>>>> When we unconditionally enable QEMU_CAPS_DEVICE, these tests need
>>>>> some massaging, so do it ahead of time to not mix it in with the
>>>>> big test refresh.
>>>>>
>>>>> - minimal-s390 is not a real world working config, so drop it
>>>>> - disk-usb was testing for an old code path that will be removed.
>>>>>     instead use it to test lack of USB disk support, and rename it
>>>>>     to disk-usb-nosupport. Switch xml2xml to use disk-usb-device for
>>>>>     input.
>>>>> - cputune-numatune was needlessly using q35, switch it to an older
>>>>>     machine type
>>>>> ---
>>>>>    .../qemuxml2argv-cputune-numatune.args             |  7 +++----
>>>>>    .../qemuxml2argv-cputune-numatune.xml              | 12 +----------
>>>>>    .../qemuxml2argv-disk-usb-nosupport.xml}           |  0
>>>>>    tests/qemuxml2argvdata/qemuxml2argv-disk-usb.args  | 23
>>>>> ----------------------
>>>>>    .../qemuxml2argv-minimal-s390.args                 | 21
>>>>> --------------------
>>>>>    .../qemuxml2argvdata/qemuxml2argv-minimal-s390.xml | 21
>>>>> --------------------
>>>>>    tests/qemuxml2argvtest.c                           |  5 ++---
>>>>>    .../qemuxml2xmlout-cputune-numatune.xml            | 14 +++----------
>>>>>    .../qemuxml2xmlout-disk-usb-device.xml}            |  6 ++----
>>>>>    tests/qemuxml2xmltest.c                            |  2 +-
>>>>>    10 files changed, 12 insertions(+), 99 deletions(-)
>>>>>    rename tests/{qemuxml2xmloutdata/qemuxml2xmlout-disk-usb.xml =>
>>>>> qemuxml2argvdata/qemuxml2argv-disk-usb-nosupport.xml} (100%)
>>>>>    delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-usb.args
>>>>>    delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-minimal-s390.args
>>>>>    delete mode 100644 tests/qemuxml2argvdata/qemuxml2argv-minimal-s390.xml
>>>>>    rename tests/{qemuxml2argvdata/qemuxml2argv-disk-usb.xml =>
>>>>> qemuxml2xmloutdata/qemuxml2xmlout-disk-usb-device.xml} (90%)
>>>>>
>>>>> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-cputune-numatune.args
>>>>> b/tests/qemuxml2argvdata/qemuxml2argv-cputune-numatune.args
>>>>> index 7e5678d..bde6338 100644
>>>>> --- a/tests/qemuxml2argvdata/qemuxml2argv-cputune-numatune.args
>>>>> +++ b/tests/qemuxml2argvdata/qemuxml2argv-cputune-numatune.args
>>>>> @@ -7,16 +7,15 @@ QEMU_AUDIO_DRV=none \
>>>>>    /usr/bin/qemu-system-x86_64 \
>>>>>    -name dummy2 \
>>>>>    -S \
>>>>> --M pc-q35-2.3 \
>>>>> +-M pc-0.11 \
>>>>>    -m 128 \
>>>>>    -smp 2,maxcpus=6,sockets=6,cores=1,threads=1 \
>>>>>    -object iothread,id=iothread1 \
>>>>>    -object iothread,id=iothread2 \
>>>>>    -uuid 4d92ec27-9ebf-400b-ae91-20c71c647c19 \
>>>>>    -nographic \
>>>>> +-nodefaults \
>>>>>    -monitor unix:/tmp/test-monitor,server,nowait \
>>>>>    -no-acpi \
>>>>>    -boot c \
>>>>> --net none \
>>>>> --serial none \
>>>>> --parallel none
>>>>> +-usb
>>>> Why did I think that we had eliminated all use of "-usb"? Is it that we're
>>>> currently only using it in the tests? If so, that's another thing to eliminate
>>>> - we should never be using -usb unless that's the only way to get a usb device
>>>> for a particular qemu (and I don't know that that is *ever* the case).
>>>>
>>> The qemu_command.c chunk is:
>>>
>>>      if (usbcontroller == 0 &&
>>>          !qemuDomainMachineIsQ35(def) &&
>>>          !ARCH_IS_S390(def->os.arch))
>>>          virCommandAddArg(cmd, "-usb")
>>>
>>> Where usbcontroller == 0 if we didn't build a -device string for a USB
>>> controller. I'd need to dig into qemu to figure out what -usb actually maps to
>>> to determine if that makes sense anymore
>>
>> Okay, that's what I remember. usbcontroller will only be 0 if there was no
>> <controller type='usb'/> in the xml, and that shouldn't happen because the
>> post-parse will add a usb controller if there isn't any (except in a few cases
>> - see qemuDomainDefAddDefaultDevices). So is the post-parse not happening for
>> the qemuxml2argvtest? (there are dozens and dozens of "-usb" in the *.args files)
>>
>>
>
>PostParse is triggered automatically via DomainDefParse. You can see in the
>qemuxml2xml output that there's a USB controller added.
>
>Looking deeper, that usbcontroller bit is only set if QEMU_CAPS_PIIX3_USB_UHCI
>is available, otherwise it uses legacyusb aka -usb. Maybe that's another flag
>that we can assume is available unconditionally for the qemu we support, I'd
>need to dig a bit
>

Just my two cents here.  I remember that we were dealing with this
because of ppc64 with Andrea and I think we are using '-usb' in one
particular case even when there is the option to specify it as a
device.  That case is when we know the default usb controller that used
to be added with '-usb' and that particular controller is not available
(listed) in the list of devices, so to keep the guest ABI the same, we
fallback to '-usb' because we have no other way to do that.  However,
this will be a problem in the future when qemu changes the default
controller added with -usb.  That's just another rabbit hole I don't
want to go too deep in.  Maybe Andrea already knows more and started
doing something with it.  Andrea?

>- Cole
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160210/fe77d1d2/attachment-0001.sig>