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

[Peter Xu <peterx@redhat.com>]

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.

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

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.

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.

Thanks,

-- 
Peter Xu