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

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> 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.
>

There's 32k bytes in the buffer, this is likely to be just a
dereference. But fair enough.

> 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.
>

Yeah, that's a good point...

If you can put up with passing QEMUFile into vmstate_next(), we could
get the byte only when we're expecting it. Something like:

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 25fd9e52bc..16d9c1753f 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -137,7 +137,7 @@ static void *vmstate_first(void *opaque, const VMStateField *field,
 }
 
 static void *vmstate_next(void **first, const VMStateField *field,
-                          int size, int i, bool alloc)
+                          int size, int i, QEMUFile *f)
 {
     void **array_elem;
     void *next;
@@ -150,8 +150,11 @@ static void *vmstate_next(void **first, const VMStateField *field,
     array_elem = first + i;
     next = *array_elem;
 
-    if (alloc) {
-        if (!next || field->flags & VMS_ALLOC) {
+    if (f && (!next || field->flags & VMS_ALLOC)) {
+        bool alloc, ok;
+
+        ok = vmstate_ptr_marker_load(f, &alloc);
+        if (ok & alloc) {
             /*
              * NOTE: do not use vmstate_size() here, because we
              * need the object size, not entry size of the
@@ -315,18 +318,11 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
                              !(field->flags & VMS_ARRAY_OF_POINTER));
 
         for (i = 0; i < n_elems; i++) {
-            /* If we will process the load of field? */
-            bool load_field = true;
             void *curr_elem;
 
-            if (field->flags & VMS_ARRAY_OF_POINTER) {
-                /* 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);
+            curr_elem = vmstate_next(head, field, size, i, f);
 
-            if (load_field) {
+            if (curr_elem) {
                 ok = vmstate_load_field(f, curr_elem, size, field, errp);
             }
 
@@ -673,7 +669,7 @@ 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 = vmstate_next(head, field, size, i, false);
+            void *curr_elem = vmstate_next(head, field, size, i, NULL);
             int max_elems = n_elems - i;
 
             if (field->flags & VMS_ARRAY_OF_POINTER) {
@@ -711,8 +707,7 @@ 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 = vmstate_next(head, field, size, j,
-                                                      false);
+                            void *elem = vmstate_next(head, field, size, j, NULL);
                             bool elem_is_null = !elem;
 
                             if (is_null != elem_is_null) {
-- 
2.51.0