On Thu, May 22, 2025 at 08:08:44PM +0200, Thomas Huth wrote:
> On 22/05/2025 16.38, Peter Xu wrote:
> > On Thu, May 22, 2025 at 03:37:55PM +0200, Thomas Huth wrote:
> >
> > [...]
> >
> > > + def test_vmstate(self):
> > > + target_machine = {
> > > + 'aarch64': 'virt-7.2',
> > > + 'm68k': 'virt-7.2',
> > > + 'ppc64': 'pseries-7.2',
> > > + 's390x': 's390-ccw-virtio-7.2',
> > > + 'x86_64': 'pc-q35-7.2',
> > > + }
> > > + self.set_machine(target_machine[self.arch])
> > > +
> > > + # Run QEMU to get the current vmstate json file:
> > > + dst_json = self.scratch_file('dest.json')
> > > + self.log.info('Dumping vmstate from ' + self.qemu_bin)
> > > + cp = subprocess.run([self.qemu_bin, '-nodefaults',
> > > + '-M', target_machine[self.arch],
> > > + '-dump-vmstate', dst_json],
> > > + stdout=subprocess.PIPE,
> > > + stderr=subprocess.STDOUT,
> > > + text=True)
> > > + if cp.returncode != 0:
> > > + self.fail('Running QEMU failed:\n' + cp.stdout)
> > > + if cp.stdout:
> > > + self.log.info('QEMU output: ' + cp.stdout)
> > > +
> > > + # Check whether the old vmstate json file is still compatible:
> > > + src_json = self.data_file('..', 'data', 'vmstate-static-checker',
> > > + self.arch,
> > > + target_machine[self.arch] + '.json')
> > > + self.log.info('Comparing vmstate with ' + src_json)
> > > + cp = self.run_vmstate_checker(src_json, dst_json)
> > > + if cp.returncode != 0:
> > > + self.fail('Running vmstate-static-checker failed:\n' +
> > > cp.stdout)
> >
> > Would false positives happen here? Would it fail "make check" and CI, even
> > if the change was intended?
>
> Yes. In that case, the quick fix is to remove the problematic piece from the
> 7.2 json files. Or we could try to improve the vmstate-static-checker
> script. At least we now notice it immediately, not only after a long delay
> until someone runs the script manually again.
Yes, the thing is I worry it'll almost always be false positives (from
statistical POV.. unfortunately). Then in that case it's actually better
to be found later because otherwise it means we're adding overhead to every
developer who might cause the false positive and each of them doing this
work with no real gain.. :(
>
> But yes, this can be confusing for the who runs into this problem for the
> first time. I guess I should at least add some friendly words here with
> instructions what has to be done?
Some instructions would be helpful for sure. Though do we have easy way to
whitelist any false positives? As this test compares the dumps so there's
no diff to fix or work around.
--
Peter Xu
