From: Juan Quintela <quintela@redhat.com>
To: Peter Xu <peterx@redhat.com>
Cc: qemu-devel@nongnu.org,
Leonardo Bras Soares Passos <lsoaresp@redhat.com>
Subject: Re: [PATCH 0/2] vmstate-static-checker: Fix VMS_ARRAY comparisons
Date: Wed, 26 Apr 2023 20:54:02 +0200 [thread overview]
Message-ID: <87mt2ueaph.fsf@secure.mitica> (raw)
In-Reply-To: <ZElvWffplx+1vNH1@x1n> (Peter Xu's message of "Wed, 26 Apr 2023 14:37:13 -0400")
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.
next prev parent reply other threads:[~2023-04-26 18:55 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-25 18:05 [PATCH 0/2] vmstate-static-checker: Fix VMS_ARRAY comparisons Peter Xu
2023-04-25 18:05 ` [PATCH 1/2] migration/vmstate-dump: Dump array size too as "num" Peter Xu
2023-04-26 16:30 ` Juan Quintela
2023-04-25 18:05 ` [PATCH 2/2] vmstate-static-checker: Recognize "num" field Peter Xu
2023-04-26 16:30 ` Juan Quintela
2023-04-26 16:36 ` [PATCH 0/2] vmstate-static-checker: Fix VMS_ARRAY comparisons Juan Quintela
2023-04-26 18:37 ` Peter Xu
2023-04-26 18:54 ` Juan Quintela [this message]
2023-04-26 19:12 ` Peter Xu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87mt2ueaph.fsf@secure.mitica \
--to=quintela@redhat.com \
--cc=lsoaresp@redhat.com \
--cc=peterx@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).