[RFC PATCH v1 06/17] vmstate: Put array of pointers code together

[Fabiano Rosas <farosas@suse.de>]

In vmstate_save|load_vmsd[_v], move the code handling
VMS_ARRAY_OF_POINTER all to the same block. These functions deal with
single elements, scalar arrays and arrays of pointers, of which the
latter is the most complex code. Group that use-case so it's easier to
reason about.

This allows for a significant design cleanup, which is to stop using
the inner_field variable in the case where there is NO pointer
marker. Now the inner_field is internal to the VMS_ARRAY_OF_POINTER
code.

By adding a save_field boolean, we can reuse the "actual field" write
for the !use_marker case. This brings the benefit of being able to
drop the field->size specialization. Like so:

===
before:
size = vmstate_size();

if (use_marker) {
   inner_field = fake;
} else {
   inner_field = field;
}

vmstate_save_field_with_vmdesc(inner_field, size)

if (use_marker && !curr) {
    vmstate_save_field_with_vmdesc(field, field->size)
}

after:
size = vmstate_size();

if (use_marker) {
   inner_field = fake;
   vmstate_save_field_with_vmdesc(inner_field, 1)
   save_field = !curr;
}

if (save_field) {
    vmstate_save_field_with_vmdesc(field, size)
}

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/vmstate.c | 193 +++++++++++++++++++++-----------------------
 1 file changed, 92 insertions(+), 101 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 7a12245d36..a190c3f63f 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -277,12 +277,6 @@ bool vmstate_load_vmsd(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);
-            /*
-             * When this is enabled, it means we will always push a ptr
-             * marker first for each element saying if it's populated.
-             */
-            bool use_dynamic_array =
-                field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
 
             vmstate_handle_alloc(first_elem, field, opaque);
             if (field->flags & VMS_POINTER) {
@@ -294,36 +288,40 @@ bool vmstate_load_vmsd(QEMUFile *f, const VMStateDescription *vmsd,
                 /* If we will process the load of field? */
                 bool load_field = true;
                 bool ok = true;
-                bool use_marker_field;
                 void *curr_elem_p = first_elem + size * i;
                 void *curr_elem = curr_elem_p;
 
                 if (field->flags & VMS_ARRAY_OF_POINTER) {
+                    bool use_dynamic_array =
+                        field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
+                    bool use_marker_field;
+
                     curr_elem = *(void **)curr_elem_p;
-                }
 
-                use_marker_field = use_dynamic_array || (!curr_elem && size);
+                    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 (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.
-                         */
-                        curr_elem = g_malloc0(field->size);
-                        /* Remember to update the root pointer! */
-                        *(void **)curr_elem_p = curr_elem;
+                        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.
+                             */
+                            curr_elem = g_malloc0(field->size);
+                            /* Remember to update the root pointer! */
+                            *(void **)curr_elem_p = curr_elem;
+                        }
                     }
                 }
 
@@ -625,13 +623,6 @@ static bool vmstate_save_vmsd_v(QEMUFile *f, const VMStateDescription *vmsd,
             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.
-             */
-            bool use_dynamic_array =
-                field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
-
             trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
             if (field->flags & VMS_POINTER) {
                 first_elem = *(void **)first_elem;
@@ -639,83 +630,83 @@ 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 = first_elem + size * i;
-                const VMStateField *inner_field;
-                bool use_marker_field, is_null = false;
                 int max_elems = n_elems - i;
 
                 if (field->flags & VMS_ARRAY_OF_POINTER) {
+                    const VMStateField *inner_field;
+                    bool use_marker_field, is_null, use_dynamic_array;
+
                     assert(curr_elem);
                     curr_elem = *(void **)curr_elem;
+
                     is_null = !curr_elem;
-                }
 
-                use_marker_field = use_dynamic_array || is_null;
-
-                if (use_marker_field) {
-                    inner_field = vmsd_create_ptr_marker_field(field);
-                } else {
-                    inner_field = field;
-                }
-
-                /*
-                 * This logic only matters when dumping VM Desc, and only
-                 * when the VMSD field can be compressed.
-                 *
-                 * Due to the fake nullptr handling above, if there's mixed
-                 * null/non-null data, it doesn't make sense to emit a
-                 * compressed array representation spanning the entire array
-                 * because the field types will be different (e.g. struct
-                 * vs. nullptr). Search ahead for the next null/non-null element
-                 * and start a new compressed array if found.
-                 */
-                if (vmdesc && vmsd_can_compress(field) &&
-                    (field->flags & VMS_ARRAY_OF_POINTER) &&
-                    is_null != is_null_prev) {
-
-                    is_null_prev = is_null;
-                    use_vmdesc = true;
-
-                    for (int j = i + 1; j < n_elems; j++) {
-                        void *elem = *(void **)(first_elem + size * j);
-                        bool elem_is_null = !elem;
-
-                        if (is_null != elem_is_null) {
-                            max_elems = j - i;
-                            break;
-                        }
-                    }
-                }
-
-                if (use_dynamic_array) {
-                    use_vmdesc = true;
-                }
-
-                ok = vmstate_save_field_with_vmdesc(f, curr_elem, size, vmsd,
-                                                    inner_field,
-                                                    use_vmdesc ? vmdesc : NULL,
-                                                    i, max_elems, errp);
-
-                /* If we used a fake temp field.. free it now */
-                if (use_marker_field) {
-                    g_clear_pointer((gpointer *)&inner_field, g_free);
-                }
-
-                if (!ok) {
-                    goto out;
-                }
-
-                /*
-                 * If we're using dynamic array and the element is
-                 * populated, dump the real object right after the marker.
-                 */
-                if (use_dynamic_array && curr_elem) {
                     /*
-                     * NOTE: do not use vmstate_size() here because we want
-                     * to dump the real VMSD object now.
+                     * When this is enabled, it means we will always push a ptr
+                     * marker first for each element saying if it's populated.
                      */
+                    use_dynamic_array =
+                        field->flags & VMS_ARRAY_OF_POINTER_AUTO_ALLOC;
+
+                    use_marker_field = use_dynamic_array || is_null;
+
+                    if (vmdesc && vmsd_can_compress(field)) {
+                        /*
+                         * This logic only matters when dumping VM
+                         * Desc, and only when the VMSD field can be
+                         * compressed.
+                         *
+                         * Due to the fake nullptr handling above, if
+                         * there's mixed null/non-null data, it
+                         * doesn't make sense to emit a compressed
+                         * array representation spanning the entire
+                         * array because the field types will be
+                         * different (e.g. struct vs. nullptr). Search
+                         * ahead for the next null/non-null element
+                         * and start a new compressed array if found.
+                         */
+                        if (is_null != is_null_prev) {
+                            is_null_prev = is_null;
+                            use_vmdesc = true;
+
+                            for (int j = i + 1; j < n_elems; j++) {
+                                void *elem = *(void **)(first_elem + size * j);
+                                bool elem_is_null = !elem;
+
+                                if (is_null != elem_is_null) {
+                                    max_elems = j - i;
+                                    break;
+                                }
+                            }
+                        }
+                    }
+
+                    if (use_dynamic_array) {
+                        use_vmdesc = true;
+                    }
+
+                    if (use_marker_field) {
+                        inner_field = vmsd_create_ptr_marker_field(field);
+
+                        ok = vmstate_save_field_with_vmdesc(
+                            f, curr_elem, 1, vmsd, inner_field,
+                            use_vmdesc ? vmdesc : NULL, i, max_elems, errp);
+
+                        g_clear_pointer((gpointer *)&inner_field, g_free);
+
+                        if (!ok) {
+                            goto out;
+                        }
+
+                        save_field = !!curr_elem;
+                    }
+                }
+
+                if (save_field) {
                     ok = vmstate_save_field_with_vmdesc(
-                        f, curr_elem, field->size, vmsd, field,
+                        f, curr_elem, size, vmsd, field,
                         use_vmdesc ? vmdesc : NULL, i, max_elems, errp);
 
                     if (!ok) {
-- 
2.51.0