Re: [PATCH v5 02/23] migration: push Error **errp into vmstate_load_state()

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Jul 17, 2025 at 06:07:25AM +0530, Arun Menon wrote:
> This is an incremental step in converting vmstate loading
> code to report error via Error objects instead of directly
> printing it to console/monitor.
> It is ensured that vmstate_load_state() must report an error
> in errp, in case of failure.
> 
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  hw/display/virtio-gpu.c     |  2 +-
>  hw/pci/pci.c                |  2 +-
>  hw/s390x/virtio-ccw.c       |  2 +-
>  hw/scsi/spapr_vscsi.c       |  2 +-
>  hw/vfio/pci.c               |  2 +-
>  hw/virtio/virtio-mmio.c     |  2 +-
>  hw/virtio/virtio-pci.c      |  2 +-
>  hw/virtio/virtio.c          |  4 ++--
>  include/migration/vmstate.h |  2 +-
>  migration/cpr.c             |  4 ++--
>  migration/savevm.c          |  6 ++++--
>  migration/vmstate-types.c   | 10 +++++-----
>  migration/vmstate.c         | 43 +++++++++++++++++++++++++++----------------
>  tests/unit/test-vmstate.c   | 18 +++++++++---------
>  ui/vdagent.c                |  2 +-
>  15 files changed, 58 insertions(+), 45 deletions(-)
> 

> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 526668a020562f303d2ddf030b1c8466659b67be..078a00003023cc248fb16f05017d4c4251fd86df 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -132,29 +132,30 @@ static void vmstate_handle_alloc(void *ptr, const VMStateField *field,
>  }
>  
>  int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> -                       void *opaque, int version_id)
> +                       void *opaque, int version_id, Error **errp)
>  {
>      const VMStateField *field = vmsd->fields;
>      int ret = 0;
>  
>      trace_vmstate_load_state(vmsd->name, version_id);
>      if (version_id > vmsd->version_id) {
> -        error_report("%s: incoming version_id %d is too new "
> -                     "for local version_id %d",
> -                     vmsd->name, version_id, vmsd->version_id);
> +        error_setg(errp, "%s: incoming version_id %d is too new "
> +                   "for local version_id %d",
> +                   vmsd->name, version_id, vmsd->version_id);
>          trace_vmstate_load_state_end(vmsd->name, "too new", -EINVAL);
>          return -EINVAL;
>      }
>      if  (version_id < vmsd->minimum_version_id) {
> -        error_report("%s: incoming version_id %d is too old "
> -                     "for local minimum version_id  %d",
> -                     vmsd->name, version_id, vmsd->minimum_version_id);
> +        error_setg(errp, "%s: incoming version_id %d is too old "
> +                   "for local minimum version_id  %d",
> +                   vmsd->name, version_id, vmsd->minimum_version_id);
>          trace_vmstate_load_state_end(vmsd->name, "too old", -EINVAL);
>          return -EINVAL;
>      }
>      if (vmsd->pre_load) {
>          ret = vmsd->pre_load(opaque);
>          if (ret) {
> +            error_setg(errp, "VM pre load failed : %d", ret);

Suggest including

  vmsd->name, version_id, vmsd->minimum_version_id

as those are all relevant context which may aid in diagnosing
this error.

>              return ret;
>          }
>      }
> @@ -192,10 +193,12 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>  
>                  if (inner_field->flags & VMS_STRUCT) {
>                      ret = vmstate_load_state(f, inner_field->vmsd, curr_elem,
> -                                             inner_field->vmsd->version_id);
> +                                             inner_field->vmsd->version_id,
> +                                             errp);
>                  } else if (inner_field->flags & VMS_VSTRUCT) {
>                      ret = vmstate_load_state(f, inner_field->vmsd, curr_elem,
> -                                             inner_field->struct_version_id);
> +                                             inner_field->struct_version_id,
> +                                             errp);
>                  } else {
>                      ret = inner_field->info->get(f, curr_elem, size,
>                                                   inner_field);

This should have the "error_setg" call, so...

> @@ -211,27 +214,35 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>                  }
>                  if (ret < 0) {
>                      qemu_file_set_error(f, ret);
> -                    error_report("Failed to load %s:%s", vmsd->name,
> -                                 field->name);
> +                    if (errp != NULL) {
> +                        error_prepend(errp, "Failed to load %s:%s ", vmsd->name,
> +                                      field->name);
> +                    } else {
> +                        error_setg(errp, "Failed to load %s:%s", vmsd->name,
> +                                   field->name);
> +                    }

...this should exclusively do error_prepend.

Also include the 'version_id' here as an bit of extra context.

>                      trace_vmstate_load_field_error(field->name, ret);
>                      return ret;
>                  }
>              }
>          } else if (field->flags & VMS_MUST_EXIST) {
> -            error_report("Input validation failed: %s/%s",
> -                         vmsd->name, field->name);
> +            error_setg(errp, "Input validation failed: %s/%s",
> +                       vmsd->name, field->name);


Inlucde the version_id too

>              return -1;
>          }
>          field++;
>      }
>      assert(field->flags == VMS_END);
> -    ret = vmstate_subsection_load(f, vmsd, opaque, NULL);
> +    ret = vmstate_subsection_load(f, vmsd, opaque, errp);
>      if (ret != 0) {
>          qemu_file_set_error(f, ret);
>          return ret;
>      }
>      if (vmsd->post_load) {
>          ret = vmsd->post_load(opaque, version_id);
> +        if (ret < 0) {
> +            error_setg(errp, "VM Post load failed : %d", ret);
> +        }

Suggest including

  vmsd->name, version_id, vmsd->minimum_version_id


>      }
>      trace_vmstate_load_state_end(vmsd->name, "end", ret);
>      return ret;



With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|