Re: [RFC PATCH v1 02/17] vmstate: Remove vmstate_use_marker_field

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Tue, Mar 24, 2026 at 04:43:17PM -0300, Fabiano Rosas wrote:
>> Bring back is_null, but set use_marker_field based on it. This makes
>> it clear that the compression logic is only because of the null
>> marker. When the marker is not null, it doesn't make sense to
>> compress.
>
> Yeah I see where this came from, I agree the is_null case is at least
> harder to follow now when without that variable being around.
>
> I'll squash this into my commit for now, maybe I'll still try to keep the
> comments somewhere.
>
> Then I'll also hope we can remove the whole JSON blob very soon.  Do you
> plan to look into the "vmsd walker rewrite analyze-migration script" plan a
> bit?  Let me know if you want me to try; it'll not happen immediately, but
> after seeing this I'm more eager to try removing those..
>

I don't think I'll have time to work on it properly. Actually, I have
the feeling this is something that will take some effort. We may need to
make little steps of progress towards it. If you have the opportunity to
work on something, go ahead.

The first step might be to stop emitting the JSON where
appropriate.

> When those are ready, we should be able to remove is_null again.
>

I'm also considering whether we really need the compression logic at
all. If we remove the blob from the stream and make it a separate
command then we could simply just output the entire array of zeroes and
let external tools deal with it. I bet jq has a way to display that data
nicely.

>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/vmstate.c | 36 ++++++++++--------------------------
>>  1 file changed, 10 insertions(+), 26 deletions(-)
>> 
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index 7d7d9c7e18..7edfa3d990 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -240,22 +240,6 @@ static bool vmstate_post_load(const VMStateDescription *vmsd,
>>      return true;
>>  }
>>  
>> -
>> -/*
>> - * If we will use a ptr marker in the stream for a field?  Two use cases:
>> - *
>> - * (1) When used with VMS_ARRAY_OF_POINTER_ALLOW_NULL, it must always be
>> - *     present to imply the population status of the pointer.
>> - *
>> - * (2) When used with normal VMSD array fields, only emit a null ptr marker
>> - *     if the pointer is NULL.
>> - */
>> -static inline bool
>> -vmstate_use_marker_field(void *ptr, int size, bool dynamic_array)
>> -{
>> -    return (!ptr && size) || dynamic_array;
>> -}
>> -
>>  bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
>>                         void *opaque, int version_id, Error **errp)
>>  {
>> @@ -318,8 +302,8 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
>>                      curr_elem = *(void **)curr_elem_p;
>>                  }
>>  
>> -                use_marker_field = vmstate_use_marker_field(curr_elem, size,
>> -                                                            use_dynamic_array);
>> +                use_marker_field = use_dynamic_array || (!curr_elem && size);
>> +
>>                  if (use_marker_field) {
>>                      /* Read the marker instead of VMSD first */
>>                      if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
>> @@ -634,7 +618,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>>              int i, n_elems = vmstate_n_elems(opaque, field);
>>              int size = vmstate_size(opaque, field);
>>              JSONWriter *vmdesc_loop = vmdesc;
>> -            bool use_marker_field_prev = false;
>> +            bool is_null_prev = false;
>>              /*
>>               * When this is enabled, it means we will always push a ptr
>>               * marker first for each element saying if it's populated.
>> @@ -651,7 +635,7 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>>              for (i = 0; i < n_elems; i++) {
>>                  void *curr_elem = first_elem + size * i;
>>                  const VMStateField *inner_field;
>> -                bool use_marker_field;
>> +                bool use_marker_field, is_null;
>>                  int max_elems = n_elems - i;
>>  
>>                  if (field->flags & VMS_ARRAY_OF_POINTER) {
>> @@ -659,8 +643,8 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>>                      curr_elem = *(void **)curr_elem;
>>                  }
>>  
>> -                use_marker_field = vmstate_use_marker_field(curr_elem, size,
>> -                                                            use_dynamic_array);
>> +                is_null = (!curr_elem && size);
>> +                use_marker_field = use_dynamic_array || is_null;
>>  
>>                  if (use_marker_field) {
>>                      inner_field = vmsd_create_ptr_marker_field(field);
>> @@ -681,16 +665,16 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>>                   */
>>                  if (vmdesc && vmsd_can_compress(field) &&
>>                      (field->flags & VMS_ARRAY_OF_POINTER) &&
>> -                    use_marker_field != use_marker_field_prev) {
>> +                    is_null != is_null_prev) {
>>  
>> -                    use_marker_field_prev = use_marker_field;
>> +                    is_null_prev = is_null;
>>                      vmdesc_loop = vmdesc;
>>  
>>                      for (int j = i + 1; j < n_elems; j++) {
>>                          void *elem = *(void **)(first_elem + size * j);
>> -                        bool elem_use_marker_field = !elem && size;
>> +                        bool elem_is_null = !elem && size;
>>  
>> -                        if (use_marker_field != elem_use_marker_field) {
>> +                        if (is_null != elem_is_null) {
>>                              max_elems = j - i;
>>                              break;
>>                          }
>> -- 
>> 2.51.0
>>