Re: [RFC PATCH v1 05/17] vmstate: Remove vmdesc_loop

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Mar 24, 2026 at 04:43:20PM -0300, Fabiano Rosas wrote:
>> The vmdesc_loop variable is being used as a hacky way of writing the
>> JSON description only for the first element of a compressed
>> array. There are also a few implicit uses, such as writing the JSON
>> for the first non-compressed element after a compressed portion.
>> 
>> Future work will have trouble with this. Use a simple boolean to be
>> explicit now.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/vmstate.c | 25 ++++++++++++++-----------
>>  1 file changed, 14 insertions(+), 11 deletions(-)
>> 
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index dec9cf920b..7a12245d36 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -622,8 +622,9 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>>              void *first_elem = opaque + field->offset;
>>              int i, n_elems = vmstate_n_elems(opaque, field);
>>              int size = vmstate_size(opaque, field);
>> -            JSONWriter *vmdesc_loop = vmdesc;
>>              bool is_null_prev = false;
>> +            bool use_vmdesc = true;
>> +
>>              /*
>>               * When this is enabled, it means we will always push a ptr
>>               * marker first for each element saying if it's populated.
>> @@ -673,7 +674,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>>                      is_null != is_null_prev) {
>>  
>>                      is_null_prev = is_null;
>> -                    vmdesc_loop = vmdesc;
>> +                    use_vmdesc = true;
>>  
>>                      for (int j = i + 1; j < n_elems; j++) {
>>                          void *elem = *(void **)(first_elem + size * j);
>> @@ -686,8 +687,13 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>>                      }
>>                  }
>>  
>> +                if (use_dynamic_array) {
>> +                    use_vmdesc = true;
>> +                }
>
> [1]
>
>> +
>>                  ok = vmstate_save_field_with_vmdesc(f, curr_elem, size, vmsd,
>> -                                                    inner_field, vmdesc_loop,
>> +                                                    inner_field,
>> +                                                    use_vmdesc ? vmdesc : NULL,
>>                                                      i, max_elems, errp);
>>  
>>                  /* If we used a fake temp field.. free it now */
>> @@ -708,19 +714,16 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>>                       * NOTE: do not use vmstate_size() here because we want
>>                       * to dump the real VMSD object now.
>>                       */
>> -                    ok = vmstate_save_field_with_vmdesc(f, curr_elem,
>> -                                                        field->size, vmsd,
>> -                                                        field, vmdesc_loop,
>> -                                                        i, max_elems, errp);
>> +                    ok = vmstate_save_field_with_vmdesc(
>> +                        f, curr_elem, field->size, vmsd, field,
>> +                        use_vmdesc ? vmdesc : NULL, i, max_elems, errp);
>> +
>>                      if (!ok) {
>>                          goto out;
>>                      }
>>                  }
>>  
>> -                /* Compressed arrays only care about the first element */
>> -                if (vmdesc_loop && vmsd_can_compress(field)) {
>> -                    vmdesc_loop = NULL;
>> -                }
>> +                use_vmdesc = false;
>
> Hmm...
>
> vmsd_can_compress() can return arbitrary things depending on the field
> itself, now we always do the dedup almost for everything.
>
> The patch did add above [1] to make AUTO_ALLOC be ok, by enforce setting
> TRUE for it for every loop, but what about other cases where
> vmsd_can_compress() may return false (where the field does not support
> compression)?
>

Hmm, I think you're right. But what I'm doing by setting
use_vmdesc=false here is just clearing it for the next iteration, where
it should be set according to necessity. So I'd leave this down here,
but up there change to:

         if (vmdesc && vmsd_can_compress(field)) {
             if (is_null != is_null_prev) {
                 use_vmdesc = true;
                 ...
             }
-        }
-
-        if (use_dynamic_array) {
+        } else {
             use_vmdesc = true;
         }

In words: all fields should appear in the JSON, unless its part of the
NULL portion of a compressed array. For compressed arrays only the first
element appears in the JSON. If a stream of NULLs is followed by a
non-NULL member, then that member should appear in the JSON after the
compressed entry.

These two catch all these corner-cases I think:

PYTHON=$(pwd)/pyvenv/bin/python3.11
QTEST_QEMU_BINARY=./qemu-system-s390x ./tests/qtest/migration-test -p
/s390x/migration/analyze-script

PYTHON=$(pwd)/pyvenv/bin/python3.11
QTEST_QEMU_BINARY=./qemu-system-ppc64 ./tests/qtest/migration-test-p
/ppc64/migration/analyze-script

>>              }
>>          } else {
>>              if (field->flags & VMS_MUST_EXIST) {
>> -- 
>> 2.51.0
>>