[Qemu-devel] [PATCH RFC 0/1] vmstate: fix the failed iotests case 68 and 91

[Qemu-devel] [PATCH RFC 0/1] vmstate: fix the failed iotests case 68 and 91

[QingFeng Hao]

Hi All,
I am not sure if the fix is correct because I am not very clear about the
logic in vmstate.c. From my test, once size=0, the iotests case 68 failed
due to the assert. So just send this draft patch for your comments!
The patch is based on commit 17783ac828a "Merge remote-tracking branch
'remotes/dgibson/tags/ppc-for-2.9-20170303' into staging".
Thanks!

QingFeng Hao (1):
  vmstate: fix the failure of iotests cases 68 and 91

 migration/vmstate.c | 8 ++++++++
 1 file changed, 8 insertions(+)

-- 
2.8.4

[Qemu-devel] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91

[QingFeng Hao]

I am not very clear about the logic in vmstate.c, but from its context in
vmstate_save_state, it seems size should not be 0, otherwise the followed
for loop will keep working on the same element. So I just add a simple
check to pass that case, not sure if it's right but it can pass iotest
case 68 and 91 now.

The iotest's failed output is:
068 1s ... - output mismatch (see 068.out.bad)
--- /home/haoqf/KVMonz/gitcheck/work/qemu-master/tree/qemu/tests/qemu-iotests/068.out   2017-03-06 05:52:24.817328899 +0100
+++ 068.out.bad 2017-03-07 03:28:44.426714519 +0100
@@ -3,9 +3,13 @@
 === Saving and reloading a VM state to/from a qcow2 image ===

 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
+qemu-system-s390x: migration/vmstate.c:336: vmstate_save_state: Assertion `first_elem || !n_elems' failed.
+./common.config: line 109: 52497 Aborted                 ( if [ -n "${QEMU_NEED_PID}" ]; then
+    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
+fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) savevm 0
-(qemu) quit
+qemu-system-s390x: Device 'virtio0' does not have the requested snapshot '0'
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) quit
 *** done

091 1s ... [failed, exit status 1] - output mismatch (see 091.out.bad)
    --- tests/qemu-iotests/091.out	2016-08-30 12:35:04.207683276 +0200
    +++ 091.out.bad	2017-03-06 13:08:03.717135426 +0100
    @@ -11,18 +11,23 @@
     
     vm1: qemu-io disk write complete
     vm1: live migration started
    -vm1: live migration completed
    -
    -=== VM 2: Post-migration, write to disk, verify running ===
    -
    -vm2: qemu-io disk write complete
    -vm2: qemu process running successfully
    -vm2: flush io, and quit
    -Check image pattern
    -read 4194304/4194304 bytes at offset 0
    -4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
    -Running 'qemu-img check -r all $TEST_IMG'
    -No errors were found on the image.
    -80/16384 = 0.49% allocated, 0.00% fragmented, 0.00% compressed clusters
    -Image end offset: 5570560
    -*** done
    +./common.qemu: line 110: write error: Broken pipe
    +./common.qemu: line 110: write error: Broken pipe
    +./common.qemu: line 110: write error: Broken pipe
    +./common.qemu: line 110: write error: Broken pipe
    +./common.qemu: line 110: write error: Broken pipe
    +./common.qemu: line 110: write error: Broken pipe
    +./common.qemu: line 110: write error: Broken pipe
    +./common.qemu: line 110: write error: Broken pipe
    +./common.qemu: line 110: write error: Broken pipe
    +./common.qemu: line 110: write error: Broken pipe
    +./common.qemu: line 110: write error: Broken pipe
    +./common.qemu: line 110: write error: Broken pipe
    +./common.qemu: line 110: write error: Broken pipe
    +./common.qemu: line 110: write error: Broken pipe
    +./common.qemu: line 110: write error: Broken pipe
    +./common.qemu: line 110: write error: Broken pipe
    +./common.qemu: line 110: write error: Broken pipe
    +./common.qemu: line 110: write error: Broken pipe
    +./common.qemu: line 110: write error: Broken pipe
    +Timeout waiting for completed on handle 0

Signed-off-by: QingFeng Hao <haoqf@linux.vnet.ibm.com>
---
 migration/vmstate.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 78b3cd4..ff28dde 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -106,6 +106,10 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
             int i, n_elems = vmstate_n_elems(opaque, field);
             int size = vmstate_size(opaque, field);
 
+            if (size == 0) {
+                field++;
+                continue;
+            }
             vmstate_handle_alloc(first_elem, field, opaque);
             if (field->flags & VMS_POINTER) {
                 first_elem = *(void **)first_elem;
@@ -322,6 +326,10 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
             int64_t old_offset, written_bytes;
             QJSON *vmdesc_loop = vmdesc;
 
+            if (size == 0) {
+                field++;
+                continue;
+            }
             trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
             if (field->flags & VMS_POINTER) {
                 first_elem = *(void **)first_elem;
-- 
2.8.4

Re: [Qemu-devel] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91

[Kevin Wolf]

Am 07.03.2017 um 03:53 hat QingFeng Hao geschrieben:
> I am not very clear about the logic in vmstate.c, but from its context in
> vmstate_save_state, it seems size should not be 0, otherwise the followed
> for loop will keep working on the same element. So I just add a simple
> check to pass that case, not sure if it's right but it can pass iotest
> case 68 and 91 now.
> 
> The iotest's failed output is:
> 068 1s ... - output mismatch (see 068.out.bad)
> --- /home/haoqf/KVMonz/gitcheck/work/qemu-master/tree/qemu/tests/qemu-iotests/068.out   2017-03-06 05:52:24.817328899 +0100
> +++ 068.out.bad 2017-03-07 03:28:44.426714519 +0100
> @@ -3,9 +3,13 @@
>  === Saving and reloading a VM state to/from a qcow2 image ===
> 
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
> +qemu-system-s390x: migration/vmstate.c:336: vmstate_save_state: Assertion `first_elem || !n_elems' failed.
> +./common.config: line 109: 52497 Aborted                 ( if [ -n "${QEMU_NEED_PID}" ]; then
> +    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
> +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
>  QEMU X.Y.Z monitor - type 'help' for more information
>  (qemu) savevm 0
> -(qemu) quit
> +qemu-system-s390x: Device 'virtio0' does not have the requested snapshot '0'
>  QEMU X.Y.Z monitor - type 'help' for more information
>  (qemu) quit
>  *** done
> 
> 091 1s ... [failed, exit status 1] - output mismatch (see 091.out.bad)
>     --- tests/qemu-iotests/091.out	2016-08-30 12:35:04.207683276 +0200
>     +++ 091.out.bad	2017-03-06 13:08:03.717135426 +0100
>     @@ -11,18 +11,23 @@
>      
>      vm1: qemu-io disk write complete
>      vm1: live migration started
>     -vm1: live migration completed
>     -
>     -=== VM 2: Post-migration, write to disk, verify running ===
>     -
>     -vm2: qemu-io disk write complete
>     -vm2: qemu process running successfully
>     -vm2: flush io, and quit
>     -Check image pattern
>     -read 4194304/4194304 bytes at offset 0
>     -4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>     -Running 'qemu-img check -r all $TEST_IMG'
>     -No errors were found on the image.
>     -80/16384 = 0.49% allocated, 0.00% fragmented, 0.00% compressed clusters
>     -Image end offset: 5570560
>     -*** done
>     +./common.qemu: line 110: write error: Broken pipe
>     +./common.qemu: line 110: write error: Broken pipe
>     +./common.qemu: line 110: write error: Broken pipe
>     +./common.qemu: line 110: write error: Broken pipe
>     +./common.qemu: line 110: write error: Broken pipe
>     +./common.qemu: line 110: write error: Broken pipe
>     +./common.qemu: line 110: write error: Broken pipe
>     +./common.qemu: line 110: write error: Broken pipe
>     +./common.qemu: line 110: write error: Broken pipe
>     +./common.qemu: line 110: write error: Broken pipe
>     +./common.qemu: line 110: write error: Broken pipe
>     +./common.qemu: line 110: write error: Broken pipe
>     +./common.qemu: line 110: write error: Broken pipe
>     +./common.qemu: line 110: write error: Broken pipe
>     +./common.qemu: line 110: write error: Broken pipe
>     +./common.qemu: line 110: write error: Broken pipe
>     +./common.qemu: line 110: write error: Broken pipe
>     +./common.qemu: line 110: write error: Broken pipe
>     +./common.qemu: line 110: write error: Broken pipe
>     +Timeout waiting for completed on handle 0
> 
> Signed-off-by: QingFeng Hao <haoqf@linux.vnet.ibm.com>
> ---
>  migration/vmstate.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 78b3cd4..ff28dde 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -106,6 +106,10 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>              int i, n_elems = vmstate_n_elems(opaque, field);
>              int size = vmstate_size(opaque, field);
>  
> +            if (size == 0) {
> +                field++;
> +                continue;
> +            }
>              vmstate_handle_alloc(first_elem, field, opaque);
>              if (field->flags & VMS_POINTER) {
>                  first_elem = *(void **)first_elem;
> @@ -322,6 +326,10 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>              int64_t old_offset, written_bytes;
>              QJSON *vmdesc_loop = vmdesc;
>  
> +            if (size == 0) {
> +                field++;
> +                continue;
> +            }
>              trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
>              if (field->flags & VMS_POINTER) {
>                  first_elem = *(void **)first_elem;

This is really a live migration fix, so I'm adding Juan and Dave to CC.

I suspect the real question is why a field with size 0 was even stored
in the vmstate to begin with.

Kevin

Re: [Qemu-devel] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91

[Dr. David Alan Gilbert]

* Kevin Wolf (kwolf@redhat.com) wrote:
> Am 07.03.2017 um 03:53 hat QingFeng Hao geschrieben:
> > I am not very clear about the logic in vmstate.c, but from its context in
> > vmstate_save_state, it seems size should not be 0, otherwise the followed
> > for loop will keep working on the same element. So I just add a simple
> > check to pass that case, not sure if it's right but it can pass iotest
> > case 68 and 91 now.
> > 
> > The iotest's failed output is:
> > 068 1s ... - output mismatch (see 068.out.bad)
> > --- /home/haoqf/KVMonz/gitcheck/work/qemu-master/tree/qemu/tests/qemu-iotests/068.out   2017-03-06 05:52:24.817328899 +0100
> > +++ 068.out.bad 2017-03-07 03:28:44.426714519 +0100
> > @@ -3,9 +3,13 @@
> >  === Saving and reloading a VM state to/from a qcow2 image ===
> > 
> >  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
> > +qemu-system-s390x: migration/vmstate.c:336: vmstate_save_state: Assertion `first_elem || !n_elems' failed.
> > +./common.config: line 109: 52497 Aborted                 ( if [ -n "${QEMU_NEED_PID}" ]; then
> > +    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
> > +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
> >  QEMU X.Y.Z monitor - type 'help' for more information
> >  (qemu) savevm 0
> > -(qemu) quit
> > +qemu-system-s390x: Device 'virtio0' does not have the requested snapshot '0'
> >  QEMU X.Y.Z monitor - type 'help' for more information
> >  (qemu) quit
> >  *** done
> > 
> > 091 1s ... [failed, exit status 1] - output mismatch (see 091.out.bad)
> >     --- tests/qemu-iotests/091.out	2016-08-30 12:35:04.207683276 +0200
> >     +++ 091.out.bad	2017-03-06 13:08:03.717135426 +0100
> >     @@ -11,18 +11,23 @@
> >      
> >      vm1: qemu-io disk write complete
> >      vm1: live migration started
> >     -vm1: live migration completed
> >     -
> >     -=== VM 2: Post-migration, write to disk, verify running ===
> >     -
> >     -vm2: qemu-io disk write complete
> >     -vm2: qemu process running successfully
> >     -vm2: flush io, and quit
> >     -Check image pattern
> >     -read 4194304/4194304 bytes at offset 0
> >     -4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> >     -Running 'qemu-img check -r all $TEST_IMG'
> >     -No errors were found on the image.
> >     -80/16384 = 0.49% allocated, 0.00% fragmented, 0.00% compressed clusters
> >     -Image end offset: 5570560
> >     -*** done
> >     +./common.qemu: line 110: write error: Broken pipe
> >     +./common.qemu: line 110: write error: Broken pipe
> >     +./common.qemu: line 110: write error: Broken pipe
> >     +./common.qemu: line 110: write error: Broken pipe
> >     +./common.qemu: line 110: write error: Broken pipe
> >     +./common.qemu: line 110: write error: Broken pipe
> >     +./common.qemu: line 110: write error: Broken pipe
> >     +./common.qemu: line 110: write error: Broken pipe
> >     +./common.qemu: line 110: write error: Broken pipe
> >     +./common.qemu: line 110: write error: Broken pipe
> >     +./common.qemu: line 110: write error: Broken pipe
> >     +./common.qemu: line 110: write error: Broken pipe
> >     +./common.qemu: line 110: write error: Broken pipe
> >     +./common.qemu: line 110: write error: Broken pipe
> >     +./common.qemu: line 110: write error: Broken pipe
> >     +./common.qemu: line 110: write error: Broken pipe
> >     +./common.qemu: line 110: write error: Broken pipe
> >     +./common.qemu: line 110: write error: Broken pipe
> >     +./common.qemu: line 110: write error: Broken pipe
> >     +Timeout waiting for completed on handle 0
> > 
> > Signed-off-by: QingFeng Hao <haoqf@linux.vnet.ibm.com>
> > ---
> >  migration/vmstate.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > index 78b3cd4..ff28dde 100644
> > --- a/migration/vmstate.c
> > +++ b/migration/vmstate.c
> > @@ -106,6 +106,10 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> >              int i, n_elems = vmstate_n_elems(opaque, field);
> >              int size = vmstate_size(opaque, field);
> >  
> > +            if (size == 0) {
> > +                field++;
> > +                continue;
> > +            }
> >              vmstate_handle_alloc(first_elem, field, opaque);
> >              if (field->flags & VMS_POINTER) {
> >                  first_elem = *(void **)first_elem;
> > @@ -322,6 +326,10 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> >              int64_t old_offset, written_bytes;
> >              QJSON *vmdesc_loop = vmdesc;
> >  
> > +            if (size == 0) {
> > +                field++;
> > +                continue;
> > +            }
> >              trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
> >              if (field->flags & VMS_POINTER) {
> >                  first_elem = *(void **)first_elem;
> 
> This is really a live migration fix, so I'm adding Juan and Dave to CC.
> 
> I suspect the real question is why a field with size 0 was even stored
> in the vmstate to begin with.

Yes; which field is it that's failing?

Dave

> Kevin
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91

[Fam Zheng]

On Tue, 03/07 09:41, Dr. David Alan Gilbert wrote:
> > This is really a live migration fix, so I'm adding Juan and Dave to CC.
> > 
> > I suspect the real question is why a field with size 0 was even stored
> > in the vmstate to begin with.
> 
> Yes; which field is it that's failing?

See Qingfeng's other reply in this thread. It is irqstate.

Fam

Re: [Qemu-devel] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91

[Halil Pasic]

On 03/07/2017 10:29 AM, Kevin Wolf wrote:
> Am 07.03.2017 um 03:53 hat QingFeng Hao geschrieben:
>> I am not very clear about the logic in vmstate.c, but from its context in
>> vmstate_save_state, it seems size should not be 0, otherwise the followed
>> for loop will keep working on the same element. So I just add a simple
>> check to pass that case, not sure if it's right but it can pass iotest
>> case 68 and 91 now.
>>
>> The iotest's failed output is:
>> 068 1s ... - output mismatch (see 068.out.bad)
>> --- /home/haoqf/KVMonz/gitcheck/work/qemu-master/tree/qemu/tests/qemu-iotests/068.out   2017-03-06 05:52:24.817328899 +0100
>> +++ 068.out.bad 2017-03-07 03:28:44.426714519 +0100
>> @@ -3,9 +3,13 @@
>>  === Saving and reloading a VM state to/from a qcow2 image ===
>>
>>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
>> +qemu-system-s390x: migration/vmstate.c:336: vmstate_save_state: Assertion `first_elem || !n_elems' failed.
>> +./common.config: line 109: 52497 Aborted                 ( if [ -n "${QEMU_NEED_PID}" ]; then
>> +    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
>> +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
>>  QEMU X.Y.Z monitor - type 'help' for more information
>>  (qemu) savevm 0
>> -(qemu) quit
>> +qemu-system-s390x: Device 'virtio0' does not have the requested snapshot '0'
>>  QEMU X.Y.Z monitor - type 'help' for more information
>>  (qemu) quit
>>  *** done
>>
>> 091 1s ... [failed, exit status 1] - output mismatch (see 091.out.bad)
>>     --- tests/qemu-iotests/091.out	2016-08-30 12:35:04.207683276 +0200
>>     +++ 091.out.bad	2017-03-06 13:08:03.717135426 +0100
>>     @@ -11,18 +11,23 @@
>>      
>>      vm1: qemu-io disk write complete
>>      vm1: live migration started
>>     -vm1: live migration completed
>>     -
>>     -=== VM 2: Post-migration, write to disk, verify running ===
>>     -
>>     -vm2: qemu-io disk write complete
>>     -vm2: qemu process running successfully
>>     -vm2: flush io, and quit
>>     -Check image pattern
>>     -read 4194304/4194304 bytes at offset 0
>>     -4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>     -Running 'qemu-img check -r all $TEST_IMG'
>>     -No errors were found on the image.
>>     -80/16384 = 0.49% allocated, 0.00% fragmented, 0.00% compressed clusters
>>     -Image end offset: 5570560
>>     -*** done
>>     +./common.qemu: line 110: write error: Broken pipe
>>     +./common.qemu: line 110: write error: Broken pipe
>>     +./common.qemu: line 110: write error: Broken pipe
>>     +./common.qemu: line 110: write error: Broken pipe
>>     +./common.qemu: line 110: write error: Broken pipe
>>     +./common.qemu: line 110: write error: Broken pipe
>>     +./common.qemu: line 110: write error: Broken pipe
>>     +./common.qemu: line 110: write error: Broken pipe
>>     +./common.qemu: line 110: write error: Broken pipe
>>     +./common.qemu: line 110: write error: Broken pipe
>>     +./common.qemu: line 110: write error: Broken pipe
>>     +./common.qemu: line 110: write error: Broken pipe
>>     +./common.qemu: line 110: write error: Broken pipe
>>     +./common.qemu: line 110: write error: Broken pipe
>>     +./common.qemu: line 110: write error: Broken pipe
>>     +./common.qemu: line 110: write error: Broken pipe
>>     +./common.qemu: line 110: write error: Broken pipe
>>     +./common.qemu: line 110: write error: Broken pipe
>>     +./common.qemu: line 110: write error: Broken pipe
>>     +Timeout waiting for completed on handle 0
>>
>> Signed-off-by: QingFeng Hao <haoqf@linux.vnet.ibm.com>
>> ---
>>  migration/vmstate.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index 78b3cd4..ff28dde 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -106,6 +106,10 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>              int i, n_elems = vmstate_n_elems(opaque, field);
>>              int size = vmstate_size(opaque, field);
>>  
>> +            if (size == 0) {
>> +                field++;
>> +                continue;
>> +            }
>>              vmstate_handle_alloc(first_elem, field, opaque);
>>              if (field->flags & VMS_POINTER) {
>>                  first_elem = *(void **)first_elem;
>> @@ -322,6 +326,10 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>>              int64_t old_offset, written_bytes;
>>              QJSON *vmdesc_loop = vmdesc;
>>  
>> +            if (size == 0) {
>> +                field++;
>> +                continue;
>> +            }
>>              trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
>>              if (field->flags & VMS_POINTER) {
>>                  first_elem = *(void **)first_elem;
> 
> This is really a live migration fix, so I'm adding Juan and Dave to CC.

You are right, this is migration stuff and has very little to do with
qemu-block.
> 
> I suspect the real question is why a field with size 0 was even stored
> in the vmstate to begin with.
> 

I have looked onto the issue. It affects s390x only if we
are running without KVM. Basically, S390CPU.irqstate is unused
if we do not use KVM, and thus no buffer is allocated.

IMHO this is a missing field and the cleaner way to handle such
missing fields is exist. However this used to work, and I recommended
QuiFeng Hao to discuss the problem upstream.

By the way, I think, if we want to go back to the old behavior
and support VMS_VBUFFER with size 0 and nullptr, a much
cleaner way to do the fix is to change the assert to:

assert(first_elem  || !n_elems || !size)

Obviously the other clean way to fix is to implement exists.

I think the migration maintainers (Juan and Dave) should make a
call regarding if the described usage  of VMS_BUFFER is a legal
or an illegal one.

Regards,
Halil

Re: [Qemu-devel] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91

[Dr. David Alan Gilbert]

* Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
> 
> 
> On 03/07/2017 10:29 AM, Kevin Wolf wrote:
> > Am 07.03.2017 um 03:53 hat QingFeng Hao geschrieben:
> >> I am not very clear about the logic in vmstate.c, but from its context in
> >> vmstate_save_state, it seems size should not be 0, otherwise the followed
> >> for loop will keep working on the same element. So I just add a simple
> >> check to pass that case, not sure if it's right but it can pass iotest
> >> case 68 and 91 now.
> >>
> >> The iotest's failed output is:
> >> 068 1s ... - output mismatch (see 068.out.bad)
> >> --- /home/haoqf/KVMonz/gitcheck/work/qemu-master/tree/qemu/tests/qemu-iotests/068.out   2017-03-06 05:52:24.817328899 +0100
> >> +++ 068.out.bad 2017-03-07 03:28:44.426714519 +0100
> >> @@ -3,9 +3,13 @@
> >>  === Saving and reloading a VM state to/from a qcow2 image ===
> >>
> >>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
> >> +qemu-system-s390x: migration/vmstate.c:336: vmstate_save_state: Assertion `first_elem || !n_elems' failed.
> >> +./common.config: line 109: 52497 Aborted                 ( if [ -n "${QEMU_NEED_PID}" ]; then
> >> +    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
> >> +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
> >>  QEMU X.Y.Z monitor - type 'help' for more information
> >>  (qemu) savevm 0
> >> -(qemu) quit
> >> +qemu-system-s390x: Device 'virtio0' does not have the requested snapshot '0'
> >>  QEMU X.Y.Z monitor - type 'help' for more information
> >>  (qemu) quit
> >>  *** done
> >>
> >> 091 1s ... [failed, exit status 1] - output mismatch (see 091.out.bad)
> >>     --- tests/qemu-iotests/091.out	2016-08-30 12:35:04.207683276 +0200
> >>     +++ 091.out.bad	2017-03-06 13:08:03.717135426 +0100
> >>     @@ -11,18 +11,23 @@
> >>      
> >>      vm1: qemu-io disk write complete
> >>      vm1: live migration started
> >>     -vm1: live migration completed
> >>     -
> >>     -=== VM 2: Post-migration, write to disk, verify running ===
> >>     -
> >>     -vm2: qemu-io disk write complete
> >>     -vm2: qemu process running successfully
> >>     -vm2: flush io, and quit
> >>     -Check image pattern
> >>     -read 4194304/4194304 bytes at offset 0
> >>     -4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> >>     -Running 'qemu-img check -r all $TEST_IMG'
> >>     -No errors were found on the image.
> >>     -80/16384 = 0.49% allocated, 0.00% fragmented, 0.00% compressed clusters
> >>     -Image end offset: 5570560
> >>     -*** done
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +Timeout waiting for completed on handle 0
> >>
> >> Signed-off-by: QingFeng Hao <haoqf@linux.vnet.ibm.com>
> >> ---
> >>  migration/vmstate.c | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >> diff --git a/migration/vmstate.c b/migration/vmstate.c
> >> index 78b3cd4..ff28dde 100644
> >> --- a/migration/vmstate.c
> >> +++ b/migration/vmstate.c
> >> @@ -106,6 +106,10 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> >>              int i, n_elems = vmstate_n_elems(opaque, field);
> >>              int size = vmstate_size(opaque, field);
> >>  
> >> +            if (size == 0) {
> >> +                field++;
> >> +                continue;
> >> +            }
> >>              vmstate_handle_alloc(first_elem, field, opaque);
> >>              if (field->flags & VMS_POINTER) {
> >>                  first_elem = *(void **)first_elem;
> >> @@ -322,6 +326,10 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> >>              int64_t old_offset, written_bytes;
> >>              QJSON *vmdesc_loop = vmdesc;
> >>  
> >> +            if (size == 0) {
> >> +                field++;
> >> +                continue;
> >> +            }
> >>              trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
> >>              if (field->flags & VMS_POINTER) {
> >>                  first_elem = *(void **)first_elem;
> > 
> > This is really a live migration fix, so I'm adding Juan and Dave to CC.
> 
> You are right, this is migration stuff and has very little to do with
> qemu-block.
> > 
> > I suspect the real question is why a field with size 0 was even stored
> > in the vmstate to begin with.
> > 
> 
> I have looked onto the issue. It affects s390x only if we
> are running without KVM. Basically, S390CPU.irqstate is unused
> if we do not use KVM, and thus no buffer is allocated.
> 
> IMHO this is a missing field and the cleaner way to handle such
> missing fields is exist. However this used to work, and I recommended
> QuiFeng Hao to discuss the problem upstream.
> 
> By the way, I think, if we want to go back to the old behavior
> and support VMS_VBUFFER with size 0 and nullptr, a much
> cleaner way to do the fix is to change the assert to:
> 
> assert(first_elem  || !n_elems || !size)
> 
> Obviously the other clean way to fix is to implement exists.
> 
> I think the migration maintainers (Juan and Dave) should make a
> call regarding if the described usage  of VMS_BUFFER is a legal
> or an illegal one.

So is it this code in target/s390x/machine.c ?

        VMSTATE_VBUFFER_UINT32(irqstate, S390CPU, 4, NULL,
                               irqstate_saved_size),

it should be legal for that to be zero length.
It also makes sense that if that's zero length it should be OK for
the pointer to be NULL.

I think it's best if you do change that assert.

Dave


> Regards,
> Halil
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91

[Halil Pasic]

On 03/07/2017 11:03 AM, Dr. David Alan Gilbert wrote:
> * Halil Pasic (pasic@linux.vnet.ibm.com) wrote:
>>
>>
>> On 03/07/2017 10:29 AM, Kevin Wolf wrote:
>>> Am 07.03.2017 um 03:53 hat QingFeng Hao geschrieben:
>>>> I am not very clear about the logic in vmstate.c, but from its context in
>>>> vmstate_save_state, it seems size should not be 0, otherwise the followed
>>>> for loop will keep working on the same element. So I just add a simple
>>>> check to pass that case, not sure if it's right but it can pass iotest
>>>> case 68 and 91 now.

[..]

>>
>> I have looked onto the issue. It affects s390x only if we
>> are running without KVM. Basically, S390CPU.irqstate is unused
>> if we do not use KVM, and thus no buffer is allocated.
>>
>> IMHO this is a missing field and the cleaner way to handle such
>> missing fields is exist. However this used to work, and I recommended
>> QuiFeng Hao to discuss the problem upstream.
>>
>> By the way, I think, if we want to go back to the old behavior
>> and support VMS_VBUFFER with size 0 and nullptr, a much
>> cleaner way to do the fix is to change the assert to:
>>
>> assert(first_elem  || !n_elems || !size)
>>
>> Obviously the other clean way to fix is to implement exists.
>>
>> I think the migration maintainers (Juan and Dave) should make a
>> call regarding if the described usage  of VMS_BUFFER is a legal
>> or an illegal one.
> 
> So is it this code in target/s390x/machine.c ?
> 
>         VMSTATE_VBUFFER_UINT32(irqstate, S390CPU, 4, NULL,
>                                irqstate_saved_size),
> 

Yes!

> it should be legal for that to be zero length.
> It also makes sense that if that's zero length it should be OK for
> the pointer to be NULL.
> 
> I think it's best if you do change that assert.
> 

Makes sense. I think QingFeng will agree too and send a
new version soon.

Regards,
Halil

> Dave

Re: [Qemu-devel] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91

[Kevin Wolf]

Am 07.03.2017 um 10:54 hat Halil Pasic geschrieben:
> 
> 
> On 03/07/2017 10:29 AM, Kevin Wolf wrote:
> > Am 07.03.2017 um 03:53 hat QingFeng Hao geschrieben:
> >> I am not very clear about the logic in vmstate.c, but from its context in
> >> vmstate_save_state, it seems size should not be 0, otherwise the followed
> >> for loop will keep working on the same element. So I just add a simple
> >> check to pass that case, not sure if it's right but it can pass iotest
> >> case 68 and 91 now.
> >>
> >> The iotest's failed output is:
> >> 068 1s ... - output mismatch (see 068.out.bad)
> >> --- /home/haoqf/KVMonz/gitcheck/work/qemu-master/tree/qemu/tests/qemu-iotests/068.out   2017-03-06 05:52:24.817328899 +0100
> >> +++ 068.out.bad 2017-03-07 03:28:44.426714519 +0100
> >> @@ -3,9 +3,13 @@
> >>  === Saving and reloading a VM state to/from a qcow2 image ===
> >>
> >>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
> >> +qemu-system-s390x: migration/vmstate.c:336: vmstate_save_state: Assertion `first_elem || !n_elems' failed.
> >> +./common.config: line 109: 52497 Aborted                 ( if [ -n "${QEMU_NEED_PID}" ]; then
> >> +    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
> >> +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
> >>  QEMU X.Y.Z monitor - type 'help' for more information
> >>  (qemu) savevm 0
> >> -(qemu) quit
> >> +qemu-system-s390x: Device 'virtio0' does not have the requested snapshot '0'
> >>  QEMU X.Y.Z monitor - type 'help' for more information
> >>  (qemu) quit
> >>  *** done
> >>
> >> 091 1s ... [failed, exit status 1] - output mismatch (see 091.out.bad)
> >>     --- tests/qemu-iotests/091.out	2016-08-30 12:35:04.207683276 +0200
> >>     +++ 091.out.bad	2017-03-06 13:08:03.717135426 +0100
> >>     @@ -11,18 +11,23 @@
> >>      
> >>      vm1: qemu-io disk write complete
> >>      vm1: live migration started
> >>     -vm1: live migration completed
> >>     -
> >>     -=== VM 2: Post-migration, write to disk, verify running ===
> >>     -
> >>     -vm2: qemu-io disk write complete
> >>     -vm2: qemu process running successfully
> >>     -vm2: flush io, and quit
> >>     -Check image pattern
> >>     -read 4194304/4194304 bytes at offset 0
> >>     -4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> >>     -Running 'qemu-img check -r all $TEST_IMG'
> >>     -No errors were found on the image.
> >>     -80/16384 = 0.49% allocated, 0.00% fragmented, 0.00% compressed clusters
> >>     -Image end offset: 5570560
> >>     -*** done
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +./common.qemu: line 110: write error: Broken pipe
> >>     +Timeout waiting for completed on handle 0
> >>
> >> Signed-off-by: QingFeng Hao <haoqf@linux.vnet.ibm.com>
> >> ---
> >>  migration/vmstate.c | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >> diff --git a/migration/vmstate.c b/migration/vmstate.c
> >> index 78b3cd4..ff28dde 100644
> >> --- a/migration/vmstate.c
> >> +++ b/migration/vmstate.c
> >> @@ -106,6 +106,10 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> >>              int i, n_elems = vmstate_n_elems(opaque, field);
> >>              int size = vmstate_size(opaque, field);
> >>  
> >> +            if (size == 0) {
> >> +                field++;
> >> +                continue;
> >> +            }
> >>              vmstate_handle_alloc(first_elem, field, opaque);
> >>              if (field->flags & VMS_POINTER) {
> >>                  first_elem = *(void **)first_elem;
> >> @@ -322,6 +326,10 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> >>              int64_t old_offset, written_bytes;
> >>              QJSON *vmdesc_loop = vmdesc;
> >>  
> >> +            if (size == 0) {
> >> +                field++;
> >> +                continue;
> >> +            }
> >>              trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
> >>              if (field->flags & VMS_POINTER) {
> >>                  first_elem = *(void **)first_elem;
> > 
> > This is really a live migration fix, so I'm adding Juan and Dave to CC.
> 
> You are right, this is migration stuff and has very little to do with
> qemu-block.
> > 
> > I suspect the real question is why a field with size 0 was even stored
> > in the vmstate to begin with.
> > 
> 
> I have looked onto the issue. It affects s390x only if we
> are running without KVM. Basically, S390CPU.irqstate is unused
> if we do not use KVM, and thus no buffer is allocated.
> 
> IMHO this is a missing field and the cleaner way to handle such
> missing fields is exist. However this used to work, and I recommended
> QuiFeng Hao to discuss the problem upstream.
> 
> By the way, I think, if we want to go back to the old behavior
> and support VMS_VBUFFER with size 0 and nullptr, a much
> cleaner way to do the fix is to change the assert to:
> 
> assert(first_elem  || !n_elems || !size)
> 
> Obviously the other clean way to fix is to implement exists.

If you're right that this specific vmstate was valid in earlier
versions, then I think it's clear that we need to make it work again.
Otherwise we're breaking migration from old versions.

> I think the migration maintainers (Juan and Dave) should make a
> call regarding if the described usage  of VMS_BUFFER is a legal
> or an illegal one.

Kevin

Re: [Qemu-devel] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91

[Halil Pasic]

On 03/07/2017 11:05 AM, Kevin Wolf wrote:
> Am 07.03.2017 um 10:54 hat Halil Pasic geschrieben:
>>
>>
>> On 03/07/2017 10:29 AM, Kevin Wolf wrote:
>>> Am 07.03.2017 um 03:53 hat QingFeng Hao geschrieben:
>>>> I am not very clear about the logic in vmstate.c, but from its context in
>>>> vmstate_save_state, it seems size should not be 0, otherwise the followed
>>>> for loop will keep working on the same element. So I just add a simple
>>>> check to pass that case, not sure if it's right but it can pass iotest
>>>> case 68 and 91 now.
>>>>
>>>> The iotest's failed output is:
>>>> 068 1s ... - output mismatch (see 068.out.bad)
>>>> --- /home/haoqf/KVMonz/gitcheck/work/qemu-master/tree/qemu/tests/qemu-iotests/068.out   2017-03-06 05:52:24.817328899 +0100
>>>> +++ 068.out.bad 2017-03-07 03:28:44.426714519 +0100
>>>> @@ -3,9 +3,13 @@
>>>>  === Saving and reloading a VM state to/from a qcow2 image ===
>>>>
>>>>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
>>>> +qemu-system-s390x: migration/vmstate.c:336: vmstate_save_state: Assertion `first_elem || !n_elems' failed.
>>>> +./common.config: line 109: 52497 Aborted                 ( if [ -n "${QEMU_NEED_PID}" ]; then
>>>> +    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
>>>> +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
>>>>  QEMU X.Y.Z monitor - type 'help' for more information
>>>>  (qemu) savevm 0
>>>> -(qemu) quit
>>>> +qemu-system-s390x: Device 'virtio0' does not have the requested snapshot '0'
>>>>  QEMU X.Y.Z monitor - type 'help' for more information
>>>>  (qemu) quit
>>>>  *** done
>>>>
>>>> 091 1s ... [failed, exit status 1] - output mismatch (see 091.out.bad)
>>>>     --- tests/qemu-iotests/091.out	2016-08-30 12:35:04.207683276 +0200
>>>>     +++ 091.out.bad	2017-03-06 13:08:03.717135426 +0100
>>>>     @@ -11,18 +11,23 @@
>>>>      
>>>>      vm1: qemu-io disk write complete
>>>>      vm1: live migration started
>>>>     -vm1: live migration completed
>>>>     -
>>>>     -=== VM 2: Post-migration, write to disk, verify running ===
>>>>     -
>>>>     -vm2: qemu-io disk write complete
>>>>     -vm2: qemu process running successfully
>>>>     -vm2: flush io, and quit
>>>>     -Check image pattern
>>>>     -read 4194304/4194304 bytes at offset 0
>>>>     -4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>>>     -Running 'qemu-img check -r all $TEST_IMG'
>>>>     -No errors were found on the image.
>>>>     -80/16384 = 0.49% allocated, 0.00% fragmented, 0.00% compressed clusters
>>>>     -Image end offset: 5570560
>>>>     -*** done
>>>>     +./common.qemu: line 110: write error: Broken pipe
>>>>     +./common.qemu: line 110: write error: Broken pipe
>>>>     +./common.qemu: line 110: write error: Broken pipe
>>>>     +./common.qemu: line 110: write error: Broken pipe
>>>>     +./common.qemu: line 110: write error: Broken pipe
>>>>     +./common.qemu: line 110: write error: Broken pipe
>>>>     +./common.qemu: line 110: write error: Broken pipe
>>>>     +./common.qemu: line 110: write error: Broken pipe
>>>>     +./common.qemu: line 110: write error: Broken pipe
>>>>     +./common.qemu: line 110: write error: Broken pipe
>>>>     +./common.qemu: line 110: write error: Broken pipe
>>>>     +./common.qemu: line 110: write error: Broken pipe
>>>>     +./common.qemu: line 110: write error: Broken pipe
>>>>     +./common.qemu: line 110: write error: Broken pipe
>>>>     +./common.qemu: line 110: write error: Broken pipe
>>>>     +./common.qemu: line 110: write error: Broken pipe
>>>>     +./common.qemu: line 110: write error: Broken pipe
>>>>     +./common.qemu: line 110: write error: Broken pipe
>>>>     +./common.qemu: line 110: write error: Broken pipe
>>>>     +Timeout waiting for completed on handle 0
>>>>
>>>> Signed-off-by: QingFeng Hao <haoqf@linux.vnet.ibm.com>
>>>> ---
>>>>  migration/vmstate.c | 8 ++++++++
>>>>  1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>>>> index 78b3cd4..ff28dde 100644
>>>> --- a/migration/vmstate.c
>>>> +++ b/migration/vmstate.c
>>>> @@ -106,6 +106,10 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>>>              int i, n_elems = vmstate_n_elems(opaque, field);
>>>>              int size = vmstate_size(opaque, field);
>>>>  
>>>> +            if (size == 0) {
>>>> +                field++;
>>>> +                continue;
>>>> +            }
>>>>              vmstate_handle_alloc(first_elem, field, opaque);
>>>>              if (field->flags & VMS_POINTER) {
>>>>                  first_elem = *(void **)first_elem;
>>>> @@ -322,6 +326,10 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>>>>              int64_t old_offset, written_bytes;
>>>>              QJSON *vmdesc_loop = vmdesc;
>>>>  
>>>> +            if (size == 0) {
>>>> +                field++;
>>>> +                continue;
>>>> +            }
>>>>              trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
>>>>              if (field->flags & VMS_POINTER) {
>>>>                  first_elem = *(void **)first_elem;
>>>
>>> This is really a live migration fix, so I'm adding Juan and Dave to CC.
>>
>> You are right, this is migration stuff and has very little to do with
>> qemu-block.
>>>
>>> I suspect the real question is why a field with size 0 was even stored
>>> in the vmstate to begin with.
>>>
>>
>> I have looked onto the issue. It affects s390x only if we
>> are running without KVM. Basically, S390CPU.irqstate is unused
>> if we do not use KVM, and thus no buffer is allocated.
>>
>> IMHO this is a missing field and the cleaner way to handle such
>> missing fields is exist. However this used to work, and I recommended
>> QuiFeng Hao to discuss the problem upstream.
>>
>> By the way, I think, if we want to go back to the old behavior
>> and support VMS_VBUFFER with size 0 and nullptr, a much
>> cleaner way to do the fix is to change the assert to:
>>
>> assert(first_elem  || !n_elems || !size)
>>
>> Obviously the other clean way to fix is to implement exists.
> 
> If you're right that this specific vmstate was valid in earlier
> versions, then I think it's clear that we need to make it work again.
> Otherwise we're breaking migration from old versions.

Not really. We would not break migration because nothing was written to
the stream for VMS_VBUFFER of size 0 except the vmdesc which is at the end,
'debug only', and does not affect migration compatibility. 

IMHO it is an API question. I would have said, there is no data, therefore
there is no field if it's from scratch. But with prior history, I agree
with Dave, we should restore old behavior -- which was changed unintentionally
because I made a wrong assumption.

Regards,
Halil

Re: [Qemu-devel] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91

[QingFeng Hao]

在 2017/3/7 18:19, Halil Pasic 写道:
>
> On 03/07/2017 11:05 AM, Kevin Wolf wrote:
>> Am 07.03.2017 um 10:54 hat Halil Pasic geschrieben:
>>>
>>> On 03/07/2017 10:29 AM, Kevin Wolf wrote:
>>>> Am 07.03.2017 um 03:53 hat QingFeng Hao geschrieben:
>>>>> I am not very clear about the logic in vmstate.c, but from its context in
>>>>> vmstate_save_state, it seems size should not be 0, otherwise the followed
>>>>> for loop will keep working on the same element. So I just add a simple
>>>>> check to pass that case, not sure if it's right but it can pass iotest
>>>>> case 68 and 91 now.
>>>>>
>>>>> The iotest's failed output is:
>>>>> 068 1s ... - output mismatch (see 068.out.bad)
>>>>> --- /home/haoqf/KVMonz/gitcheck/work/qemu-master/tree/qemu/tests/qemu-iotests/068.out   2017-03-06 05:52:24.817328899 +0100
>>>>> +++ 068.out.bad 2017-03-07 03:28:44.426714519 +0100
>>>>> @@ -3,9 +3,13 @@
>>>>>   === Saving and reloading a VM state to/from a qcow2 image ===
>>>>>
>>>>>   Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
>>>>> +qemu-system-s390x: migration/vmstate.c:336: vmstate_save_state: Assertion `first_elem || !n_elems' failed.
>>>>> +./common.config: line 109: 52497 Aborted                 ( if [ -n "${QEMU_NEED_PID}" ]; then
>>>>> +    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
>>>>> +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
>>>>>
>>>>>
>>>>> Signed-off-by: QingFeng Hao <haoqf@linux.vnet.ibm.com>
>>>>> ---
>>>>>   migration/vmstate.c | 8 ++++++++
>>>>>   1 file changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>>>>> index 78b3cd4..ff28dde 100644
>>>>> --- a/migration/vmstate.c
>>>>> +++ b/migration/vmstate.c
>>>>> @@ -106,6 +106,10 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>>>>               int i, n_elems = vmstate_n_elems(opaque, field);
>>>>>               int size = vmstate_size(opaque, field);
>>>>>   
>>>>> +            if (size == 0) {
>>>>> +                field++;
>>>>> +                continue;
>>>>> +            }
>>>>>               vmstate_handle_alloc(first_elem, field, opaque);
>>>>>               if (field->flags & VMS_POINTER) {
>>>>>                   first_elem = *(void **)first_elem;
>>>>> @@ -322,6 +326,10 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>>>>>               int64_t old_offset, written_bytes;
>>>>>               QJSON *vmdesc_loop = vmdesc;
>>>>>   
>>>>> +            if (size == 0) {
>>>>> +                field++;
>>>>> +                continue;
>>>>> +            }
>>>>>               trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
>>>>>               if (field->flags & VMS_POINTER) {
>>>>>                   first_elem = *(void **)first_elem;
>>>> This is really a live migration fix, so I'm adding Juan and Dave to CC.
>>> You are right, this is migration stuff and has very little to do with
>>> qemu-block.
>>>> I suspect the real question is why a field with size 0 was even stored
>>>> in the vmstate to begin with.
>>>>
>>> I have looked onto the issue. It affects s390x only if we
>>> are running without KVM. Basically, S390CPU.irqstate is unused
>>> if we do not use KVM, and thus no buffer is allocated.
>>>
>>> IMHO this is a missing field and the cleaner way to handle such
>>> missing fields is exist. However this used to work, and I recommended
>>> QuiFeng Hao to discuss the problem upstream.
>>>
>>> By the way, I think, if we want to go back to the old behavior
>>> and support VMS_VBUFFER with size 0 and nullptr, a much
>>> cleaner way to do the fix is to change the assert to:
>>>
>>> assert(first_elem  || !n_elems || !size)
>>>
>>> Obviously the other clean way to fix is to implement exists.
>> If you're right that this specific vmstate was valid in earlier
>> versions, then I think it's clear that we need to make it work again.
>> Otherwise we're breaking migration from old versions.
> Not really. We would not break migration because nothing was written to
> the stream for VMS_VBUFFER of size 0 except the vmdesc which is at the end,
> 'debug only', and does not affect migration compatibility.
>
> IMHO it is an API question. I would have said, there is no data, therefore
> there is no field if it's from scratch. But with prior history, I agree
> with Dave, we should restore old behavior -- which was changed unintentionally
> because I made a wrong assumption.
Halil,
Unfortunately, another assert failed after I change the code as below in
vmstate_save_state and vmstate_load_state:
assert(first_elem  || !n_elems || !size);

The error is:
+qemu-system-s390x: migration/vmstate.c:341: vmstate_save_state: 
Assertion `field->flags & VMS_ARRAY_OF_POINTER' failed.
It's the code as below:
  if (!curr_elem) {
                     /* if null pointer write placeholder and do not 
follow      */
                   assert(field->flags & VMS_ARRAY_OF_POINTER);
After debug, I found that the assert failure was still of irqstate and 
its field flags is 0x102(VMS_VBUFFER | VMS_POINTER),
while VMS_ARRAY_OF_POINTER is 0x040. The flags's value matches Dave's 
former assumption
on machine.c although machine.c wasn't compiled, which also confuses me.

Then I commented out that assert(field->flags & VMS_ARRAY_OF_POINTER) 
and the case 68 & 91
can both work!

The question is: can we do that change and what the second assert of 
field's flags is used for?
Thanks!
> Regards,
> Halil

-- 
Regards
QingFeng Hao

Re: [Qemu-devel] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91

[Halil Pasic]

On 03/08/2017 08:05 AM, QingFeng Hao wrote:
> 
> 
> 在 2017/3/7 18:19, Halil Pasic 写道:
>>
>> On 03/07/2017 11:05 AM, Kevin Wolf wrote:
>>> Am 07.03.2017 um 10:54 hat Halil Pasic geschrieben:
>>>>
>>>> On 03/07/2017 10:29 AM, Kevin Wolf wrote:
>>>>> Am 07.03.2017 um 03:53 hat QingFeng Hao geschrieben:
>>>>>> I am not very clear about the logic in vmstate.c, but from its context in
>>>>>> vmstate_save_state, it seems size should not be 0, otherwise the followed
>>>>>> for loop will keep working on the same element. So I just add a simple
>>>>>> check to pass that case, not sure if it's right but it can pass iotest
>>>>>> case 68 and 91 now.
>>>>>>
>>>>>> The iotest's failed output is:
>>>>>> 068 1s ... - output mismatch (see 068.out.bad)
>>>>>> --- /home/haoqf/KVMonz/gitcheck/work/qemu-master/tree/qemu/tests/qemu-iotests/068.out   2017-03-06 05:52:24.817328899 +0100
>>>>>> +++ 068.out.bad 2017-03-07 03:28:44.426714519 +0100
>>>>>> @@ -3,9 +3,13 @@
>>>>>>   === Saving and reloading a VM state to/from a qcow2 image ===
>>>>>>
>>>>>>   Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
>>>>>> +qemu-system-s390x: migration/vmstate.c:336: vmstate_save_state: Assertion `first_elem || !n_elems' failed.
>>>>>> +./common.config: line 109: 52497 Aborted                 ( if [ -n "${QEMU_NEED_PID}" ]; then
>>>>>> +    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
>>>>>> +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
>>>>>>
>>>>>>
>>>>>> Signed-off-by: QingFeng Hao <haoqf@linux.vnet.ibm.com>
>>>>>> ---
>>>>>>   migration/vmstate.c | 8 ++++++++
>>>>>>   1 file changed, 8 insertions(+)
>>>>>>
>>>>>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>>>>>> index 78b3cd4..ff28dde 100644
>>>>>> --- a/migration/vmstate.c
>>>>>> +++ b/migration/vmstate.c
>>>>>> @@ -106,6 +106,10 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>>>>>               int i, n_elems = vmstate_n_elems(opaque, field);
>>>>>>               int size = vmstate_size(opaque, field);
>>>>>>   +            if (size == 0) {
>>>>>> +                field++;
>>>>>> +                continue;
>>>>>> +            }
>>>>>>               vmstate_handle_alloc(first_elem, field, opaque);
>>>>>>               if (field->flags & VMS_POINTER) {
>>>>>>                   first_elem = *(void **)first_elem;
>>>>>> @@ -322,6 +326,10 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>>>>>>               int64_t old_offset, written_bytes;
>>>>>>               QJSON *vmdesc_loop = vmdesc;
>>>>>>   +            if (size == 0) {
>>>>>> +                field++;
>>>>>> +                continue;
>>>>>> +            }
>>>>>>               trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
>>>>>>               if (field->flags & VMS_POINTER) {
>>>>>>                   first_elem = *(void **)first_elem;
>>>>> This is really a live migration fix, so I'm adding Juan and Dave to CC.
>>>> You are right, this is migration stuff and has very little to do with
>>>> qemu-block.
>>>>> I suspect the real question is why a field with size 0 was even stored
>>>>> in the vmstate to begin with.
>>>>>
>>>> I have looked onto the issue. It affects s390x only if we
>>>> are running without KVM. Basically, S390CPU.irqstate is unused
>>>> if we do not use KVM, and thus no buffer is allocated.
>>>>
>>>> IMHO this is a missing field and the cleaner way to handle such
>>>> missing fields is exist. However this used to work, and I recommended
>>>> QuiFeng Hao to discuss the problem upstream.
>>>>
>>>> By the way, I think, if we want to go back to the old behavior
>>>> and support VMS_VBUFFER with size 0 and nullptr, a much
>>>> cleaner way to do the fix is to change the assert to:
>>>>
>>>> assert(first_elem  || !n_elems || !size)
>>>>
>>>> Obviously the other clean way to fix is to implement exists.
>>> If you're right that this specific vmstate was valid in earlier
>>> versions, then I think it's clear that we need to make it work again.
>>> Otherwise we're breaking migration from old versions.
>> Not really. We would not break migration because nothing was written to
>> the stream for VMS_VBUFFER of size 0 except the vmdesc which is at the end,
>> 'debug only', and does not affect migration compatibility.
>>
>> IMHO it is an API question. I would have said, there is no data, therefore
>> there is no field if it's from scratch. But with prior history, I agree
>> with Dave, we should restore old behavior -- which was changed unintentionally
>> because I made a wrong assumption.
> Halil,
> Unfortunately, another assert failed after I change the code as below in
> vmstate_save_state and vmstate_load_state:
> assert(first_elem  || !n_elems || !size);
> 
> The error is:
> +qemu-system-s390x: migration/vmstate.c:341: vmstate_save_state: Assertion `field->flags & VMS_ARRAY_OF_POINTER' failed.
> It's the code as below:
>  if (!curr_elem) {

Sorry, I failed to mention this yesterday. You should also do

-                 if (!curr_elem) {
+                 if (!curr_elem && size) {




>                     /* if null pointer write placeholder and do not follow      */
>                   assert(field->flags & VMS_ARRAY_OF_POINTER);
> After debug, I found that the assert failure was still of irqstate and its field flags is 0x102(VMS_VBUFFER | VMS_POINTER),
> while VMS_ARRAY_OF_POINTER is 0x040. The flags's value matches Dave's former assumption
> on machine.c although machine.c wasn't compiled, which also confuses me.
> 
> Then I commented out that assert(field->flags & VMS_ARRAY_OF_POINTER) and the case 68 & 91
> can both work!

Yeah but that would break migration compatibility for write by 
writing a nullptr-marker character into the stream.

> 
> The question is: can we do that change and what the second assert of field's flags is used for?

The assert is there to guard against unintended use of this nullptr-marker
mechanism.

I have tested so this should work. By the way a vmstate test covering this
corner-case would be a good idea too (IMHO). Would you like to write one?

Regards,
Halil

> Thanks!
>> Regards,
>> Halil
> 

Re: [Qemu-devel] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91

[QingFeng Hao]

在 2017/3/8 19:33, Halil Pasic 写道:
>
> On 03/08/2017 08:05 AM, QingFeng Hao wrote:
>>
>> 在 2017/3/7 18:19, Halil Pasic 写道:
>>> On 03/07/2017 11:05 AM, Kevin Wolf wrote:
>>>> Am 07.03.2017 um 10:54 hat Halil Pasic geschrieben:
>>>>> On 03/07/2017 10:29 AM, Kevin Wolf wrote:
>>>>>> Am 07.03.2017 um 03:53 hat QingFeng Hao geschrieben:
>>>>>>> I am not very clear about the logic in vmstate.c, but from its context in
>>>>>>> vmstate_save_state, it seems size should not be 0, otherwise the followed
>>>>>>> for loop will keep working on the same element. So I just add a simple
>>>>>>> check to pass that case, not sure if it's right but it can pass iotest
>>>>>>> case 68 and 91 now.
>>>>>>>
>>>>>>> The iotest's failed output is:
>>>>>>> 068 1s ... - output mismatch (see 068.out.bad)
>>>>>>> --- /home/haoqf/KVMonz/gitcheck/work/qemu-master/tree/qemu/tests/qemu-iotests/068.out   2017-03-06 05:52:24.817328899 +0100
>>>>>>> +++ 068.out.bad 2017-03-07 03:28:44.426714519 +0100
>>>>>>> @@ -3,9 +3,13 @@
>>>>>>>    === Saving and reloading a VM state to/from a qcow2 image ===
>>>>>>>
>>>>>>>    Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
>>>>>>> +qemu-system-s390x: migration/vmstate.c:336: vmstate_save_state: Assertion `first_elem || !n_elems' failed.
>>>>>>> +./common.config: line 109: 52497 Aborted                 ( if [ -n "${QEMU_NEED_PID}" ]; then
>>>>>>> +    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
>>>>>>> +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
>>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: QingFeng Hao <haoqf@linux.vnet.ibm.com>
>>>>>>> ---
>>>>>>>    migration/vmstate.c | 8 ++++++++
>>>>>>>    1 file changed, 8 insertions(+)
>>>>>>>
>>>>>>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>>>>>>> index 78b3cd4..ff28dde 100644
>>>>>>> --- a/migration/vmstate.c
>>>>>>> +++ b/migration/vmstate.c
>>>>>>> @@ -106,6 +106,10 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>>>>>>                int i, n_elems = vmstate_n_elems(opaque, field);
>>>>>>>                int size = vmstate_size(opaque, field);
>>>>>>>    +            if (size == 0) {
>>>>>>> +                field++;
>>>>>>> +                continue;
>>>>>>> +            }
>>>>>>>                vmstate_handle_alloc(first_elem, field, opaque);
>>>>>>>                if (field->flags & VMS_POINTER) {
>>>>>>>                    first_elem = *(void **)first_elem;
>>>>>>> @@ -322,6 +326,10 @@ void vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>>>>>>>                int64_t old_offset, written_bytes;
>>>>>>>                QJSON *vmdesc_loop = vmdesc;
>>>>>>>    +            if (size == 0) {
>>>>>>> +                field++;
>>>>>>> +                continue;
>>>>>>> +            }
>>>>>>>                trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
>>>>>>>                if (field->flags & VMS_POINTER) {
>>>>>>>                    first_elem = *(void **)first_elem;
>>>>>> This is really a live migration fix, so I'm adding Juan and Dave to CC.
>>>>> You are right, this is migration stuff and has very little to do with
>>>>> qemu-block.
>>>>>> I suspect the real question is why a field with size 0 was even stored
>>>>>> in the vmstate to begin with.
>>>>>>
>>>>> I have looked onto the issue. It affects s390x only if we
>>>>> are running without KVM. Basically, S390CPU.irqstate is unused
>>>>> if we do not use KVM, and thus no buffer is allocated.
>>>>>
>>>>> IMHO this is a missing field and the cleaner way to handle such
>>>>> missing fields is exist. However this used to work, and I recommended
>>>>> QuiFeng Hao to discuss the problem upstream.
>>>>>
>>>>> By the way, I think, if we want to go back to the old behavior
>>>>> and support VMS_VBUFFER with size 0 and nullptr, a much
>>>>> cleaner way to do the fix is to change the assert to:
>>>>>
>>>>> assert(first_elem  || !n_elems || !size)
>>>>>
>>>>> Obviously the other clean way to fix is to implement exists.
>>>> If you're right that this specific vmstate was valid in earlier
>>>> versions, then I think it's clear that we need to make it work again.
>>>> Otherwise we're breaking migration from old versions.
>>> Not really. We would not break migration because nothing was written to
>>> the stream for VMS_VBUFFER of size 0 except the vmdesc which is at the end,
>>> 'debug only', and does not affect migration compatibility.
>>>
>>> IMHO it is an API question. I would have said, there is no data, therefore
>>> there is no field if it's from scratch. But with prior history, I agree
>>> with Dave, we should restore old behavior -- which was changed unintentionally
>>> because I made a wrong assumption.
>> Halil,
>> Unfortunately, another assert failed after I change the code as below in
>> vmstate_save_state and vmstate_load_state:
>> assert(first_elem  || !n_elems || !size);
>>
>> The error is:
>> +qemu-system-s390x: migration/vmstate.c:341: vmstate_save_state: Assertion `field->flags & VMS_ARRAY_OF_POINTER' failed.
>> It's the code as below:
>>   if (!curr_elem) {
> Sorry, I failed to mention this yesterday. You should also do
>
> -                 if (!curr_elem) {
> +                 if (!curr_elem && size) {
>
yes, this works per my test!
>
>
>>                      /* if null pointer write placeholder and do not follow      */
>>                    assert(field->flags & VMS_ARRAY_OF_POINTER);
>> After debug, I found that the assert failure was still of irqstate and its field flags is 0x102(VMS_VBUFFER | VMS_POINTER),
>> while VMS_ARRAY_OF_POINTER is 0x040. The flags's value matches Dave's former assumption
>> on machine.c although machine.c wasn't compiled, which also confuses me.
>>
>> Then I commented out that assert(field->flags & VMS_ARRAY_OF_POINTER) and the case 68 & 91
>> can both work!
> Yeah but that would break migration compatibility for write by
> writing a nullptr-marker character into the stream.
>
>> The question is: can we do that change and what the second assert of field's flags is used for?
> The assert is there to guard against unintended use of this nullptr-marker
> mechanism.
>
> I have tested so this should work. By the way a vmstate test covering this
> corner-case would be a good idea too (IMHO). Would you like to write one?
Sorry, I don't know how to write that test case. Can you please write one?
thanks!
> Regards,
> Halil
>
>> Thanks!
>>> Regards,
>>> Halil

-- 
Regards
QingFeng Hao

Re: [Qemu-devel] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91

[Halil Pasic]

On 03/09/2017 03:55 AM, QingFeng Hao wrote:
> 
> 
> 在 2017/3/8 19:33, Halil Pasic 写道:
>>
>> On 03/08/2017 08:05 AM, QingFeng Hao wrote:
>>>
>>> 在 2017/3/7 18:19, Halil Pasic 写道:
>>>> On 03/07/2017 11:05 AM, Kevin Wolf wrote:
>>>>> Am 07.03.2017 um 10:54 hat Halil Pasic geschrieben:
>>>>>> On 03/07/2017 10:29 AM, Kevin Wolf wrote:
>>>>>>> Am 07.03.2017 um 03:53 hat QingFeng Hao geschrieben:
[..]
>>> The question is: can we do that change and what the second assert of field's flags is used for?
>> The assert is there to guard against unintended use of this nullptr-marker
>> mechanism.
>>
>> I have tested so this should work. By the way a vmstate test covering this
>> corner-case would be a good idea too (IMHO). Would you like to write one?
> Sorry, I don't know how to write that test case. Can you please write one?
> thanks!

The test is just nice to have, but we definitely need the fix!

Halil

Re: [Qemu-devel] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91

[QingFeng Hao]

在 2017/3/9 19:45, Halil Pasic 写道:
>
> On 03/09/2017 03:55 AM, QingFeng Hao wrote:
>>
>> 在 2017/3/8 19:33, Halil Pasic 写道:
>>> On 03/08/2017 08:05 AM, QingFeng Hao wrote:
>>>> 在 2017/3/7 18:19, Halil Pasic 写道:
>>>>> On 03/07/2017 11:05 AM, Kevin Wolf wrote:
>>>>>> Am 07.03.2017 um 10:54 hat Halil Pasic geschrieben:
>>>>>>> On 03/07/2017 10:29 AM, Kevin Wolf wrote:
>>>>>>>> Am 07.03.2017 um 03:53 hat QingFeng Hao geschrieben:
> [..]
>>>> The question is: can we do that change and what the second assert of field's flags is used for?
>>> The assert is there to guard against unintended use of this nullptr-marker
>>> mechanism.
>>>
>>> I have tested so this should work. By the way a vmstate test covering this
>>> corner-case would be a good idea too (IMHO). Would you like to write one?
>> Sorry, I don't know how to write that test case. Can you please write one?
>> thanks!
> The test is just nice to have, but we definitely need the fix!
Ok, I'll send out the discussed fix patch.
>
> Halil

-- 
Regards
QingFeng Hao

Re: [Qemu-devel] [PATCH RFC 0/1] vmstate: fix the failed iotests case 68 and 91

[Fam Zheng]

On Tue, 03/07 03:53, QingFeng Hao wrote:
> Hi All,
> I am not sure if the fix is correct because I am not very clear about the
> logic in vmstate.c. From my test, once size=0, the iotests case 68 failed
> due to the assert. So just send this draft patch for your comments!
> The patch is based on commit 17783ac828a "Merge remote-tracking branch
> 'remotes/dgibson/tags/ppc-for-2.9-20170303' into staging".

I cannot reproduce the failure on either 17783ac828a or current head
56b51708e9e. Both passes for me. I wonder where do you get the size=0.

Fam

Re: [Qemu-devel] [PATCH RFC 0/1] vmstate: fix the failed iotests case 68 and 91

[QingFeng Hao]

在 2017/3/7 14:37, Fam Zheng 写道:
> On Tue, 03/07 03:53, QingFeng Hao wrote:
>> Hi All,
>> I am not sure if the fix is correct because I am not very clear about the
>> logic in vmstate.c. From my test, once size=0, the iotests case 68 failed
>> due to the assert. So just send this draft patch for your comments!
>> The patch is based on commit 17783ac828a "Merge remote-tracking branch
>> 'remotes/dgibson/tags/ppc-for-2.9-20170303' into staging".
> I cannot reproduce the failure on either 17783ac828a or current head
> 56b51708e9e. Both passes for me. I wonder where do you get the size=0.
The error happens when running "savevm 0" in case 068. It can be 
manually reproduced
by "./check -qcow2 68" or "./s390x-softmmu/qemu-system-s390x -nodefaults \
-machine accel=qtest -no-shutdown -nographic -monitor stdio -serial none \
-hda /home/mc/gitcheck/work/qemu-master/tree/qemu/tests/t.img.bak", then 
type
"savevm 0".  t.img.bak is the backup image for t.img generated by 068.

I added the print in vmstate_save_state:
    QJSON *vmdesc_loop = vmdesc;
+ error_report("haoqf:%s:opaque:%p, offset:%lx, size:%d, field name:%s, 
vname:%s\n", __FUNCTION__, opaque, field->offset, size, field->name, 
vmsd->name);

And here is the test log:
haoqf:vmstate_save_state:opaque:0x2aa5a5715c0, offset:122e1, size:1, 
field name:env.sigp_order, vname:cpu

haoqf:vmstate_size: field size:4, offset:0

haoqf:vmstate_save_state:opaque:0x2aa5a5715c0, offset:12300, size:4, 
field name:irqstate_saved_size, vname:cpu

haoqf:vmstate_size: field size:0, offset:74496

haoqf:vmstate_size: calculated size:0

haoqf:vmstate_save_state:opaque:0x2aa5a5715c0, offset:122f8, size:0, 
field name:irqstate, vname:cpu

haoqf:vmstate_save_state:firstelem:(nil), elements: 1

qemu-system-s390x: ../migration/vmstate.c:336: vmstate_save_state: 
Assertion `first_elem || !n_elems' failed.
Aborted (core dumped)

I also did the test for x86 with: "./x86_64-softmmu/qemu-system-x86_64 
-nodefaults \
-machine accel=qtest -no-shutdown -nographic -monitor stdio -serial none \
-hda /home/mc/gitcheck/work/qemu-master/tree/qemu/tests/t.img.bak",
and then ran "savevm 0", but it didn't core and the size are all non-zero:
haoqf:vmstate_save calling vmstate_save_state:

haoqf:vmstate_size: field size:4, offset:0

haoqf:vmstate_save_state:opaque:0x2aa13325438, offset:4, size:4, field 
name:size, vname:globalstate

haoqf:vmstate_size: field size:100, offset:0

haoqf:vmstate_save_state:opaque:0x2aa13325438, offset:8, size:100, field 
name:runstate, vname:globalstate

haoqf:vmstate_save:called vmstate_save_state

So probably x86 doesn't have this problem.
> Fam
>

-- 
Regards
QingFeng Hao