Re: [PATCH 0/2] vmstate-static-checker: Fix VMS_ARRAY comparisons

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Wed, Apr 26, 2023 at 06:36:00PM +0200, Juan Quintela wrote:
>> Peter Xu <peterx@redhat.com> wrote:
>> > I'm doing some machine type checks to make sure nothing breaks for
>> > 7.2<->8.0.  Along the way I found one false negative report on e1000e using
>> > the static checker, turns out to be an issue in the checker itself.
>> >
>> > The problem is the checker doesn't take VMS_ARRAY into account when
>> > comparing with UNUSED, hence the total size is wrongly calculated.
>> >
>> > Fix that first in qemu by start dumping size of array as "num", then teach
>> > the checker for that.
>> >
>> > NOTE: the patchset will change both behaviors for either -dump-vmstate on
>> > QEMU or the checker, however both patches will be compatible even with old
>> > QEMU dumps or even old vmstate-checker script.  That's not extremely
>> > important, IMHO, but still worth mentioning.
>> >
>> > Thanks,
>> >
>> > Peter Xu (2):
>> >   migration/vmstate-dump: Dump array size too as "num"
>> >   vmstate-static-checker: Recognize "num" field
>> >
>> >  migration/savevm.c                |  3 +++
>> >  scripts/vmstate-static-checker.py | 13 ++++++++++---
>> >  2 files changed, 13 insertions(+), 3 deletions(-)
>> 
>> Hi
>> 
>> once that you are working with the static checker.
>> 
>> Could we just run two checks in make check:
>> 
>> - qemu-<whatever> -M <previous-version> against the one from previous
>>   version, and see that they match.
>> - qemu-<whatever> -M <latests> against the one from previous version
>>   And we save the diffs each time that we add something incompatible and
>>   fix it on source.
>
> Normally we don't have "latest machine" but only "previous"?  Checking
> "previous" would be enough, right?  E.g. currently we're at 8.1 dev window,
> so we check against 8.0 with whatever new thing coming.

$ qemu-8.0.0 -M pc-q35-8.0.0 > dump-8.0.0-q35

We generate that dump-8.0-q35 on the tree.
We will change that once that we release 8.1, until then that is latests.

This qemu upstream is whatever is in HEAD

$ qemu-upstream -M pc-q35-8.0.0 > dump-8.0.0-upstream-q35

diff dump-8.0.0-q35 dump-8.0.0-upstream-q35

And it should be empty.

$ qemu-upstream -M pc-q35-8.1.0 > dump-8.1.0-upstream-q35

diff dump-8.0.0-q35 dump-8.1.0-upstream-q35

Each time that we find a difference, we know that we have to create a
property for that to make pc-q35-8.0.0 working.  We save that "hunk"
somehow.

Where I have put diff, we can have something more intelligent that is
able to compare json output and have into account that differences that
we already know that exist.

>> I will start with x86_64.  And once that we have it running, the other
>> architectures that care about version compatibility can add to it.
>> 
>> What do you think?
>
> It sounds a good idea to have some way to check compat bits in unit tests.
> I'm just not sure whether it's easy to integrate to make check: the
> comparision requires building two qemu binaries; one normally with an old
> tag that I built manually.

No. Single binary.  For the old binary we just have saved its output on
the tree.

> For the static checker itself, it normally also needs some intervention
> from human, e.g., it doesn't understand things like field_exists() so it
> can report "warnings" only which can be false negative even with ARRAY
> issue fixed.

This is why I mean that I want the "diff" to be a bit more intelligent
and "record" the things that we tell them that are correct.

> But ideally e.g. in a CI env we can always keep an old version qemu binary
> ready for each arch to be tested, then verify forward+backward migration
> with that old machine type with whatever patch applied on top.  One trick
> here is we need to make sure the test cmdline contains the device/anything
> that got changed by the patch.  It may not always be the case.

I will start with the default machine devices.
Once the mechanism is done, we can wonder with more configurations.

I will start small and then go from there.

Later, Juan.