| Message ID | 20230425180544.1815888-1-peterx@redhat.com |
|---|---|
| Headers | show |
| Series | vmstate-static-checker: Fix VMS_ARRAY comparisons | expand |
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. 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? Later, Juan.
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 <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.
On Wed, Apr 26, 2023 at 08:54:02PM +0200, Juan Quintela wrote: > 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. I think I see what you meant. :) > 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. Sounds good!