Re: [RFC PATCH v1 12/17] vmstate: Introduce vmstate_next

[Peter Xu <peterx@redhat.com>]

On Tue, Mar 24, 2026 at 04:43:27PM -0300, Fabiano Rosas wrote:
> Similarly to vmstate_first(), introduce a vmstate_next(), which does
> the necessary dereferencing of pointers to get to the leaf element. On
> the load side, allow the caller to pass a flag indicating whether
> allocation is expected and call the allocation function if so.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/vmstate.c | 84 +++++++++++++++++++++------------------------
>  1 file changed, 39 insertions(+), 45 deletions(-)
> 
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index ab7c6fa4ab..c1ad0ef9a5 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -127,24 +127,51 @@ static void *vmstate_first(void *opaque, const VMStateField *field,
>      return first;
>  }
>  
> -static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field,
> -                                    Error **errp)
> +static void *vmstate_next(void **first, const VMStateField *field,
> +                          int size, int i, bool alloc)
>  {
> -    int byte = qemu_get_byte(f);
> +    void **array_elem;
> +    void *next;
> +
> +    if (!(field->flags & VMS_ARRAY_OF_POINTER)) {
> +        next = (void *)first + size * i;
> +        return next;
> +    }
> +
> +    array_elem = first + i;
> +    next = *array_elem;
> +
> +    if (alloc) {
> +        if (!next || field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC) {
> +            /*
> +             * NOTE: do not use vmstate_size() here, because we
> +             * need the object size, not entry size of the
> +             * array.
> +             */
> +            next = vmstate_handle_alloc(array_elem, field->size, 1);
> +        }
> +    }
> +    return next;
> +}
> +
> +static bool vmstate_ptr_marker_load(QEMUFile *f, bool *load_field)
> +{
> +    int byte = qemu_peek_byte(f, 0);

Here, peeking the marker for ...

>  
>      if (byte == VMS_MARKER_PTR_NULL) {
>          /* When it's a null ptr marker, do not continue the load */
>          *load_field = false;
> +        qemu_file_skip(f, 1);
>          return true;
>      }
>  
>      if (byte == VMS_MARKER_PTR_VALID) {
>          /* We need to load this field right after the marker */
>          *load_field = true;
> +        qemu_file_skip(f, 1);
>          return true;
>      }
>  
> -    error_setg(errp, "Unexpected ptr marker: %d", byte);
>      return false;
>  }
>  
> @@ -273,41 +300,13 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
>                  void *curr_elem;
>  
>                  if (field->flags & VMS_ARRAY_OF_POINTER) {

... every VMS_ARRAY_OF_POINTER case makes me feel uneasy.

Note that we only have two use cases that both of them are certain on the
upcoming byte to come:

(1) for the null-only pointer, which is before this series introduced, both
src/dst qemu knows 100% which ptr is NULL, so dest QEMU expects that 0x30
for each null pointer.

(2) for the new _AUTO_ALLOC introduced here, we also know exactly either
0x30 or 0x31 will come, and one of them must come before the real field
dump.

I think we may shoot us in the foot if we see 0x30 but in reality it's just
the 1st byte of some array field's binary stream.

> -                    void **array_elem = (void **)head + i;
> -                    bool use_dynamic_array =
> -                        field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
> -                    bool use_marker_field;
> -
> -                    curr_elem = *array_elem;
> -                    use_marker_field = use_dynamic_array || !curr_elem;
> -
> -                    if (use_marker_field) {
> -                        /* Read the marker instead of VMSD first */
> -                        if (!vmstate_ptr_marker_load(f, &load_field, errp)) {
> -                            trace_vmstate_load_field_error(field->name,
> -                                                           -EINVAL);
> -                            return false;
> -                        }
> -
> -                        if (load_field) {
> -                            /*
> -                             * When reaching here, it means we received a
> -                             * non-NULL ptr marker, so we need to populate the
> -                             * field before loading it.
> -                             *
> -                             * NOTE: do not use vmstate_size() here, because we
> -                             * need the object size, not entry size of the
> -                             * array.
> -                             */
> -                            assert(!curr_elem);
> -                            curr_elem = vmstate_handle_alloc(array_elem,
> -                                                             field->size, 1);
> -                        }
> -                    }
> -                } else {
> -                    curr_elem = head + size * i;
> +                    /* Peek a possible pointer marker instead of VMSD first */
> +                    ok = vmstate_ptr_marker_load(f, &load_field);
>                  }
>  
> +                curr_elem = vmstate_next(head, field, size, i,
> +                                         load_field && ok);
> +
>                  if (load_field) {
>                      ok = vmstate_load_field(f, curr_elem, size, field, errp);
>                  }
> @@ -647,15 +646,11 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>  
>              for (i = 0; i < n_elems; i++) {
>                  bool save_field = true;
> -                void *curr_elem;
> +                void *curr_elem = vmstate_next(head, field, size, i, false);
>                  int max_elems = n_elems - i;
>  
>                  if (field->flags & VMS_ARRAY_OF_POINTER) {
>                      bool use_marker_field, is_null, use_dynamic_array;
> -                    void **array_elem = (void **)head + i;
> -
> -                    assert(array_elem);
> -                    curr_elem = *array_elem;
>  
>                      is_null = !curr_elem;
>  
> @@ -688,7 +683,8 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>                              use_vmdesc = true;
>  
>                              for (int j = i + 1; j < n_elems; j++) {
> -                                void *elem = *(void **)(head + size * j);
> +                                void *elem = vmstate_next(head, field, size, j,
> +                                                          false);
>                                  bool elem_is_null = !elem;
>  
>                                  if (is_null != elem_is_null) {
> @@ -713,8 +709,6 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
>  
>                          save_field = !!curr_elem;
>                      }
> -                } else {
> -                    curr_elem = head + size * i;
>                  }
>  
>                  if (save_field) {
> -- 
> 2.51.0
> 

-- 
Peter Xu