Re: [PATCH v2 07/16] migration: factor out vmstate_save_field() from vmstate_save_state()

[Peter Xu <peterx@redhat.com>]

On Sat, Feb 21, 2026 at 12:02:05AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Simplify vmstate_save_state() which is rather big, and simplify further
> refactoring.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

I wonder how hard it is to have this helper also cover the rest in the loop
over elements, on e.g. VMS_ARRAY_OF_POINTER, vmdesc_loop being able to
change, and all the nullptr handling.

I had a feeling that you intentionally skipped those because it'll not be
as trivial - I think it's fine, but IIUC we can always figure a way out.
Can be done later.  Agree this is an improvement already,

Reviewed-by: Peter Xu <peterx@redhat.com>

> ---
>  migration/vmstate.c | 40 +++++++++++++++++++++++++---------------
>  1 file changed, 25 insertions(+), 15 deletions(-)
> 
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 810b131f18..d8c30830d6 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -26,6 +26,9 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
>                                     Error **errp);
>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>                                     void *opaque, Error **errp);
> +static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
> +                                void *opaque, JSONWriter *vmdesc,
> +                                int version_id, Error **errp);
>  
>  /* Whether this field should exist for either save or load the VM? */
>  static bool
> @@ -450,6 +453,26 @@ static bool vmstate_pre_save(const VMStateDescription *vmsd, void *opaque,
>      return true;
>  }
>  
> +static bool vmstate_save_field(QEMUFile *f, void *pv, size_t size,
> +                               const VMStateField *field,
> +                               JSONWriter *vmdesc, Error **errp)
> +{
> +    if (field->flags & VMS_STRUCT) {
> +        return vmstate_save_state(f, field->vmsd, pv, vmdesc, errp) >= 0;
> +    } else if (field->flags & VMS_VSTRUCT) {
> +        return vmstate_save_state_v(f, field->vmsd, pv, vmdesc,
> +                                    field->struct_version_id,
> +                                    errp) >= 0;
> +    }
> +
> +    if (field->info->put(f, pv, size, field, vmdesc) < 0) {
> +        error_setg(errp, "put failed");
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
>  static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>                                  void *opaque, JSONWriter *vmdesc,
>                                  int version_id, Error **errp)
> @@ -542,21 +565,8 @@ static int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
>                  vmsd_desc_field_start(vmsd, vmdesc_loop, inner_field,
>                                        i, max_elems);
>  
> -                if (inner_field->flags & VMS_STRUCT) {
> -                    ret = vmstate_save_state(f, inner_field->vmsd,
> -                                             curr_elem, vmdesc_loop, errp);
> -                } else if (inner_field->flags & VMS_VSTRUCT) {
> -                    ret = vmstate_save_state_v(f, inner_field->vmsd,
> -                                               curr_elem, vmdesc_loop,
> -                                               inner_field->struct_version_id,
> -                                               errp);
> -                } else {
> -                    ret = inner_field->info->put(f, curr_elem, size,
> -                                                 inner_field, vmdesc_loop);
> -                    if (ret < 0) {
> -                        error_setg(errp, "put failed");
> -                    }
> -                }
> +                ret = vmstate_save_field(f, curr_elem, size, inner_field,
> +                                         vmdesc_loop, errp) ? 0 : -EINVAL;
>  
>                  written_bytes = qemu_file_transferred(f) - old_offset;
>                  vmsd_desc_field_end(vmsd, vmdesc_loop, inner_field,
> -- 
> 2.52.0
> 

-- 
Peter Xu