qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
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.



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