[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] test: remove s390 tests used only for testing usb and ide controllers



On Thu, Apr 30, 2015 at 04:40:46PM -0400, Laine Stump wrote:
> Back in 2013, commit 877bc089 added in some tests that made sure no
> error was generated on a domain definition that had an automatically
> added usb controller if that domain didn't have a PCI bus to attach
> the usb controller to. In particular, two s390-specific tests were
> added, one with <controller type='usb' model='none'/> and another
> (called "s390-piix-controllers") that had both usb and ide
> controllers, but nothing attached to them.
> 

It was a synthetic test (I never ran the command line) meant to check
the fix for starting s390 guests:
https://www.redhat.com/archives/libvir-list/2013-April/msg01920.html
which I posted instead of this proposed patch
https://www.redhat.com/archives/libvir-list/2013-April/msg01925.html

> Then in February of this year, commit 09ab9dcc eliminated the annoying
> auto-adding of a usb device for s390 and s390x machines, stating:
> 
>  "Since s390 does not support usb the default creation of a usb
>   controller for a domain should not occur."
> 
> Since s390 doesn't support usb and usb controllers aren't added to
> s390 domain definitions automatically, there is no reason to have the
> tests with a usb controller and expect them to succeed.

QEMU seems to ignore the -usb parameter, so the only reason would be:
'It worked before'. (Unless running the qemu-system-s390x binary without
an guest on x86_64 does not give me results matching the real-world
usage).

I'm okay with breaking the controllers with explicit PCI addresses,
since libvirt stopped adding those almost 2 years ago.

Erroring out on bare <controller type='usb'> would break domains that
worked just three releases ago (the tests and the fix was added
precisely to avoid that).

Also, USB controller model='none' should be allowed.

> And since the
> only reference of an IDE controller wrt s390 that I've found is in the
> one test case mentioned above, and the commit log that added it
> specifically mentions the purpose to be quieting error messages on
> machines with no PCI bus, I'm assuming that the s390 also doesn't
> support IDE controllers. Based on that reasoning

The IDE controller seem to be added only if something uses the IDE bus
(the problem seems to be the disk target starting with 'hd')
Both tests can be fixed to avoid that.

> (and the fact that
> s390-piix-controllers causes a test error for an upcoming patch),

Honestly, that is not a great reason to remove a test :)

> this patch removes those two tests.

Jan

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]