[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 05/04/2015 08:59 AM, Ján Tomko wrote:
> 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

(Thanks for the extra history. I had extracted part of it from the
commit logs, but didn't know about that original patch.)

Yes, that definitely would not be the right fix. We shouldn't pretend
that a domain has hardware that it doesn't actually have (it was fixing
up yet another occurrence of that problem that led me to this).

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

Will the badness never end? :-/

(seriously, this entire collection of problems/patches is all due to one
bit of code or another silently implying that nonexistent things exist
and/or not logging an error and failing when they actually don't exist.
If everyone involved would just *tell the truth* we could stop the
insanity).

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

Ugh. This is very distasteful, but I do see your point. Note that this
patch doesn't add any new error conditions, it just removes a test that
verifies acceptance of something that shouldn't have happened in the
first place. However, it was libvirt itself that created these
technically invalid configs (by auto-adding the <usb>, so it would be a
bit mean-spirited to suddenly begin failing.

The "other patch" I refer to in the commit log is one that results in an
error log only for IDE controllers in a domain that doesn't support IDE,
though. Since libvirt itself never adds an IDE controller automatically
unless someone adds a disk that attaches to an IDE controller, and since
any disk attached to a nonexistent IDE controller would result in an
error from QEMU on startup (except in the case of a Q35 domain, which is
exactly the ignored case where I *want* it to cause an error), I
seriously doubt that there are any configs in the wild that match the
following:

  1) target domain doesn't support IDE
  2) config has IDE controller
  3) config has no IDE disk (and thus currently doesn't cause an error)

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

Yeah, I guess I can live with that one.


>> 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')

Right. So up until now there are only two ways an IDE controller would
have been added:

1) user adds a disk and says that it attaches to the IDE bus. qemu
generates an error, user removes disk.

or

2) user manually adds an IDE controller for no good reason.

or the *real* problem, which occurs only on Q35 domains (since the Q35
domain has a SATA controller that qemu gives the id "ide" (which is
identical to the integrated IDE controller on a 440fx domain):

3) user adds a disk and says that it attaches to the IDE bus. libvirt
generates commandline saying "attach this to id=ide", qemu says "okay,
I'm attaching this to the SATA controller named 'ide'" and proceeds on
its happy way. (this later causes a problem if the user attaches a disk
to the SATA bus, or a 2nd disk to the "IDE" bus).

libvirt should have *always* flagged all of these as errors - not
flagging (1) and (2) as errors is what led to the bug in (3) creeping in
and being unidentified for so long (see
https://bugzilla.redhat.com/show_bug.cgi?id=1207834 ) and leaving it
that way would only lead to more bugs.

> Both tests can be fixed to avoid that.

Some amount of lenience for bad config is okay, but since allowing a
non-supported IDE controller into the code has been demonstrated to have
allowed a bug into the code that lay dormant for over a year and has
caused such a headache now, I think the proper thing to do is fix it and
take the short-term pain rather than continuing to gloss it over.


>> (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 :)

The upcoming patch correctly identifies an IDE controller being defined
when it shouldn't be; the current s390-piix-controllers test is arguably
invalid - the test is just testing that we allow a bad config.

My opinion of this is that:

1) we shouldn't test to verify that we allow an invalid config. This is
institutionalizing bad behavior (and misleading those who read the
tests). (For awhile I was wondering if the s390 virtual machine actually
did have a piix controller on it - "there's a test case for it, so it
must exist")

2) I have no problem with *allowing* <controller usb ...> in domains
that have no support for USB, although I think we should add a
VIR_WARNING() pointing out that the config has an unsupported (and
therefore ignored) USB controller defined, and that sometime in the
future this may turn into an error. Then after [some undetermined time]
this VIR_WARNING() can be changed to virReportError().

3) Since ide controllers should only have been added by user
interaction, I think we should immediately flag them all as errors.

(neither 2 nor 3 is done by this current patch, but (3) is done by the
next patch after this one on my dev branch - I just sent this patch
early because I wanted to make sure the authors of 09ab9dcc had ample
time to respond).

Perhaps a reasonable compromise would be this:

1) rename the s390-piix-controllers test to
"s390-allow-bogus-usb-controller" (or something like that) and remove
the ide controller from it.

2) likewise rename "s390-usb-none" to "s390-allow-bogus-usb-controller-none"

(the idea of both of these is to make it apparent to the untrained eye
that this is testing for something that isn't really valid, but we're
allowing for now).

3) in a separate patch, add a VIR_WARN() any time a domain has a USB
controller but doesn't support USB.

4) (already done in an upcoming patch) log an error if a domain has an
ide controller defined when it isn't supported.

How much (if any :-) or that would you agree to?



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