The parsing for the S390StorageAttributes section is currently leaving
an unconsumed token that is later interpreted by the generic code as
QEMU_VM_EOF, cutting the parsing short.
The migration will issue a STATTR_FLAG_DONE between iterations, which
the script consumes correctly, but there's a final STATTR_FLAG_EOS at
.save_complete that the script is ignoring. Since the EOS flag is a
u64 0x1ULL and the stream is big endian, on little endian hosts a byte
read from it will be 0x0, the same as QEMU_VM_EOF.
Fixes: 81c2c9dd5d ("tests/qtest/migration-test: Fix analyze-migration.py for s390x")
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
scripts/analyze-migration.py | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/scripts/analyze-migration.py b/scripts/analyze-migration.py
index f2457b1dde..fcda11f31d 100755
--- a/scripts/analyze-migration.py
+++ b/scripts/analyze-migration.py
@@ -65,6 +65,9 @@ def readvar(self, size = None):
def tell(self):
return self.file.tell()
+ def seek(self, a, b):
+ return self.file.seek(a, b)
+
# The VMSD description is at the end of the file, after EOF. Look for
# the last NULL byte, then for the beginning brace of JSON.
def read_migration_debug_json(self):
@@ -272,11 +275,24 @@ def __init__(self, file, version_id, device, section_key):
self.section_key = section_key
def read(self):
+ pos = 0
while True:
addr_flags = self.file.read64()
flags = addr_flags & 0xfff
- if (flags & (self.STATTR_FLAG_DONE | self.STATTR_FLAG_EOS)):
+
+ if flags & self.STATTR_FLAG_DONE:
+ pos = self.file.tell()
+ continue
+ elif flags & self.STATTR_FLAG_EOS:
return
+ else:
+ # No EOS came after DONE, that's OK, but rewind the
+ # stream because this is not our data.
+ if pos:
+ self.file.seek(pos, os.SEEK_SET)
+ return
+ raise Exception("Unknown flags %x", flags)
+
if (flags & self.STATTR_FLAG_ERROR):
raise Exception("Error in migration stream")
count = self.file.read64()
--
2.35.309.01.2025 21:52, Fabiano Rosas wrote:
> The parsing for the S390StorageAttributes section is currently leaving
> an unconsumed token that is later interpreted by the generic code as
> QEMU_VM_EOF, cutting the parsing short.
>
> The migration will issue a STATTR_FLAG_DONE between iterations, which
> the script consumes correctly, but there's a final STATTR_FLAG_EOS at
> .save_complete that the script is ignoring. Since the EOS flag is a
> u64 0x1ULL and the stream is big endian, on little endian hosts a byte
> read from it will be 0x0, the same as QEMU_VM_EOF.
>
> Fixes: 81c2c9dd5d ("tests/qtest/migration-test: Fix analyze-migration.py for s390x")
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
This looks like a qemu-stable material (if not only for tests), is it not?
Thanks,
/mjt
12.01.2025 16:06, Michael Tokarev wrote:
> 09.01.2025 21:52, Fabiano Rosas wrote:
>> The parsing for the S390StorageAttributes section is currently leaving
>> an unconsumed token that is later interpreted by the generic code as
>> QEMU_VM_EOF, cutting the parsing short.
>>
>> The migration will issue a STATTR_FLAG_DONE between iterations, which
>> the script consumes correctly, but there's a final STATTR_FLAG_EOS at
>> .save_complete that the script is ignoring. Since the EOS flag is a
>> u64 0x1ULL and the stream is big endian, on little endian hosts a byte
>> read from it will be 0x0, the same as QEMU_VM_EOF.
>>
>> Fixes: 81c2c9dd5d ("tests/qtest/migration-test: Fix analyze-migration.py for s390x")
>> Reviewed-by: Peter Xu <peterx@redhat.com>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>
> This looks like a qemu-stable material (if not only for tests), is it not?
This one, when applied to 9.2 together with "s390x: Fix CSS migration",
causes s390x-migration-test failure.
First, it goes:
# starting QEMU: exec ./qemu-system-s390x -qtest unix:/tmp/qtest-1137270.sock -qtest-log /dev/null -chardev
socket,path=/tmp/qtest-1137270.qmp,id=char0 -mon chardev=char0,mode=control -display none -audio none -accel kvm -accel tcg -machine
s390-ccw-virtio-9.2, -name target,debug-threads=on -m 128M -serial file:/tmp/migration-test-T987Z2/dest_serial -incoming tcp:127.0.0.1:0 -bios
/tmp/migration-test-T987Z2/bootsect -accel qtest
Traceback (most recent call last):
File "/tmp/q/scripts/analyze-migration.py", line 704, in <module>
dump.read(dump_memory = args.memory)
File "/tmp/q/scripts/analyze-migration.py", line 641, in read
section.read()
File "/tmp/q/scripts/analyze-migration.py", line 477, in read
field['data'] = reader(field, self.file)
^^^^^^^^^^^^^^^^^^^^^^^^
File "/tmp/q/scripts/analyze-migration.py", line 450, in __init__
for field in self.desc['struct']['fields']:
~~~~~~~~~~~~~~~~~~~^^^^^^^^^^
KeyError: 'fields'
# Failed to analyze the migration stream
and finally
**
ERROR:../../build/qemu/9.2/tests/qtest/migration-test.c:4039:main: assertion failed (ret == 0): (1 == 0)
Bail out! ERROR:../../build/qemu/9.2/tests/qtest/migration-test.c:4039:main: assertion failed (ret == 0): (1 == 0)
It doesn't happen when both these patches are applied to 9.1 though -
there, the test succeeds.
Hmm..
Thanks,
/mjt
On 12/01/2025 15.29, Michael Tokarev wrote:
> 12.01.2025 16:06, Michael Tokarev wrote:
>> 09.01.2025 21:52, Fabiano Rosas wrote:
>>> The parsing for the S390StorageAttributes section is currently leaving
>>> an unconsumed token that is later interpreted by the generic code as
>>> QEMU_VM_EOF, cutting the parsing short.
>>>
>>> The migration will issue a STATTR_FLAG_DONE between iterations, which
>>> the script consumes correctly, but there's a final STATTR_FLAG_EOS at
>>> .save_complete that the script is ignoring. Since the EOS flag is a
>>> u64 0x1ULL and the stream is big endian, on little endian hosts a byte
>>> read from it will be 0x0, the same as QEMU_VM_EOF.
>>>
>>> Fixes: 81c2c9dd5d ("tests/qtest/migration-test: Fix analyze-migration.py
>>> for s390x")
>>> Reviewed-by: Peter Xu <peterx@redhat.com>
>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>
>> This looks like a qemu-stable material (if not only for tests), is it not?
>
> This one, when applied to 9.2 together with "s390x: Fix CSS migration",
> causes s390x-migration-test failure.
>
> First, it goes:
>
> # starting QEMU: exec ./qemu-system-s390x -qtest unix:/tmp/
> qtest-1137270.sock -qtest-log /dev/null -chardev socket,path=/tmp/
> qtest-1137270.qmp,id=char0 -mon chardev=char0,mode=control -display none -
> audio none -accel kvm -accel tcg -machine s390-ccw-virtio-9.2, -name
> target,debug-threads=on -m 128M -serial file:/tmp/migration-test-T987Z2/
> dest_serial -incoming tcp:127.0.0.1:0 -bios /tmp/migration-test-T987Z2/
> bootsect -accel qtest
> Traceback (most recent call last):
> File "/tmp/q/scripts/analyze-migration.py", line 704, in <module>
> dump.read(dump_memory = args.memory)
> File "/tmp/q/scripts/analyze-migration.py", line 641, in read
> section.read()
> File "/tmp/q/scripts/analyze-migration.py", line 477, in read
> field['data'] = reader(field, self.file)
> ^^^^^^^^^^^^^^^^^^^^^^^^
> File "/tmp/q/scripts/analyze-migration.py", line 450, in __init__
> for field in self.desc['struct']['fields']:
> ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^
> KeyError: 'fields'
> # Failed to analyze the migration stream
I think you need to backport patch 1/7, too?
Thomas
13.01.2025 09:39, Thomas Huth wrote: > On 12/01/2025 15.29, Michael Tokarev wrote: >> # starting QEMU: exec ./qemu-system-s390x -qtest unix:/tmp/ qtest-1137270.sock -qtest-log /dev/null -chardev socket,path=/tmp/ >> qtest-1137270.qmp,id=char0 -mon chardev=char0,mode=control -display none - audio none -accel kvm -accel tcg -machine s390-ccw-virtio-9.2, -name >> target,debug-threads=on -m 128M -serial file:/tmp/migration-test-T987Z2/ dest_serial -incoming tcp:127.0.0.1:0 -bios /tmp/migration-test-T987Z2/ >> bootsect -accel qtest >> Traceback (most recent call last): >> File "/tmp/q/scripts/analyze-migration.py", line 704, in <module> >> dump.read(dump_memory = args.memory) >> File "/tmp/q/scripts/analyze-migration.py", line 641, in read >> section.read() >> File "/tmp/q/scripts/analyze-migration.py", line 477, in read >> field['data'] = reader(field, self.file) >> ^^^^^^^^^^^^^^^^^^^^^^^^ >> File "/tmp/q/scripts/analyze-migration.py", line 450, in __init__ >> for field in self.desc['struct']['fields']: >> ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^ >> KeyError: 'fields' >> # Failed to analyze the migration stream > > I think you need to backport patch 1/7, too? Picked up: 1/7 migration: Add more error handling to analyze-migration.py 3/7 migration: Fix parsing of s390 stream 7/7 s390x: Fix CSS migration but still getting the same error: https://gitlab.com/mjt0k/qemu/-/jobs/8832218999 (9.2 branch) https://gitlab.com/mjt0k/qemu/-/jobs/8832224338 (9.1 branch) Thanks, /mjt
On 13/01/2025 08.51, Michael Tokarev wrote: > 13.01.2025 09:39, Thomas Huth wrote: >> On 12/01/2025 15.29, Michael Tokarev wrote: > >>> # starting QEMU: exec ./qemu-system-s390x -qtest unix:/tmp/ >>> qtest-1137270.sock -qtest-log /dev/null -chardev socket,path=/tmp/ >>> qtest-1137270.qmp,id=char0 -mon chardev=char0,mode=control -display none >>> - audio none -accel kvm -accel tcg -machine s390-ccw-virtio-9.2, -name >>> target,debug-threads=on -m 128M -serial file:/tmp/migration-test-T987Z2/ >>> dest_serial -incoming tcp:127.0.0.1:0 -bios /tmp/migration-test-T987Z2/ >>> bootsect -accel qtest >>> Traceback (most recent call last): >>> File "/tmp/q/scripts/analyze-migration.py", line 704, in <module> >>> dump.read(dump_memory = args.memory) >>> File "/tmp/q/scripts/analyze-migration.py", line 641, in read >>> section.read() >>> File "/tmp/q/scripts/analyze-migration.py", line 477, in read >>> field['data'] = reader(field, self.file) >>> ^^^^^^^^^^^^^^^^^^^^^^^^ >>> File "/tmp/q/scripts/analyze-migration.py", line 450, in __init__ >>> for field in self.desc['struct']['fields']: >>> ~~~~~~~~~~~~~~~~~~~^^^^^^^^^^ >>> KeyError: 'fields' >>> # Failed to analyze the migration stream >> >> I think you need to backport patch 1/7, too? > > Picked up: > > 1/7 migration: Add more error handling to analyze-migration.py > 3/7 migration: Fix parsing of s390 stream > 7/7 s390x: Fix CSS migration > > but still getting the same error: > > https://gitlab.com/mjt0k/qemu/-/jobs/8832218999 (9.2 branch) > https://gitlab.com/mjt0k/qemu/-/jobs/8832224338 (9.1 branch) Blindly guessing: You need now patch 4/7 and 5/7, too? ... hopefully Fabiano could reply and point you to the exact set of patches that you need... Thomas
13.01.2025 11:19, Thomas Huth wrote: > On 13/01/2025 08.51, Michael Tokarev wrote: >> Picked up: >> >> 1/7 migration: Add more error handling to analyze-migration.py >> 3/7 migration: Fix parsing of s390 stream >> 7/7 s390x: Fix CSS migration >> >> but still getting the same error: >> >> https://gitlab.com/mjt0k/qemu/-/jobs/8832218999 (9.2 branch) >> https://gitlab.com/mjt0k/qemu/-/jobs/8832224338 (9.1 branch) > > Blindly guessing: You need now patch 4/7 and 5/7, too? ... hopefully Fabiano could reply and point you to the exact set of patches that you need... Yes, after picking up ALL 7 out of 7 in this patchset (b/c even the trivial 2/7, "migration: Remove unused argument in vmsd_desc_field_end", is needed or else the subsequent fixes doesn't apply) it is now green finally, eg, https://gitlab.com/mjt0k/qemu/-/jobs/8832849813 Fabiano, what do you think, - should the whole patchset be picked up for 9.2 and 9.1? Thanks, /mjt
Michael Tokarev <mjt@tls.msk.ru> writes: > 13.01.2025 11:19, Thomas Huth wrote: >> On 13/01/2025 08.51, Michael Tokarev wrote: > >>> Picked up: >>> >>> 1/7 migration: Add more error handling to analyze-migration.py >>> 3/7 migration: Fix parsing of s390 stream >>> 7/7 s390x: Fix CSS migration >>> >>> but still getting the same error: >>> >>> https://gitlab.com/mjt0k/qemu/-/jobs/8832218999 (9.2 branch) >>> https://gitlab.com/mjt0k/qemu/-/jobs/8832224338 (9.1 branch) >> >> Blindly guessing: You need now patch 4/7 and 5/7, too? ... hopefully Fabiano could reply and point you to the exact set of patches that you need... > > Yes, after picking up ALL 7 out of 7 in this patchset (b/c even the > trivial 2/7, "migration: Remove unused argument in vmsd_desc_field_end", > is needed or else the subsequent fixes doesn't apply) it is now green > finally, eg, https://gitlab.com/mjt0k/qemu/-/jobs/8832849813 > > Fabiano, what do you think, - should the whole patchset be picked up > for 9.2 and 9.1? Yeah, sorry, I was focused on unbreaking the migration PR and added a bunch of patches without thinking of stable. So the s390x regression (1/7) is 9.1, but 9.0 already had the broken analyze-script.py (3/7) and the broken array compression code (4-6/7). We definitely need 1/7 for 9.1 and 9.2. The rest of the series is "just" to avoid breaking the tests. If you can apply it easily I think that's preferable. Otherwise maybe we could disable the analyze-migration.py test for stable? I can also work on a backport if needed. Let me know what you prefer. > > Thanks, > > /mjt
13.01.2025 16:03, Fabiano Rosas wrote: > Michael Tokarev <mjt@tls.msk.ru> writes: > >> 13.01.2025 11:19, Thomas Huth wrote: >>> On 13/01/2025 08.51, Michael Tokarev wrote: >> >>>> Picked up: >>>> >>>> 1/7 migration: Add more error handling to analyze-migration.py >>>> 3/7 migration: Fix parsing of s390 stream >>>> 7/7 s390x: Fix CSS migration >>>> >>>> but still getting the same error: >>>> >>>> https://gitlab.com/mjt0k/qemu/-/jobs/8832218999 (9.2 branch) >>>> https://gitlab.com/mjt0k/qemu/-/jobs/8832224338 (9.1 branch) >>> >>> Blindly guessing: You need now patch 4/7 and 5/7, too? ... hopefully Fabiano could reply and point you to the exact set of patches that you need... >> >> Yes, after picking up ALL 7 out of 7 in this patchset (b/c even the >> trivial 2/7, "migration: Remove unused argument in vmsd_desc_field_end", >> is needed or else the subsequent fixes doesn't apply) it is now green >> finally, eg, https://gitlab.com/mjt0k/qemu/-/jobs/8832849813 >> >> Fabiano, what do you think, - should the whole patchset be picked up >> for 9.2 and 9.1? > > Yeah, sorry, I was focused on unbreaking the migration PR and added a > bunch of patches without thinking of stable. That's okay, it's already good (and large) enough to think about current master. > So the s390x regression (7/7) is 9.1, but 9.0 already had the broken > analyze-script.py (3/7) and the broken array compression code (4-6/7). > > We definitely need 7/7 for 9.1 and 9.2. The rest of the series is "just" > to avoid breaking the tests. If you can apply it easily I think that's > preferable. Otherwise maybe we could disable the analyze-migration.py > test for stable? I can also work on a backport if needed. Let me know > what you prefer. I applied all 7 to both 9.1 and 9.2. 9.0 is end-of-life at this point, so everything should be ok. Both 9.1 and 9.2 passes the tests with this whole series applied. Thank you! /mjt >> >> Thanks, >> >> /mjt
Fabiano Rosas <farosas@suse.de> writes: > Michael Tokarev <mjt@tls.msk.ru> writes: > >> 13.01.2025 11:19, Thomas Huth wrote: >>> On 13/01/2025 08.51, Michael Tokarev wrote: >> >>>> Picked up: >>>> >>>> 1/7 migration: Add more error handling to analyze-migration.py >>>> 3/7 migration: Fix parsing of s390 stream >>>> 7/7 s390x: Fix CSS migration >>>> >>>> but still getting the same error: >>>> >>>> https://gitlab.com/mjt0k/qemu/-/jobs/8832218999 (9.2 branch) >>>> https://gitlab.com/mjt0k/qemu/-/jobs/8832224338 (9.1 branch) >>> >>> Blindly guessing: You need now patch 4/7 and 5/7, too? ... hopefully Fabiano could reply and point you to the exact set of patches that you need... >> >> Yes, after picking up ALL 7 out of 7 in this patchset (b/c even the >> trivial 2/7, "migration: Remove unused argument in vmsd_desc_field_end", >> is needed or else the subsequent fixes doesn't apply) it is now green >> finally, eg, https://gitlab.com/mjt0k/qemu/-/jobs/8832849813 >> >> Fabiano, what do you think, - should the whole patchset be picked up >> for 9.2 and 9.1? > > Yeah, sorry, I was focused on unbreaking the migration PR and added a > bunch of patches without thinking of stable. > > So the s390x regression (1/7) is 9.1, but 9.0 already had the broken 7/7 > analyze-script.py (3/7) and the broken array compression code (4-6/7). > > We definitely need 1/7 for 9.1 and 9.2. The rest of the series is "just" 7/7 > to avoid breaking the tests. If you can apply it easily I think that's > preferable. Otherwise maybe we could disable the analyze-migration.py > test for stable? I can also work on a backport if needed. Let me know > what you prefer. > >> >> Thanks, >> >> /mjt
© 2016 - 2025 Red Hat, Inc.