Re: [Qemu-devel] [PATCH 3/3] pc: Don't make CPU properties mandatory unless necessary

From: Markus Armbruster <armbru@redhat.com>
To: Eduardo Habkost <ehabkost@redhat.com>
Cc: Yash Mankad <ymankad@redhat.com>,
	Peter Krempa <pkrempa@redhat.com>,
	Like Xu <like.xu@linux.intel.com>,
	Erik Skultety <eskultet@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	qemu-devel@nongnu.org, Igor Mammedov <imammedo@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Miroslav Rezanina <mrezanin@redhat.com>,
	"Danilo C. L. de Paula" <ddepaula@redhat.com>,
	Richard Henderson <rth@twiddle.net>
Subject: Re: [Qemu-devel] [PATCH 3/3] pc: Don't make CPU properties mandatory unless necessary
Date: Sat, 17 Aug 2019 07:34:41 +0200	[thread overview]
Message-ID: <8736i0iccu.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20190816174241.GE3908@habkost.net> (Eduardo Habkost's message of "Fri, 16 Aug 2019 14:42:41 -0300")

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Fri, Aug 16, 2019 at 02:22:58PM +0200, Markus Armbruster wrote:
>> Erik Skultety <eskultet@redhat.com> writes:
>> 
>> > On Fri, Aug 16, 2019 at 08:10:20AM +0200, Markus Armbruster wrote:
>> >> Eduardo Habkost <ehabkost@redhat.com> writes:
>> >>
>> >> > We have this issue reported when using libvirt to hotplug CPUs:
>> >> > https://bugzilla.redhat.com/show_bug.cgi?id=1741451
>> >> >
>> >> > Basically, libvirt is not copying die-id from
>> >> > query-hotpluggable-cpus, but die-id is now mandatory.
>> >>
>> >> Uh-oh, "is now mandatory": making an optional property mandatory is an
>> >> incompatible change.  When did we do that?  Commit hash, please.
>> >>
>> >> [...]
>> >>
>> >
>> > I don't even see it as being optional ever - the property wasn't even
>> > recognized before commit 176d2cda0de introduced it as mandatory.
>> 
>> Compatibility break.
>> 
>> Commit 176d2cda0de is in v4.1.0.  If I had learned about it a bit
>> earlier, I would've argued for a last minute fix or a revert.  Now we
>> have a regression in the release.
>> 
>> Eduardo, I think this fix should go into v4.1.1.  Please add cc:
>> qemu-stable.
>
> I did it in v2.
>
>> 
>> How can we best avoid such compatibility breaks to slip in undetected?
>> 
>> A static checker would be nice.  For vmstate, we have
>> scripts/vmstate-static-checker.py.  Not sure it's used.
>
> I don't think this specific bug would be detected with a static
> checker.  "die-id is mandatory" is not something that can be
> extracted by looking at QOM data structures.  The new rule was
> being enforced by the hotplug handler callbacks, and the hotplug
> handler call tree is a bit complex (too complex for my taste, but
> I digress).

QOM does too much in code.  Turing tarpit.

> We could have detected this with a simple CPU hotplug automated
> test case, though.  Or with a very simple -device test case like
> the one I have submitted with this patch.

The external QOM interface is huge.  Even if we had an army of
industrious gnomes writing simple test cases for all of it, we'd still
need a fleet of machines to actually run them, and at least a batallion
of gnomes to maintain them.

The extremely basic qom-test gobbles up a painful amount of CPU cycles
already:

$ time for i in `find bld/*-softmmu -maxdepth 1 -name qemu-system-\* -perm /u+x`; do QTEST_QEMU_BINARY=$i bld/tests/qom-test; done
/aarch64/qom/versatileab: OK
[260 lines of the form name/of/test: OK omitted...]
/xtensaeb/qom/lx60: OK

real	3m33.001s
user	2m18.081s
sys	1m31.809s

> This was detected by libvirt automated test cases.  It would be

Nice.

> nice if this was run during the -rc stage and not only after the
> 4.1.0 release, though.

We don't always get lucky.

> I don't know details of the test job.  Danilo, Mirek, Yash: do
> you know how this bug was detected, and what we could do to run
> the same test jobs in upstream QEMU release candidates?

Thinking about how to make the best use of the tests we have is in
order.