Re: [PATCH v11 01/27] migration: push Error **errp into vmstate_subsection_load()

Re: [PATCH v11 01/27] migration: push Error **errp into vmstate_subsection_load()

[Fabiano Rosas]

Arun Menon <armenon@redhat.com> writes:

> 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_subsection_load() must report an error
> in errp, in case of failure.
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  migration/vmstate.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 5feaa3244d259874f03048326b2497e7db32e47c..6108c7fe283a5013ce42ea9987723c489aef26a2 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -25,7 +25,7 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
>                                     void *opaque, JSONWriter *vmdesc,
>                                     Error **errp);
>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
> -                                   void *opaque);
> +                                   void *opaque, Error **errp);
>  
>  /* Whether this field should exist for either save or load the VM? */
>  static bool
> @@ -225,7 +225,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>          field++;
>      }
>      assert(field->flags == VMS_END);
> -    ret = vmstate_subsection_load(f, vmsd, opaque);
> +    ret = vmstate_subsection_load(f, vmsd, opaque, &error_fatal);
>      if (ret != 0) {
>          qemu_file_set_error(f, ret);
>          return ret;

This is now unreachable, no?

> @@ -566,7 +566,7 @@ vmstate_get_subsection(const VMStateDescription * const *sub,
>  }
>  
>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
> -                                   void *opaque)
> +                                   void *opaque, Error **errp)
>  {
>      trace_vmstate_subsection_load(vmsd->name);
>  
> @@ -598,6 +598,8 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>          sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
>          if (sub_vmsd == NULL) {
>              trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(lookup)");
> +            error_setg(errp, "VM subsection '%s' in '%s' does not exist",
> +                       idstr, vmsd->name);
>              return -ENOENT;
>          }
>          qemu_file_skip(f, 1); /* subsection */
> @@ -608,6 +610,9 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>          ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
>          if (ret) {
>              trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(child)");
> +            error_setg(errp,
> +                       "Loading VM subsection '%s' in '%s' failed: %d",
> +                       idstr, vmsd->name, ret);
>              return ret;
>          }
>      }

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

[Fabiano Rosas]

Arun Menon <armenon@redhat.com> writes:

> 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.
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  hw/display/virtio-gpu.c     |  2 +-
>  hw/pci/pci.c                |  3 ++-
>  hw/s390x/virtio-ccw.c       |  2 +-
>  hw/scsi/spapr_vscsi.c       |  2 +-
>  hw/vfio/pci.c               |  3 ++-
>  hw/virtio/virtio-mmio.c     |  3 ++-
>  hw/virtio/virtio-pci.c      |  2 +-
>  hw/virtio/virtio.c          |  4 +--
>  include/migration/vmstate.h |  2 +-
>  migration/cpr.c             |  5 ++--
>  migration/savevm.c          |  6 +++--
>  migration/vmstate-types.c   | 22 ++++++++++++----
>  migration/vmstate.c         | 61 ++++++++++++++++++++++++++++++-------------
>  tests/unit/test-vmstate.c   | 63 ++++++++++++++++++++++++++++++++++++++-------
>  ui/vdagent.c                |  3 ++-
>  15 files changed, 136 insertions(+), 47 deletions(-)
>
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 0a1a625b0ea6cf26cb0d799171a57ed3d3ab2442..5dc31bc6bfb0272e29a4364ab10de2595a4bedf7 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -1343,7 +1343,7 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
>      }
>  
>      /* load & apply scanout state */
> -    vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1);
> +    vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, &error_fatal);
>  
>      return 0;
>  }
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index c70b5ceebaf1f2b10768bd030526cbb518da2b8d..6be932d3bb67ff0c4808707db2a7b6378a90e82b 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -934,7 +934,8 @@ void pci_device_save(PCIDevice *s, QEMUFile *f)
>  int pci_device_load(PCIDevice *s, QEMUFile *f)
>  {
>      int ret;
> -    ret = vmstate_load_state(f, &vmstate_pci_device, s, s->version_id);
> +    ret = vmstate_load_state(f, &vmstate_pci_device, s, s->version_id,
> +                             &error_fatal);
>      /* Restore the interrupt status bit. */
>      pci_update_irq_status(s);
>      return ret;
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index d2f85b39f30f7fc82e0c600144c0a958e1269b2c..6a9641a03d5d3a38a4de7ceb9deffc0cc303bcff 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -1136,7 +1136,7 @@ static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f)
>  static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
>  {
>      VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
> -    return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1);
> +    return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1, &error_fatal);
>  }
>  
>  static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
> diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
> index 20f70fb2729de78b9636a6b8c869695dab4f8902..8c896022d324f51962605288d6d6df1648c83cde 100644
> --- a/hw/scsi/spapr_vscsi.c
> +++ b/hw/scsi/spapr_vscsi.c
> @@ -648,7 +648,7 @@ static void *vscsi_load_request(QEMUFile *f, SCSIRequest *sreq)
>      assert(!req->active);
>  
>      memset(req, 0, sizeof(*req));
> -    rc = vmstate_load_state(f, &vmstate_spapr_vscsi_req, req, 1);
> +    rc = vmstate_load_state(f, &vmstate_spapr_vscsi_req, req, 1, &error_fatal);
>      if (rc) {
>          fprintf(stderr, "VSCSI: failed loading request tag#%u\n", sreq->tag);
>          return NULL;
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 4fa692c1a32bcfa4e4939e5fcb64f2bf19905b3b..0be54762cdcbdb4780b8228b0bdf7fc6bd74dd57 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2795,7 +2795,8 @@ static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
>          old_addr[bar] = pdev->io_regions[bar].addr;
>      }
>  
> -    ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1);
> +    ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1,
> +                             &error_fatal);
>      if (ret) {
>          return ret;
>      }
> diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> index 532c67107ba1d2978a76cf49f9cdc1de1dea3e11..0a688909fc606a3c9fde933667ae8c309ab527d0 100644
> --- a/hw/virtio/virtio-mmio.c
> +++ b/hw/virtio/virtio-mmio.c
> @@ -34,6 +34,7 @@
>  #include "qemu/error-report.h"
>  #include "qemu/log.h"
>  #include "trace.h"
> +#include "qapi/error.h"
>  
>  static bool virtio_mmio_ioeventfd_enabled(DeviceState *d)
>  {
> @@ -619,7 +620,7 @@ static int virtio_mmio_load_extra_state(DeviceState *opaque, QEMUFile *f)
>  {
>      VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
>  
> -    return vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1);
> +    return vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1, &error_fatal);
>  }
>  
>  static bool virtio_mmio_has_extra_state(DeviceState *opaque)
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 767216d795998708f5716a23ae16c79cd90ff489..b04faa1e5c91b5cef40e54ec41d92422d16bfc13 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -161,7 +161,7 @@ static int virtio_pci_load_extra_state(DeviceState *d, QEMUFile *f)
>  {
>      VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
>  
> -    return vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1);
> +    return vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1, &error_fatal);
>  }
>  
>  static void virtio_pci_save_queue(DeviceState *d, int n, QEMUFile *f)
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 9a81ad912e013fc254899c4e55cff1f76a6112a4..6bcafb338d1b5becadcacf092ba33a6ae4c3d194 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3327,14 +3327,14 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>      }
>  
>      if (vdc->vmsd) {
> -        ret = vmstate_load_state(f, vdc->vmsd, vdev, version_id);
> +        ret = vmstate_load_state(f, vdc->vmsd, vdev, version_id, &error_fatal);
>          if (ret) {
>              return ret;
>          }
>      }
>  
>      /* Subsections */
> -    ret = vmstate_load_state(f, &vmstate_virtio, vdev, 1);
> +    ret = vmstate_load_state(f, &vmstate_virtio, vdev, 1, &error_fatal);
>      if (ret) {
>          return ret;
>      }
> diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> index 1ff7bd9ac425ba67cd5ca7ad97bcf570f9e19abe..056781b1c21e737583f081594d9f88b32adfd674 100644
> --- a/include/migration/vmstate.h
> +++ b/include/migration/vmstate.h
> @@ -1196,7 +1196,7 @@ extern const VMStateInfo vmstate_info_qlist;
>      }
>  
>  int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> -                       void *opaque, int version_id);
> +                       void *opaque, int version_id, Error **errp);
>  int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>                         void *opaque, JSONWriter *vmdesc);
>  int vmstate_save_state_with_err(QEMUFile *f, const VMStateDescription *vmsd,
> diff --git a/migration/cpr.c b/migration/cpr.c
> index 42ad0b0d500e5de57faf0c6517e216b2d1c0cacf..bdb24736f44e91ba59b6e622a315597c97e7f64d 100644
> --- a/migration/cpr.c
> +++ b/migration/cpr.c
> @@ -202,6 +202,7 @@ int cpr_state_save(MigrationChannel *channel, Error **errp)
>  
>  int cpr_state_load(MigrationChannel *channel, Error **errp)
>  {
> +    ERRP_GUARD();
>      int ret;
>      uint32_t v;
>      QEMUFile *f;
> @@ -233,9 +234,9 @@ int cpr_state_load(MigrationChannel *channel, Error **errp)
>          return -ENOTSUP;
>      }
>  
> -    ret = vmstate_load_state(f, &vmstate_cpr_state, &cpr_state, 1);
> +    ret = vmstate_load_state(f, &vmstate_cpr_state, &cpr_state, 1, errp);
>      if (ret) {
> -        error_setg(errp, "vmstate_load_state error %d", ret);
> +        error_prepend(errp, "vmstate_load_state error %d: ", ret);
>          qemu_fclose(f);
>          return ret;
>      }
> diff --git a/migration/savevm.c b/migration/savevm.c
> index fabbeb296ae987d0c06ba6dafda63720205fecfd..cb64f2855d46aaa7c617b3e4079a2c9e566079b2 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -969,7 +969,8 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se)
>      if (!se->vmsd) {         /* Old style */
>          return se->ops->load_state(f, se->opaque, se->load_version_id);
>      }
> -    return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id);
> +    return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id,
> +                              &error_fatal);
>  }
>  
>  static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
> @@ -2839,7 +2840,8 @@ static int qemu_loadvm_state_header(QEMUFile *f)
>              error_report("Configuration section missing");
>              return -EINVAL;
>          }
> -        ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0);
> +        ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0,
> +                                 &error_fatal);
>  
>          if (ret) {
>              return ret;
> diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> index 741a588b7e18c6d37724b08a0101edc8bc74a0a5..f41670cc853c5b41ccc8def354886a8e5c1451fd 100644
> --- a/migration/vmstate-types.c
> +++ b/migration/vmstate-types.c
> @@ -19,6 +19,7 @@
>  #include "qemu/error-report.h"
>  #include "qemu/queue.h"
>  #include "trace.h"
> +#include "qapi/error.h"
>  
>  /* bool */
>  
> @@ -543,13 +544,17 @@ static int get_tmp(QEMUFile *f, void *pv, size_t size,
>                     const VMStateField *field)
>  {
>      int ret;
> +    Error *local_err = NULL;
>      const VMStateDescription *vmsd = field->vmsd;
>      int version_id = field->version_id;
>      void *tmp = g_malloc(size);
>  
>      /* Writes the parent field which is at the start of the tmp */
>      *(void **)tmp = pv;
> -    ret = vmstate_load_state(f, vmsd, tmp, version_id);
> +    ret = vmstate_load_state(f, vmsd, tmp, version_id, &local_err);
> +    if (ret < 0) {
> +        warn_report_err(local_err);
> +    }
>      g_free(tmp);
>      return ret;
>  }
> @@ -626,6 +631,7 @@ static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size,
>                        const VMStateField *field)
>  {
>      int ret = 0;
> +    Error *local_err = NULL;
>      const VMStateDescription *vmsd = field->vmsd;
>      /* size of a QTAILQ element */
>      size_t size = field->size;
> @@ -649,8 +655,9 @@ static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size,
>  
>      while (qemu_get_byte(f)) {
>          elm = g_malloc(size);
> -        ret = vmstate_load_state(f, vmsd, elm, version_id);
> +        ret = vmstate_load_state(f, vmsd, elm, version_id, &local_err);
>          if (ret) {
> +            warn_report_err(local_err);
>              return ret;
>          }
>          QTAILQ_RAW_INSERT_TAIL(pv, elm, entry_offset);
> @@ -772,6 +779,7 @@ static int get_gtree(QEMUFile *f, void *pv, size_t unused_size,
>      GTree *tree = *pval;
>      void *key, *val;
>      int ret = 0;
> +    Error *local_err = NULL;
>  
>      /* in case of direct key, the key vmsd can be {}, ie. check fields */
>      if (!direct_key && version_id > key_vmsd->version_id) {
> @@ -803,18 +811,20 @@ static int get_gtree(QEMUFile *f, void *pv, size_t unused_size,
>              key = (void *)(uintptr_t)qemu_get_be64(f);
>          } else {
>              key = g_malloc0(key_size);
> -            ret = vmstate_load_state(f, key_vmsd, key, version_id);
> +            ret = vmstate_load_state(f, key_vmsd, key, version_id, &local_err);
>              if (ret) {
>                  error_report("%s : failed to load %s (%d)",
>                               field->name, key_vmsd->name, ret);
> +                warn_report_err(local_err);
>                  goto key_error;
>              }
>          }
>          val = g_malloc0(val_size);
> -        ret = vmstate_load_state(f, val_vmsd, val, version_id);
> +        ret = vmstate_load_state(f, val_vmsd, val, version_id, &local_err);
>          if (ret) {
>              error_report("%s : failed to load %s (%d)",
>                           field->name, val_vmsd->name, ret);
> +            warn_report_err(local_err);
>              goto val_error;
>          }
>          g_tree_insert(tree, key, val);
> @@ -872,6 +882,7 @@ static int get_qlist(QEMUFile *f, void *pv, size_t unused_size,
>                       const VMStateField *field)
>  {
>      int ret = 0;
> +    Error *local_err = NULL;
>      const VMStateDescription *vmsd = field->vmsd;
>      /* size of a QLIST element */
>      size_t size = field->size;
> @@ -892,10 +903,11 @@ static int get_qlist(QEMUFile *f, void *pv, size_t unused_size,
>  
>      while (qemu_get_byte(f)) {
>          elm = g_malloc(size);
> -        ret = vmstate_load_state(f, vmsd, elm, version_id);
> +        ret = vmstate_load_state(f, vmsd, elm, version_id, &local_err);
>          if (ret) {
>              error_report("%s: failed to load %s (%d)", field->name,
>                           vmsd->name, ret);
> +            warn_report_err(local_err);
>              g_free(elm);
>              return ret;
>          }
> diff --git a/migration/vmstate.c b/migration/vmstate.c
> index 6108c7fe283a5013ce42ea9987723c489aef26a2..1cd609a1d598995af1e51d1f4d58d68133f1426d 100644
> --- a/migration/vmstate.c
> +++ b/migration/vmstate.c
> @@ -132,29 +132,34 @@ 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)
>  {
> +    ERRP_GUARD();
>      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 for: '%s', "

"VM" pre load is a little ambiguous. Simply "pre load" or "pre load
hook" is better.

> +                       "version_id: %d, minimum version_id: %d, ret: %d",
> +                       vmsd->name, vmsd->version_id, vmsd->minimum_version_id,
> +                       ret);
>              return ret;
>          }
>      }
> @@ -192,13 +197,20 @@ 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);
> +                    if (ret < 0) {
> +                        error_setg(errp,
> +                                   "Failed to get info for %s: %d",
> +                                   inner_field->name, ret);

"get info" is not correct. This is the type-specific getter
invocation. Because the migration (for the most part) is a stream, each
type provides it's own getter which knows about the size of the field
and any particularities such as magic values. So:

error_setg(errp, "Failed to load element of type %s for %s: %d",
           info->name, inner_field->name, ret);

> +                    }
>                  }
>  
>                  /* If we used a fake temp field.. free it now */
> @@ -208,30 +220,42 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>  
>                  if (ret >= 0) {
>                      ret = qemu_file_get_error(f);
> +                    if (ret < 0) {
> +                        error_setg(errp, "Failed to load %s state: %d",
> +                                   vmsd->name, ret);

We could go a little more specific here, it's useful to know whether the
error was on the transport side, rather than something logical with the
migrated data. I don't really care about the actual string, but one
suggestion is "stream error":

error_setg(errp, "Failed to load %s state: stream error: %d",
           vmsd->name, ret);

> +                    }
>                  }
>                  if (ret < 0) {
>                      qemu_file_set_error(f, ret);
> -                    error_report("Failed to load %s:%s", vmsd->name,
> -                                 field->name);
> +                    error_prepend(errp,
> +                                  "Failed to load %s:%s version_id: %d: ",

Usage of : is inconsistent with below /

> +                                   vmsd->name, field->name, vmsd->version_id);
>                      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 version_id: %d",

here

> +                       vmsd->name, field->name, vmsd->version_id);
>              return -1;
>          }
>          field++;
>      }
>      assert(field->flags == VMS_END);
> -    ret = vmstate_subsection_load(f, vmsd, opaque, &error_fatal);
> +    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 for: %s, version_id: %d, "
> +                       "minimum_version: %d, ret: %d",
> +                       vmsd->name, vmsd->version_id, vmsd->minimum_version_id,
> +                       ret);
> +        }
>      }
>      trace_vmstate_load_state_end(vmsd->name, "end", ret);
>      return ret;
> @@ -568,6 +592,7 @@ vmstate_get_subsection(const VMStateDescription * const *sub,
>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>                                     void *opaque, Error **errp)
>  {
> +    ERRP_GUARD();
>      trace_vmstate_subsection_load(vmsd->name);
>  
>      while (qemu_peek_byte(f, 0) == QEMU_VM_SUBSECTION) {
> @@ -607,12 +632,12 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>          qemu_file_skip(f, len); /* idstr */
>          version_id = qemu_get_be32(f);
>  
> -        ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
> +        ret = vmstate_load_state(f, sub_vmsd, opaque, version_id, errp);
>          if (ret) {
>              trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(child)");
> -            error_setg(errp,
> -                       "Loading VM subsection '%s' in '%s' failed: %d",
> -                       idstr, vmsd->name, ret);
> +            error_prepend(errp,
> +                          "Loading VM subsection '%s' in '%s' failed: %d: ",
> +                          idstr, vmsd->name, ret);
>              return ret;
>          }
>      }
> diff --git a/tests/unit/test-vmstate.c b/tests/unit/test-vmstate.c
> index 63f28f26f45691a70936d33e7341d16477a3471f..cfab58c7f45ba50f70af164c3e58b01aaf9cc656 100644
> --- a/tests/unit/test-vmstate.c
> +++ b/tests/unit/test-vmstate.c
> @@ -30,6 +30,7 @@
>  #include "../migration/savevm.h"
>  #include "qemu/module.h"
>  #include "io/channel-file.h"
> +#include "qapi/error.h"
>  
>  static int temp_fd;
>  
> @@ -108,14 +109,16 @@ static int load_vmstate_one(const VMStateDescription *desc, void *obj,
>  {
>      QEMUFile *f;
>      int ret;
> +    Error *local_err = NULL;
>  
>      f = open_test_file(true);
>      qemu_put_buffer(f, wire, size);
>      qemu_fclose(f);
>  
>      f = open_test_file(false);
> -    ret = vmstate_load_state(f, desc, obj, version);
> +    ret = vmstate_load_state(f, desc, obj, version, &local_err);
>      if (ret) {
> +        warn_report_err(local_err);
>          g_assert(qemu_file_get_error(f));
>      } else{
>          g_assert(!qemu_file_get_error(f));
> @@ -355,6 +358,8 @@ static const VMStateDescription vmstate_versioned = {
>  
>  static void test_load_v1(void)
>  {
> +    Error *local_err = NULL;
> +    int ret;
>      uint8_t buf[] = {
>          0, 0, 0, 10,             /* a */
>          0, 0, 0, 30,             /* c */
> @@ -365,7 +370,10 @@ static void test_load_v1(void)
>  
>      QEMUFile *loading = open_test_file(false);
>      TestStruct obj = { .b = 200, .e = 500, .f = 600 };
> -    vmstate_load_state(loading, &vmstate_versioned, &obj, 1);
> +    ret = vmstate_load_state(loading, &vmstate_versioned, &obj, 1, &local_err);
> +    if (ret < 0) {
> +        warn_report_err(local_err);
> +    }
>      g_assert(!qemu_file_get_error(loading));
>      g_assert_cmpint(obj.a, ==, 10);
>      g_assert_cmpint(obj.b, ==, 200);
> @@ -378,6 +386,8 @@ static void test_load_v1(void)
>  
>  static void test_load_v2(void)
>  {
> +    Error *local_err = NULL;
> +    int ret;
>      uint8_t buf[] = {
>          0, 0, 0, 10,             /* a */
>          0, 0, 0, 20,             /* b */
> @@ -391,7 +401,10 @@ static void test_load_v2(void)
>  
>      QEMUFile *loading = open_test_file(false);
>      TestStruct obj;
> -    vmstate_load_state(loading, &vmstate_versioned, &obj, 2);
> +    ret = vmstate_load_state(loading, &vmstate_versioned, &obj, 2, &local_err);
> +    if (ret < 0) {
> +        warn_report_err(local_err);
> +    }
>      g_assert_cmpint(obj.a, ==, 10);
>      g_assert_cmpint(obj.b, ==, 20);
>      g_assert_cmpint(obj.c, ==, 30);
> @@ -467,6 +480,8 @@ static void test_save_skip(void)
>  
>  static void test_load_noskip(void)
>  {
> +    Error *local_err = NULL;
> +    int ret;
>      uint8_t buf[] = {
>          0, 0, 0, 10,             /* a */
>          0, 0, 0, 20,             /* b */
> @@ -480,7 +495,10 @@ static void test_load_noskip(void)
>  
>      QEMUFile *loading = open_test_file(false);
>      TestStruct obj = { .skip_c_e = false };
> -    vmstate_load_state(loading, &vmstate_skipping, &obj, 2);
> +    ret = vmstate_load_state(loading, &vmstate_skipping, &obj, 2, &local_err);
> +    if (ret < 0) {
> +        warn_report_err(local_err);
> +    }
>      g_assert(!qemu_file_get_error(loading));
>      g_assert_cmpint(obj.a, ==, 10);
>      g_assert_cmpint(obj.b, ==, 20);
> @@ -493,6 +511,8 @@ static void test_load_noskip(void)
>  
>  static void test_load_skip(void)
>  {
> +    Error *local_err = NULL;
> +    int ret;
>      uint8_t buf[] = {
>          0, 0, 0, 10,             /* a */
>          0, 0, 0, 20,             /* b */
> @@ -504,7 +524,10 @@ static void test_load_skip(void)
>  
>      QEMUFile *loading = open_test_file(false);
>      TestStruct obj = { .skip_c_e = true, .c = 300, .e = 500 };
> -    vmstate_load_state(loading, &vmstate_skipping, &obj, 2);
> +    ret = vmstate_load_state(loading, &vmstate_skipping, &obj, 2, &local_err);
> +    if (ret < 0) {
> +        warn_report_err(local_err);
> +    }
>      g_assert(!qemu_file_get_error(loading));
>      g_assert_cmpint(obj.a, ==, 10);
>      g_assert_cmpint(obj.b, ==, 20);
> @@ -744,6 +767,8 @@ static void test_save_q(void)
>  
>  static void test_load_q(void)
>  {
> +    int ret;
> +    Error *local_err = NULL;
>      TestQtailq obj_q = {
>          .i16 = -512,
>          .i32 = 70000,
> @@ -773,7 +798,10 @@ static void test_load_q(void)
>      TestQtailq tgt;
>  
>      QTAILQ_INIT(&tgt.q);
> -    vmstate_load_state(fload, &vmstate_q, &tgt, 1);
> +    ret = vmstate_load_state(fload, &vmstate_q, &tgt, 1, &local_err);
> +    if (ret < 0) {
> +        warn_report_err(local_err);
> +    }
>      char eof = qemu_get_byte(fload);
>      g_assert(!qemu_file_get_error(fload));
>      g_assert_cmpint(tgt.i16, ==, obj_q.i16);
> @@ -1115,6 +1143,8 @@ static void diff_iommu(TestGTreeIOMMU *iommu1, TestGTreeIOMMU *iommu2)
>  
>  static void test_gtree_load_domain(void)
>  {
> +    Error *local_err = NULL;
> +    int ret;
>      TestGTreeDomain *dest_domain = g_new0(TestGTreeDomain, 1);
>      TestGTreeDomain *orig_domain = create_first_domain();
>      QEMUFile *fload, *fsave;
> @@ -1127,7 +1157,11 @@ static void test_gtree_load_domain(void)
>  
>      fload = open_test_file(false);
>  
> -    vmstate_load_state(fload, &vmstate_domain, dest_domain, 1);
> +    ret = vmstate_load_state(fload, &vmstate_domain, dest_domain, 1,
> +                             &local_err);
> +    if (ret < 0) {
> +        warn_report_err(local_err);
> +    }
>      eof = qemu_get_byte(fload);
>      g_assert(!qemu_file_get_error(fload));
>      g_assert_cmpint(orig_domain->id, ==, dest_domain->id);
> @@ -1230,6 +1264,8 @@ static void test_gtree_save_iommu(void)
>  
>  static void test_gtree_load_iommu(void)
>  {
> +    Error *local_err = NULL;
> +    int ret;
>      TestGTreeIOMMU *dest_iommu = g_new0(TestGTreeIOMMU, 1);
>      TestGTreeIOMMU *orig_iommu = create_iommu();
>      QEMUFile *fsave, *fload;
> @@ -1241,7 +1277,10 @@ static void test_gtree_load_iommu(void)
>      qemu_fclose(fsave);
>  
>      fload = open_test_file(false);
> -    vmstate_load_state(fload, &vmstate_iommu, dest_iommu, 1);
> +    ret = vmstate_load_state(fload, &vmstate_iommu, dest_iommu, 1, &local_err);
> +    if (ret < 0) {
> +        warn_report_err(local_err);
> +    }
>      eof = qemu_get_byte(fload);
>      g_assert(!qemu_file_get_error(fload));
>      g_assert_cmpint(orig_iommu->id, ==, dest_iommu->id);
> @@ -1363,6 +1402,8 @@ static void test_save_qlist(void)
>  
>  static void test_load_qlist(void)
>  {
> +    Error *local_err = NULL;
> +    int ret;
>      QEMUFile *fsave, *fload;
>      TestQListContainer *orig_container = alloc_container();
>      TestQListContainer *dest_container = g_new0(TestQListContainer, 1);
> @@ -1376,7 +1417,11 @@ static void test_load_qlist(void)
>      qemu_fclose(fsave);
>  
>      fload = open_test_file(false);
> -    vmstate_load_state(fload, &vmstate_container, dest_container, 1);
> +    ret = vmstate_load_state(fload, &vmstate_container, dest_container, 1,
> +                             &local_err);
> +    if (ret < 0) {
> +        warn_report_err(local_err);
> +    }
>      eof = qemu_get_byte(fload);
>      g_assert(!qemu_file_get_error(fload));
>      g_assert_cmpint(eof, ==, QEMU_VM_EOF);
> diff --git a/ui/vdagent.c b/ui/vdagent.c
> index c0746fe5b168fdc7aeb4866de2ba0c3387566649..b699658c06cc3765bae2e5effa34f66b7cfd4ead 100644
> --- a/ui/vdagent.c
> +++ b/ui/vdagent.c
> @@ -1008,7 +1008,8 @@ static int get_cbinfo(QEMUFile *f, void *pv, size_t size,
>  
>      vdagent_clipboard_peer_register(vd);
>  
> -    ret = vmstate_load_state(f, &vmstate_cbinfo_array, &cbinfo, 0);
> +    ret = vmstate_load_state(f, &vmstate_cbinfo_array, &cbinfo, 0,
> +                             &error_fatal);
>      if (ret) {
>          return ret;
>      }

Unreachable?

Re: [PATCH v11 03/27] migration: push Error **errp into qemu_loadvm_state_header()

[Fabiano Rosas]

Arun Menon <armenon@redhat.com> writes:

> 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 qemu_loadvm_state_header() must report an error
> in errp, in case of failure.
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v11 04/27] migration: push Error **errp into vmstate_load()

[Fabiano Rosas]

Arun Menon <armenon@redhat.com> writes:

> 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() must report an error
> in errp, in case of failure.
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  migration/savevm.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 0c445a957fc99f826e6753ed3795bcdd51f1e3f5..7f79461844105bf672314c3325caee9cdb654c27 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -963,14 +963,20 @@ void vmstate_unregister(VMStateIf *obj, const VMStateDescription *vmsd,
>      }
>  }
>  
> -static int vmstate_load(QEMUFile *f, SaveStateEntry *se)
> +static int vmstate_load(QEMUFile *f, SaveStateEntry *se, Error **errp)
>  {
> +    int ret;
>      trace_vmstate_load(se->idstr, se->vmsd ? se->vmsd->name : "(old)");
>      if (!se->vmsd) {         /* Old style */
> -        return se->ops->load_state(f, se->opaque, se->load_version_id);
> +        ret = se->ops->load_state(f, se->opaque, se->load_version_id);
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load VM version_id: %d, ret: %d",

"VM" is ambiguous. I'd use "vmstate".

> +                       se->load_version_id, ret);
> +        }
> +        return ret;
>      }
>      return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id,
> -                              &error_fatal);
> +                              errp);
>  }
>  
>  static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
> @@ -2692,6 +2698,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
>      SaveStateEntry *se;
>      char idstr[256];
>      int ret;
> +    Error *local_err = NULL;
>  
>      /* Read section start */
>      section_id = qemu_get_be32(f);
> @@ -2741,10 +2748,11 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
>          start_ts = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
>      }
>  
> -    ret = vmstate_load(f, se);
> +    ret = vmstate_load(f, se, &local_err);
>      if (ret < 0) {
>          error_report("error while loading state for instance 0x%"PRIx32" of"
>                       " device '%s'", instance_id, idstr);
> +        warn_report_err(local_err);

I was about to ask why you're using a warning here, but I see you remove
it futher in the series. That's fine, but this kind of thing should be
mentioned in the commit message, otherwise it gets confusing to review.

>          return ret;
>      }
>  
> @@ -2769,6 +2777,7 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
>      uint32_t section_id;
>      SaveStateEntry *se;
>      int ret;
> +    Error *local_err = NULL;
>  
>      section_id = qemu_get_be32(f);
>  
> @@ -2794,10 +2803,11 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
>          start_ts = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
>      }
>  
> -    ret = vmstate_load(f, se);
> +    ret = vmstate_load(f, se, &local_err);
>      if (ret < 0) {
>          error_report("error while loading state section id %d(%s)",
>                       section_id, se->idstr);
> +        warn_report_err(local_err);
>          return ret;
>      }

Re: [PATCH v11 05/27] migration: push Error **errp into loadvm_process_command()

[Fabiano Rosas]

Arun Menon <armenon@redhat.com> writes:

> 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 loadvm_process_command() must report an error
> in errp, in case of failure.
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  migration/savevm.c | 82 +++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 60 insertions(+), 22 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 7f79461844105bf672314c3325caee9cdb654c27..715cc35394cac5fe225ef88cf448714a02321bd7 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2546,32 +2546,35 @@ static int loadvm_postcopy_handle_switchover_start(void)
>   * LOADVM_QUIT All good, but exit the loop
>   * <0          Error
>   */
> -static int loadvm_process_command(QEMUFile *f)
> +static int loadvm_process_command(QEMUFile *f, Error **errp)
>  {
>      MigrationIncomingState *mis = migration_incoming_get_current();
>      uint16_t cmd;
>      uint16_t len;
>      uint32_t tmp32;
> +    int ret;
>  
>      cmd = qemu_get_be16(f);
>      len = qemu_get_be16(f);
>  
>      /* Check validity before continue processing of cmds */
> -    if (qemu_file_get_error(f)) {
> -        return qemu_file_get_error(f);
> +    ret = qemu_file_get_error(f);
> +    if (ret) {
> +        error_setg(errp, "Failed to load VM process command: %d", ret);

"Failed to process command: stream error: %d"

> +        return ret;
>      }
>  
>      if (cmd >= MIG_CMD_MAX || cmd == MIG_CMD_INVALID) {
> -        error_report("MIG_CMD 0x%x unknown (len 0x%x)", cmd, len);
> +        error_setg(errp, "MIG_CMD 0x%x unknown (len 0x%x)", cmd, len);
>          return -EINVAL;
>      }
>  
>      trace_loadvm_process_command(mig_cmd_args[cmd].name, len);
>  
>      if (mig_cmd_args[cmd].len != -1 && mig_cmd_args[cmd].len != len) {
> -        error_report("%s received with bad length - expecting %zu, got %d",
> -                     mig_cmd_args[cmd].name,
> -                     (size_t)mig_cmd_args[cmd].len, len);
> +        error_setg(errp, "%s received with bad length - expecting %zu, got %d",
> +                   mig_cmd_args[cmd].name,
> +                   (size_t)mig_cmd_args[cmd].len, len);
>          return -ERANGE;
>      }
>  

Where's MIG_CMD_OPEN_RETURN_PATH?

> @@ -2594,11 +2597,10 @@ static int loadvm_process_command(QEMUFile *f)
>           * been created.
>           */
>          if (migrate_switchover_ack() && !mis->switchover_ack_pending_num) {
> -            int ret = migrate_send_rp_switchover_ack(mis);
> +            ret = migrate_send_rp_switchover_ack(mis);
>              if (ret) {
> -                error_report(
> -                    "Could not send switchover ack RP MSG, err %d (%s)", ret,
> -                    strerror(-ret));
> +                error_setg_errno(errp, -ret,
> +                                 "Could not send switchover ack RP MSG");
>                  return ret;
>              }
>          }
> @@ -2608,39 +2610,71 @@ static int loadvm_process_command(QEMUFile *f)
>          tmp32 = qemu_get_be32(f);
>          trace_loadvm_process_command_ping(tmp32);
>          if (!mis->to_src_file) {
> -            error_report("CMD_PING (0x%x) received with no return path",
> -                         tmp32);
> +            error_setg(errp, "CMD_PING (0x%x) received with no return path",
> +                       tmp32);
>              return -1;
>          }
>          migrate_send_rp_pong(mis, tmp32);
>          break;
>  
>      case MIG_CMD_PACKAGED:
> -        return loadvm_handle_cmd_packaged(mis);
> +        ret = loadvm_handle_cmd_packaged(mis);

I missed a lot of the discussion in this series, but I assume there's a
good reason to not put the conversion of each command first in the
series, so there's no need for temporary code in this patch.

> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +        }
> +        return ret;
>  
>      case MIG_CMD_POSTCOPY_ADVISE:
> -        return loadvm_postcopy_handle_advise(mis, len);
> +        ret = loadvm_postcopy_handle_advise(mis, len);
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +        }
> +        return ret;
>  
>      case MIG_CMD_POSTCOPY_LISTEN:
> -        return loadvm_postcopy_handle_listen(mis);
> +        ret = loadvm_postcopy_handle_listen(mis);
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +        }
> +        return ret;
>  
>      case MIG_CMD_POSTCOPY_RUN:
> -        return loadvm_postcopy_handle_run(mis);
> +        ret = loadvm_postcopy_handle_run(mis);
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +        }
> +        return ret;
>  
>      case MIG_CMD_POSTCOPY_RAM_DISCARD:
> -        return loadvm_postcopy_ram_handle_discard(mis, len);
> +        ret = loadvm_postcopy_ram_handle_discard(mis, len);
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +        }
> +        return ret;
>  
>      case MIG_CMD_POSTCOPY_RESUME:
>          return loadvm_postcopy_handle_resume(mis);
>  
>      case MIG_CMD_RECV_BITMAP:
> -        return loadvm_handle_recv_bitmap(mis, len);
> +        ret = loadvm_handle_recv_bitmap(mis, len);
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +        }
> +        return ret;
>  
>      case MIG_CMD_ENABLE_COLO:
> -        return loadvm_process_enable_colo(mis);
> +        ret = loadvm_process_enable_colo(mis);
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +        }
> +        return ret;
>  
>      case MIG_CMD_SWITCHOVER_START:
> -        return loadvm_postcopy_handle_switchover_start();
> +        ret = loadvm_postcopy_handle_switchover_start();
> +        if (ret < 0) {
> +            error_setg(errp, "Failed to load device state command: %d", ret);
> +        }
> +        return ret;
>      }
>  
>      return 0;
> @@ -3051,6 +3085,7 @@ int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
>  {
>      uint8_t section_type;
>      int ret = 0;
> +    Error *local_err = NULL;
>  
>  retry:
>      while (true) {
> @@ -3078,7 +3113,10 @@ retry:
>              }
>              break;
>          case QEMU_VM_COMMAND:
> -            ret = loadvm_process_command(f);
> +            ret = loadvm_process_command(f, &local_err);
> +            if (ret < 0) {
> +                warn_report_err(local_err);

Again, some throwaway code here. Commit message could have made this
clear: "For now, report the error with warn_report until all callers
have been converted to pass errp".

> +            }
>              trace_qemu_loadvm_state_section_command(ret);
>              if ((ret < 0) || (ret == LOADVM_QUIT)) {
>                  goto out;

Re: [PATCH v11 06/27] migration: push Error **errp into loadvm_handle_cmd_packaged()

[Fabiano Rosas]

Arun Menon <armenon@redhat.com> writes:

> 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 loadvm_handle_cmd_packaged() must report an error
> in errp, in case of failure.
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v11 07/27] migration: push Error **errp into qemu_loadvm_state()

[Fabiano Rosas]

Arun Menon <armenon@redhat.com> writes:

> 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 qemu_loadvm_state() must report an error
> in errp, in case of failure.
>
> When postcopy live migration runs, the device states are loaded by
> both the qemu coroutine process_incoming_migration_co() and the
> postcopy_ram_listen_thread(). Therefore, it is important that the
> coroutine also reports the error in case of failure, with
> error_report_err(). Otherwise, the source qemu will not display
> any errors before going into the postcopy pause state.
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  migration/migration.c |  9 +++++----
>  migration/savevm.c    | 31 +++++++++++++++++++------------
>  migration/savevm.h    |  2 +-
>  3 files changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 10c216d25dec01f206eacad2edd24d21f00e614c..c6768d88f45c870c7fad9b9957300766ff69effc 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -881,7 +881,7 @@ process_incoming_migration_co(void *opaque)
>                        MIGRATION_STATUS_ACTIVE);
>  
>      mis->loadvm_co = qemu_coroutine_self();
> -    ret = qemu_loadvm_state(mis->from_src_file);
> +    ret = qemu_loadvm_state(mis->from_src_file, &local_err);
>      mis->loadvm_co = NULL;
>  
>      trace_vmstate_downtime_checkpoint("dst-precopy-loadvm-completed");
> @@ -908,7 +908,8 @@ process_incoming_migration_co(void *opaque)
>      }
>  
>      if (ret < 0) {
> -        error_setg(&local_err, "load of migration failed: %s", strerror(-ret));
> +        error_prepend(&local_err, "load of migration failed: %s: ",
> +                      strerror(-ret));
>          goto fail;
>      }
>  
> @@ -924,13 +925,13 @@ fail:
>      migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
>                        MIGRATION_STATUS_FAILED);
>      migrate_set_error(s, local_err);
> -    error_free(local_err);
> +    error_report_err(local_err);
>  
>      migration_incoming_state_destroy();
>  
>      if (mis->exit_on_error) {
>          WITH_QEMU_LOCK_GUARD(&s->error_mutex) {
> -            error_report_err(s->error);
> +            error_free(s->error);
>              s->error = NULL;
>          }
>  
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 4f8c40e284a1b199d12f3c7dd61212b3e0e057c9..05dc392bdf4e19f340bc72bf66ba0543d56868a5 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -3159,28 +3159,24 @@ out:
>      return ret;
>  }
>  
> -int qemu_loadvm_state(QEMUFile *f)
> +int qemu_loadvm_state(QEMUFile *f, Error **errp)
>  {
>      MigrationState *s = migrate_get_current();
>      MigrationIncomingState *mis = migration_incoming_get_current();
> -    Error *local_err = NULL;
>      int ret;
>  
> -    if (qemu_savevm_state_blocked(&local_err)) {
> -        error_report_err(local_err);
> +    if (qemu_savevm_state_blocked(errp)) {
>          return -EINVAL;
>      }
>  
>      qemu_loadvm_thread_pool_create(mis);
>  
> -    ret = qemu_loadvm_state_header(f, &local_err);
> +    ret = qemu_loadvm_state_header(f, errp);
>      if (ret) {
> -        error_report_err(local_err);
>          return ret;
>      }
>  
> -    if (qemu_loadvm_state_setup(f, &local_err) != 0) {
> -        error_report_err(local_err);
> +    if (qemu_loadvm_state_setup(f, errp) != 0) {
>          return -EINVAL;
>      }
>  
> @@ -3191,6 +3187,9 @@ int qemu_loadvm_state(QEMUFile *f)
>      cpu_synchronize_all_pre_loadvm();
>  
>      ret = qemu_loadvm_state_main(f, mis);
> +    if (ret < 0) {
> +        error_setg(errp, "Load VM state failed: %d", ret);
> +    }
>      qemu_event_set(&mis->main_thread_load_event);
>  
>      trace_qemu_loadvm_state_post_main(ret);
> @@ -3208,8 +3207,14 @@ int qemu_loadvm_state(QEMUFile *f)
>          if (migrate_has_error(migrate_get_current()) ||
>              !qemu_loadvm_thread_pool_wait(s, mis)) {
>              ret = -EINVAL;
> +            error_setg(errp,
> +                       "Error while loading VM state: "
> +                       "Migration stream has error");

The stream error is the one below. Just keep a generic message here
because we'll propagate the error from qemu_loadvm_state_main() later in
the series.

>          } else {
>              ret = qemu_file_get_error(f);
> +            if (ret < 0) {
> +                error_setg(errp, "Error while loading vmstate : %d", ret);

+ stream error:

> +            }
>          }
>      }
>      /*
> @@ -3474,6 +3479,7 @@ void qmp_xen_save_devices_state(const char *filename, bool has_live, bool live,
>  
>  void qmp_xen_load_devices_state(const char *filename, Error **errp)
>  {
> +    ERRP_GUARD();
>      QEMUFile *f;
>      QIOChannelFile *ioc;
>      int ret;
> @@ -3495,10 +3501,10 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
>      f = qemu_file_new_input(QIO_CHANNEL(ioc));
>      object_unref(OBJECT(ioc));
>  
> -    ret = qemu_loadvm_state(f);
> +    ret = qemu_loadvm_state(f, errp);
>      qemu_fclose(f);
>      if (ret < 0) {
> -        error_setg(errp, "loading Xen device state failed");
> +        error_prepend(errp, "loading Xen device state failed: ");
>      }
>      migration_incoming_state_destroy();
>  }
> @@ -3506,6 +3512,7 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
>  bool load_snapshot(const char *name, const char *vmstate,
>                     bool has_devices, strList *devices, Error **errp)
>  {
> +    ERRP_GUARD();
>      BlockDriverState *bs_vm_state;
>      QEMUSnapshotInfo sn;
>      QEMUFile *f;
> @@ -3569,13 +3576,13 @@ bool load_snapshot(const char *name, const char *vmstate,
>          ret = -EINVAL;
>          goto err_drain;
>      }
> -    ret = qemu_loadvm_state(f);
> +    ret = qemu_loadvm_state(f, errp);
>      migration_incoming_state_destroy();
>  
>      bdrv_drain_all_end();
>  
>      if (ret < 0) {
> -        error_setg(errp, "Error %d while loading VM state", ret);
> +        error_prepend(errp, "Error %d while loading VM state: ", ret);

This message will get redundant, leave it out.

>          return false;
>      }
>  
> diff --git a/migration/savevm.h b/migration/savevm.h
> index 2d5e9c716686f06720325e82fe90c75335ced1de..b80770b7461a60e2ad6ba5e24a7baeae73d90955 100644
> --- a/migration/savevm.h
> +++ b/migration/savevm.h
> @@ -64,7 +64,7 @@ void qemu_savevm_send_colo_enable(QEMUFile *f);
>  void qemu_savevm_live_state(QEMUFile *f);
>  int qemu_save_device_state(QEMUFile *f);
>  
> -int qemu_loadvm_state(QEMUFile *f);
> +int qemu_loadvm_state(QEMUFile *f, Error **errp);
>  void qemu_loadvm_state_cleanup(MigrationIncomingState *mis);
>  int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis);
>  int qemu_load_device_state(QEMUFile *f);

Re: [PATCH v11 08/27] migration: push Error **errp into qemu_load_device_state()

[Fabiano Rosas]

Arun Menon <armenon@redhat.com> writes:

> 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 qemu_load_device_state() must report an error
> in errp, in case of failure.
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  migration/colo.c   | 4 ++--
>  migration/savevm.c | 5 +++--
>  migration/savevm.h | 2 +-
>  3 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/migration/colo.c b/migration/colo.c
> index e0f713c837f5da25d67afbd02ceb6c54024ca3af..0ba22ee76a13e313793f653f43a728e3c433bbc1 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -729,9 +729,9 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
>      bql_lock();
>      vmstate_loading = true;
>      colo_flush_ram_cache();
> -    ret = qemu_load_device_state(fb);
> +    ret = qemu_load_device_state(fb, errp);
>      if (ret < 0) {
> -        error_setg(errp, "COLO: load device state failed");
> +        error_prepend(errp, "COLO: load device state failed: ");

This will say: "COLO: load device state failed: Failed to load device
state: %d"

Just remove it.

>          vmstate_loading = false;
>          bql_unlock();
>          return;
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 05dc392bdf4e19f340bc72bf66ba0543d56868a5..70e021597d884030c4a0dc2a7bc27d42a7371797 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -3263,15 +3263,16 @@ int qemu_loadvm_state(QEMUFile *f, Error **errp)
>      return ret;
>  }
>  
> -int qemu_load_device_state(QEMUFile *f)
> +int qemu_load_device_state(QEMUFile *f, Error **errp)
>  {
> +    ERRP_GUARD();

Is this needed here?

>      MigrationIncomingState *mis = migration_incoming_get_current();
>      int ret;
>  
>      /* Load QEMU_VM_SECTION_FULL section */
>      ret = qemu_loadvm_state_main(f, mis);
>      if (ret < 0) {
> -        error_report("Failed to load device state: %d", ret);
> +        error_setg(errp, "Failed to load device state: %d", ret);
>          return ret;
>      }
>  
> diff --git a/migration/savevm.h b/migration/savevm.h
> index b80770b7461a60e2ad6ba5e24a7baeae73d90955..b12681839f0b1afa3255e45215d99c13a224b19f 100644
> --- a/migration/savevm.h
> +++ b/migration/savevm.h
> @@ -67,7 +67,7 @@ int qemu_save_device_state(QEMUFile *f);
>  int qemu_loadvm_state(QEMUFile *f, Error **errp);
>  void qemu_loadvm_state_cleanup(MigrationIncomingState *mis);
>  int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis);
> -int qemu_load_device_state(QEMUFile *f);
> +int qemu_load_device_state(QEMUFile *f, Error **errp);
>  int qemu_loadvm_approve_switchover(void);
>  int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
>          bool in_postcopy);

Re: [PATCH v11 09/27] migration: push Error **errp into qemu_loadvm_state_main()

[Fabiano Rosas]

Arun Menon <armenon@redhat.com> writes:

> 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 qemu_loadvm_state_main() must report an error
> in errp, in case of failure.
> loadvm_process_command also sets the errp object explicitly.
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  migration/colo.c   |  5 +++--
>  migration/savevm.c | 36 +++++++++++++++++++-----------------
>  migration/savevm.h |  3 ++-
>  3 files changed, 24 insertions(+), 20 deletions(-)
>
> diff --git a/migration/colo.c b/migration/colo.c
> index 0ba22ee76a13e313793f653f43a728e3c433bbc1..a96e4dba15516b71d1b315c736c3b4879ff04e58 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -659,6 +659,7 @@ void migrate_start_colo_process(MigrationState *s)
>  static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
>                        QEMUFile *fb, QIOChannelBuffer *bioc, Error **errp)
>  {
> +    ERRP_GUARD();

With my suggestion below, this goes away.

>      uint64_t total_size;
>      uint64_t value;
>      Error *local_err = NULL;
> @@ -686,11 +687,11 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
>  
>      bql_lock();
>      cpu_synchronize_all_states();
> -    ret = qemu_loadvm_state_main(mis->from_src_file, mis);
> +    ret = qemu_loadvm_state_main(mis->from_src_file, mis, errp);
>      bql_unlock();
>  
>      if (ret < 0) {
> -        error_setg(errp, "Load VM's live state (ram) error");
> +        error_prepend(errp, "Load VM's live state (ram) error: ");

Another one to leave out. There's enough information downstream
already. Also, this "(ram)" doesn't look right.

>          return;
>      }
>  
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 70e021597d884030c4a0dc2a7bc27d42a7371797..9ec07892cd6ea666431410657c840b6325377d97 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2105,7 +2105,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
>      qemu_file_set_blocking(f, true);
>  
>      /* TODO: sanity check that only postcopiable data will be loaded here */
> -    load_res = qemu_loadvm_state_main(f, mis);
> +    load_res = qemu_loadvm_state_main(f, mis, &error_fatal);
>  
>      /*
>       * This is tricky, but, mis->from_src_file can change after it
> @@ -2407,6 +2407,7 @@ static int loadvm_postcopy_handle_resume(MigrationIncomingState *mis)
>   */
>  static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis, Error **errp)
>  {
> +    ERRP_GUARD();
>      int ret;
>      size_t length;
>      QIOChannelBuffer *bioc;
> @@ -2456,9 +2457,9 @@ static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis, Error **errp)
>          qemu_coroutine_yield();
>      } while (1);
>  
> -    ret = qemu_loadvm_state_main(packf, mis);
> +    ret = qemu_loadvm_state_main(packf, mis, errp);
>      if (ret < 0) {
> -        error_setg(errp, "VM state load failed: %d", ret);
> +        error_prepend(errp, "Loading VM state failed: %d: ", ret);

This is getting out of hand for code review, may I suggest you
artificially trigger these errors, look at the resulting message and
remove all the unnecessary wrapping? Each error_prepend is a candidate
for removal if it will just state "load failed".

Using error_prepend partly defeats the purpose of propagating errp. We
should only use it when there's valuable information to be provided.

>      }
>      trace_loadvm_handle_cmd_packaged_main(ret);
>      qemu_fclose(packf);
> @@ -3080,18 +3081,21 @@ static bool postcopy_pause_incoming(MigrationIncomingState *mis)
>      return true;
>  }
>  
> -int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> +int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis,
> +                           Error **errp)
>  {
> +    ERRP_GUARD();
>      uint8_t section_type;
>      int ret = 0;
> -    Error *local_err = NULL;
>  
>  retry:
>      while (true) {
>          section_type = qemu_get_byte(f);
>  
> -        ret = qemu_file_get_error_obj_any(f, mis->postcopy_qemufile_dst, NULL);
> +        ret = qemu_file_get_error_obj_any(f, mis->postcopy_qemufile_dst, errp);
>          if (ret) {
> +            error_prepend(errp, "Failed to load device state section ID: %d: ",
> +                          ret);

We could drop some extra words here, the term 'section' is already quite
representative.

"Failed to load section ID: stream error: %d: "


>              break;
>          }
>  
> @@ -3112,10 +3116,7 @@ retry:
>              }
>              break;
>          case QEMU_VM_COMMAND:
> -            ret = loadvm_process_command(f, &local_err);
> -            if (ret < 0) {
> -                warn_report_err(local_err);
> -            }
> +            ret = loadvm_process_command(f, errp);

Good.

>              trace_qemu_loadvm_state_section_command(ret);
>              if ((ret < 0) || (ret == LOADVM_QUIT)) {
>                  goto out;
> @@ -3125,7 +3126,7 @@ retry:
>              /* This is the end of migration */
>              goto out;
>          default:
> -            error_report("Unknown savevm section type %d", section_type);
> +            error_setg(errp, "Unknown savevm section type %d", section_type);

Not sure if they're referring to "savevm" here as a generic term for
vmstate/migration or if it was intended to say: "savevm wrote a section
type that this loadvm instance doesn't understand".

Since you're here, could you fix this? Migration errors from source and
destination are often interleaved in logs, we don't want to see the
"savevm" word in a destination-side error message. Just put a small note
in the commit message, no need for another patch.

>              ret = -EINVAL;
>              goto out;
>          }
> @@ -3133,6 +3134,9 @@ retry:
>  
>  out:
>      if (ret < 0) {
> +        if (*errp == NULL) {
> +            error_setg(errp, "Loading VM state failed: %d", ret);
> +        }

Another candidate for removal, then we avoid having to dereference errp.

>          qemu_file_set_error(f, ret);
>  
>          /* Cancel bitmaps incoming regardless of recovery */
> @@ -3153,6 +3157,7 @@ out:
>              migrate_postcopy_ram() && postcopy_pause_incoming(mis)) {
>              /* Reset f to point to the newly created channel */
>              f = mis->from_src_file;
> +            error_free_or_abort(errp);

What's this about?

>              goto retry;
>          }
>      }
> @@ -3186,10 +3191,7 @@ int qemu_loadvm_state(QEMUFile *f, Error **errp)
>  
>      cpu_synchronize_all_pre_loadvm();
>  
> -    ret = qemu_loadvm_state_main(f, mis);
> -    if (ret < 0) {
> -        error_setg(errp, "Load VM state failed: %d", ret);
> -    }
> +    ret = qemu_loadvm_state_main(f, mis, errp);
>      qemu_event_set(&mis->main_thread_load_event);
>  
>      trace_qemu_loadvm_state_post_main(ret);
> @@ -3270,9 +3272,9 @@ int qemu_load_device_state(QEMUFile *f, Error **errp)
>      int ret;
>  
>      /* Load QEMU_VM_SECTION_FULL section */
> -    ret = qemu_loadvm_state_main(f, mis);
> +    ret = qemu_loadvm_state_main(f, mis, errp);
>      if (ret < 0) {
> -        error_setg(errp, "Failed to load device state: %d", ret);
> +        error_prepend(errp, "Failed to load device state: %d: ", ret);
>          return ret;
>      }
>  
> diff --git a/migration/savevm.h b/migration/savevm.h
> index b12681839f0b1afa3255e45215d99c13a224b19f..c337e3e3d111a7f28a57b90f61e8f70b71803d4e 100644
> --- a/migration/savevm.h
> +++ b/migration/savevm.h
> @@ -66,7 +66,8 @@ int qemu_save_device_state(QEMUFile *f);
>  
>  int qemu_loadvm_state(QEMUFile *f, Error **errp);
>  void qemu_loadvm_state_cleanup(MigrationIncomingState *mis);
> -int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis);
> +int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis,
> +                           Error **errp);
>  int qemu_load_device_state(QEMUFile *f, Error **errp);
>  int qemu_loadvm_approve_switchover(void);
>  int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,

Re: [PATCH v11 10/27] migration: push Error **errp into qemu_loadvm_section_start_full()

[Fabiano Rosas]

Arun Menon <armenon@redhat.com> writes:

> 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 qemu_loadvm_section_start_full() must report an error
> in errp, in case of failure.
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  migration/savevm.c | 38 ++++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 9ec07892cd6ea666431410657c840b6325377d97..77408347c1f1ca7eb3a04f8f130c20a5a81f6db2 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2724,21 +2724,21 @@ static bool check_section_footer(QEMUFile *f, SaveStateEntry *se)
>  }
>  
>  static int
> -qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
> +qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type, Error **errp)
>  {
> +    ERRP_GUARD();
>      bool trace_downtime = (type == QEMU_VM_SECTION_FULL);
>      uint32_t instance_id, version_id, section_id;
>      int64_t start_ts, end_ts;
>      SaveStateEntry *se;
>      char idstr[256];
>      int ret;
> -    Error *local_err = NULL;
>  
>      /* Read section start */
>      section_id = qemu_get_be32(f);
>      if (!qemu_get_counted_string(f, idstr)) {
> -        error_report("Unable to read ID string for section %u",
> -                     section_id);
> +        error_setg(errp, "Unable to read ID string for section %u",
> +                   section_id);
>          return -EINVAL;
>      }
>      instance_id = qemu_get_be32(f);
> @@ -2746,8 +2746,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
>  
>      ret = qemu_file_get_error(f);
>      if (ret) {
> -        error_report("%s: Failed to read instance/version ID: %d",
> -                     __func__, ret);
> +        error_setg(errp, "Failed to read instance/version ID: %d", ret);
>          return ret;
>      }
>  
> @@ -2756,17 +2755,17 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
>      /* Find savevm section */
>      se = find_se(idstr, instance_id);
>      if (se == NULL) {
> -        error_report("Unknown savevm section or instance '%s' %"PRIu32". "
> -                     "Make sure that your current VM setup matches your "
> -                     "saved VM setup, including any hotplugged devices",
> -                     idstr, instance_id);
> +        error_setg(errp, "Unknown savevm section or instance '%s' %"PRIu32". "

Drop the "savevm" please.

> +                   "Make sure that your current VM setup matches your "
> +                   "saved VM setup, including any hotplugged devices",
> +                   idstr, instance_id);
>          return -EINVAL;
>      }
>  
>      /* Validate version */
>      if (version_id > se->version_id) {
> -        error_report("savevm: unsupported version %d for '%s' v%d",
> -                     version_id, idstr, se->version_id);
> +        error_setg(errp, "savevm: unsupported version %d for '%s' v%d",

Same.

> +                   version_id, idstr, se->version_id);
>          return -EINVAL;
>      }
>      se->load_version_id = version_id;
> @@ -2774,7 +2773,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
>  
>      /* Validate if it is a device's state */
>      if (xen_enabled() && se->is_ram) {
> -        error_report("loadvm: %s RAM loading not allowed on Xen", idstr);
> +        error_setg(errp, "loadvm: %s RAM loading not allowed on Xen", idstr);
>          return -EINVAL;
>      }
>  
> @@ -2782,11 +2781,11 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
>          start_ts = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
>      }
>  
> -    ret = vmstate_load(f, se, &local_err);
> +    ret = vmstate_load(f, se, errp);
>      if (ret < 0) {
> -        error_report("error while loading state for instance 0x%"PRIx32" of"
> -                     " device '%s'", instance_id, idstr);
> -        warn_report_err(local_err);
> +        error_prepend(errp,
> +                      "error while loading state for instance 0x%"PRIx32" of"
> +                      " device '%s': ", instance_id, idstr);
>          return ret;
>      }
>  
> @@ -2797,6 +2796,9 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
>      }
>  
>      if (!check_section_footer(f, se)) {
> +        error_setg(errp, "Reading footer section of instance "
> +                   "0x%"PRIx32" of device '%s' for version_id: %d failed",
> +                   instance_id, idstr, version_id);

check_section_footer() already has messages saying something went wrong
with the footer. Make sure you're not duplicating information. If
necessary, trim it either here or there.

>          return -EINVAL;
>      }
>  
> @@ -3103,7 +3105,7 @@ retry:
>          switch (section_type) {
>          case QEMU_VM_SECTION_START:
>          case QEMU_VM_SECTION_FULL:
> -            ret = qemu_loadvm_section_start_full(f, section_type);
> +            ret = qemu_loadvm_section_start_full(f, section_type, errp);
>              if (ret < 0) {
>                  goto out;
>              }

Re: [PATCH v11 11/27] migration: push Error **errp into qemu_loadvm_section_part_end()

[Fabiano Rosas]

Arun Menon <armenon@redhat.com> writes:

> 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 qemu_loadvm_section_part_end() must report an error
> in errp, in case of failure.
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  migration/savevm.c | 23 ++++++++++-------------
>  1 file changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 77408347c1f1ca7eb3a04f8f130c20a5a81f6db2..ff2e4f75e070d0f452414f28435905928b1480a7 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2806,21 +2806,20 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type, Error **errp)
>  }
>  
>  static int
> -qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
> +qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type, Error **errp)
>  {
> +    ERRP_GUARD();
>      bool trace_downtime = (type == QEMU_VM_SECTION_END);
>      int64_t start_ts, end_ts;
>      uint32_t section_id;
>      SaveStateEntry *se;
>      int ret;
> -    Error *local_err = NULL;
>  
>      section_id = qemu_get_be32(f);
>  
>      ret = qemu_file_get_error(f);
>      if (ret) {
> -        error_report("%s: Failed to read section ID: %d",
> -                     __func__, ret);
> +        error_setg(errp, "Failed to read section ID: %d", ret);
>          return ret;
>      }
>  
> @@ -2831,7 +2830,7 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
>          }
>      }
>      if (se == NULL) {
> -        error_report("Unknown savevm section %d", section_id);
> +        error_setg(errp, "Unknown savevm section %d", section_id);

Drop "savevm" please.

>          return -EINVAL;
>      }
>  
> @@ -2839,11 +2838,10 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
>          start_ts = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
>      }
>  
> -    ret = vmstate_load(f, se, &local_err);
> +    ret = vmstate_load(f, se, errp);
>      if (ret < 0) {
> -        error_report("error while loading state section id %d(%s)",
> -                     section_id, se->idstr);
> -        warn_report_err(local_err);
> +        error_prepend(errp, "error while loading state section id %d(%s): ",
> +                      section_id, se->idstr);
>          return ret;
>      }
>  
> @@ -2854,6 +2852,8 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
>      }
>  
>      if (!check_section_footer(f, se)) {
> +        error_setg(errp, "Check section footer error, section_id: %d",

This became not very grammatical, maybe drop the "Check" word.

> +                   section_id);
>          return -EINVAL;
>      }
>  
> @@ -3112,7 +3112,7 @@ retry:
>              break;
>          case QEMU_VM_SECTION_PART:
>          case QEMU_VM_SECTION_END:
> -            ret = qemu_loadvm_section_part_end(f, section_type);
> +            ret = qemu_loadvm_section_part_end(f, section_type, errp);
>              if (ret < 0) {
>                  goto out;
>              }
> @@ -3136,9 +3136,6 @@ retry:
>  
>  out:
>      if (ret < 0) {
> -        if (*errp == NULL) {
> -            error_setg(errp, "Loading VM state failed: %d", ret);
> -        }

Good. Could have been mentioned in that patch's commit message, or even
a /* temporary */ comment in the code.

>          qemu_file_set_error(f, ret);
>  
>          /* Cancel bitmaps incoming regardless of recovery */

Re: [PATCH v11 14/27] migration: push Error **errp into ram_postcopy_incoming_init()

[Fabiano Rosas]

Arun Menon <armenon@redhat.com> writes:

> 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 ram_postcopy_incoming_init() must report an error
> in errp, in case of failure.
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v11 15/27] migration: push Error **errp into loadvm_postcopy_handle_advise()

[Fabiano Rosas]

Arun Menon <armenon@redhat.com> writes:

> 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 loadvm_postcopy_handle_advise() must report an error
> in errp, in case of failure.
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v11 16/27] migration: push Error **errp into loadvm_postcopy_handle_listen()

[Fabiano Rosas]

Arun Menon <armenon@redhat.com> writes:

> 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 loadvm_postcopy_handle_listen() must report an error
> in errp, in case of failure.
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v11 17/27] migration: push Error **errp into loadvm_postcopy_handle_run()

[Fabiano Rosas]

Arun Menon <armenon@redhat.com> writes:

> 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 loadvm_postcopy_handle_run() must report an error
> in errp, in case of failure.
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v11 18/27] migration: push Error **errp into loadvm_postcopy_ram_handle_discard()

[Fabiano Rosas]

Arun Menon <armenon@redhat.com> writes:

> 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 loadvm_postcopy_ram_handle_discard() must report an error
> in errp, in case of failure.
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v11 19/27] migration: push Error **errp into loadvm_handle_recv_bitmap()

[Fabiano Rosas]

Arun Menon <armenon@redhat.com> writes:

> 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 loadvm_handle_recv_bitmap() must report an error
> in errp, in case of failure.
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  migration/savevm.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 9098c4bd3394d7b9ed77e20afbb26fd9c9be6550..a7aede1b3df9164e322e68f3889df7c4166876f5 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2480,32 +2480,35 @@ static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis, Error **errp)
>   * len (1 byte) + ramblock_name (<255 bytes)
>   */
>  static int loadvm_handle_recv_bitmap(MigrationIncomingState *mis,
> -                                     uint16_t len)
> +                                     uint16_t len, Error **errp)
>  {
>      QEMUFile *file = mis->from_src_file;
>      RAMBlock *rb;
>      char block_name[256];
>      size_t cnt;
> +    int ret;
>  
>      cnt = qemu_get_counted_string(file, block_name);
>      if (!cnt) {
> -        error_report("%s: failed to read block name", __func__);
> +        error_setg(errp, "failed to read block name: %s", block_name);

Could we not print the buffer that's just failed to be written? As a
matter of principle =)

>          return -EINVAL;
>      }
>  
>      /* Validate before using the data */
> -    if (qemu_file_get_error(file)) {
> -        return qemu_file_get_error(file);
> +    ret = qemu_file_get_error(file);
> +    if (ret < 0) {
> +        error_setg(errp, "migration stream has error: %d", ret);

I've been suggesting "stream error:", probably best to keep it uniform.

> +        return ret;
>      }
>  
>      if (len != cnt + 1) {
> -        error_report("%s: invalid payload length (%d)", __func__, len);
> +        error_setg(errp, "invalid payload length (%d)", len);
>          return -EINVAL;
>      }
>  
>      rb = qemu_ram_block_by_name(block_name);
>      if (!rb) {
> -        error_report("%s: block '%s' not found", __func__, block_name);
> +        error_setg(errp, "block '%s' not found", block_name);
>          return -EINVAL;
>      }
>  
> @@ -2642,11 +2645,7 @@ static int loadvm_process_command(QEMUFile *f, Error **errp)
>          return 0;
>  
>      case MIG_CMD_RECV_BITMAP:
> -        ret = loadvm_handle_recv_bitmap(mis, len);
> -        if (ret < 0) {
> -            error_setg(errp, "Failed to load device state command: %d", ret);
> -        }
> -        return ret;
> +        return loadvm_handle_recv_bitmap(mis, len, errp);
>  
>      case MIG_CMD_ENABLE_COLO:
>          ret = loadvm_process_enable_colo(mis);

Re: [PATCH v11 20/27] migration: Return -1 on memory allocation failure in ram.c

[Fabiano Rosas]

Arun Menon <armenon@redhat.com> writes:

> The function colo_init_ram_cache() currently returns -errno if
> qemu_anon_ram_alloc() fails. However, the subsequent cleanup loop that
> calls qemu_anon_ram_free() could potentially alter the value of errno.
> This would cause the function to return a value that does not accurately
> represent the original allocation failure.
>
> This commit changes the return value to -1 on memory allocation failure.
> This ensures that the return value is consistent and is not affected by
> any errno changes that may occur during the free process.
>
> Signed-off-by: Arun Menon <armenon@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v11 21/27] migration: push Error **errp into loadvm_process_enable_colo()

[Fabiano Rosas]

Arun Menon <armenon@redhat.com> writes:

> 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 loadvm_process_enable_colo() must report an error
> in errp, in case of failure.
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v11 22/27] migration: push Error **errp into loadvm_postcopy_handle_switchover_start()

[Fabiano Rosas]

Arun Menon <armenon@redhat.com> writes:

> 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 loadvm_postcopy_handle_switchover_start() must report
> an error in errp, in case of failure.
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v11 23/27] migration: Capture error in postcopy_ram_listen_thread()

[Fabiano Rosas]

Arun Menon <armenon@redhat.com> writes:

> This is an incremental step in converting vmstate loading
> code to report error via Error objects instead of directly
> printing it to console/monitor.
> postcopy_ram_listen_thread() calls qemu_loadvm_state_main()
> to load the vm, and in case of a failure, it should set the error
> in the migration object.
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  migration/savevm.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index de2bce276faa863a0f25deedafb0b784f10559d7..b85620a03654c214f4e771fa3b2bcfdf48661214 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2095,6 +2095,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
>      QEMUFile *f = mis->from_src_file;
>      int load_res;
>      MigrationState *migr = migrate_get_current();
> +    Error *local_err = NULL;
>  
>      object_ref(OBJECT(migr));
>  
> @@ -2111,7 +2112,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
>      qemu_file_set_blocking(f, true);
>  
>      /* TODO: sanity check that only postcopiable data will be loaded here */
> -    load_res = qemu_loadvm_state_main(f, mis, &error_fatal);
> +    load_res = qemu_loadvm_state_main(f, mis, &local_err);
>  
>      /*
>       * This is tricky, but, mis->from_src_file can change after it
> @@ -2137,9 +2138,12 @@ static void *postcopy_ram_listen_thread(void *opaque)
>                           __func__, load_res);
>              load_res = 0; /* prevent further exit() */
>          } else {
> -            error_report("%s: loadvm failed: %d", __func__, load_res);
> +            error_prepend(&local_err,
> +                          "loadvm failed during postcopy: %d: ", load_res);
> +            migrate_set_error(migr, local_err);
> +            error_report_err(local_err);
>              migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> -                                           MIGRATION_STATUS_FAILED);
> +                              MIGRATION_STATUS_FAILED);

This should be left alone. Having this patch (git) conflict with
something else just because of this line would be really annoying.

>          }
>      }
>      if (load_res >= 0) {

Re: [PATCH v11 01/27] migration: push Error **errp into vmstate_subsection_load()

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> Arun Menon <armenon@redhat.com> writes:
>
>> 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_subsection_load() must report an error
>> in errp, in case of failure.
>>
>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Signed-off-by: Arun Menon <armenon@redhat.com>
>> ---
>>  migration/vmstate.c | 11 ++++++++---
>>  1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/migration/vmstate.c b/migration/vmstate.c
>> index 5feaa3244d259874f03048326b2497e7db32e47c..6108c7fe283a5013ce42ea9987723c489aef26a2 100644
>> --- a/migration/vmstate.c
>> +++ b/migration/vmstate.c
>> @@ -25,7 +25,7 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
>>                                     void *opaque, JSONWriter *vmdesc,
>>                                     Error **errp);
>>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>> -                                   void *opaque);
>> +                                   void *opaque, Error **errp);
>>  
>>  /* Whether this field should exist for either save or load the VM? */
>>  static bool
>> @@ -225,7 +225,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>>          field++;
>>      }
>>      assert(field->flags == VMS_END);
>> -    ret = vmstate_subsection_load(f, vmsd, opaque);
>> +    ret = vmstate_subsection_load(f, vmsd, opaque, &error_fatal);
>>      if (ret != 0) {
>>          qemu_file_set_error(f, ret);
>>          return ret;
>
> This is now unreachable, no?
>

Also, this temporary &error_fatal here and throughout the series will
break bisect badly, won't it? Imagine we have a bug in the code past
this point (once the future patch from this series removes the
&error_fatal), now every time this commit shows up in bisect, it will
abort earlier.

I get that having error_fatal here helps ensure the series is correct,
but maybe we should do without it.

Do others have an opinion here?

>> @@ -566,7 +566,7 @@ vmstate_get_subsection(const VMStateDescription * const *sub,
>>  }
>>  
>>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>> -                                   void *opaque)
>> +                                   void *opaque, Error **errp)
>>  {
>>      trace_vmstate_subsection_load(vmsd->name);
>>  
>> @@ -598,6 +598,8 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>>          sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
>>          if (sub_vmsd == NULL) {
>>              trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(lookup)");
>> +            error_setg(errp, "VM subsection '%s' in '%s' does not exist",
>> +                       idstr, vmsd->name);
>>              return -ENOENT;
>>          }
>>          qemu_file_skip(f, 1); /* subsection */
>> @@ -608,6 +610,9 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
>>          ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
>>          if (ret) {
>>              trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(child)");
>> +            error_setg(errp,
>> +                       "Loading VM subsection '%s' in '%s' failed: %d",
>> +                       idstr, vmsd->name, ret);
>>              return ret;
>>          }
>>      }

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

[Arun Menon]

Hi Fabiano,

Thanks for the review.

On Fri, Aug 15, 2025 at 12:41:50PM -0300, Fabiano Rosas wrote:
> Arun Menon <armenon@redhat.com> writes:
> 
> > 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.
> >
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Signed-off-by: Arun Menon <armenon@redhat.com>
> > ---
> >  hw/display/virtio-gpu.c     |  2 +-
> >  hw/pci/pci.c                |  3 ++-
> >  hw/s390x/virtio-ccw.c       |  2 +-
> >  hw/scsi/spapr_vscsi.c       |  2 +-
> >  hw/vfio/pci.c               |  3 ++-
> >  hw/virtio/virtio-mmio.c     |  3 ++-
> >  hw/virtio/virtio-pci.c      |  2 +-
> >  hw/virtio/virtio.c          |  4 +--
> >  include/migration/vmstate.h |  2 +-
> >  migration/cpr.c             |  5 ++--
> >  migration/savevm.c          |  6 +++--
> >  migration/vmstate-types.c   | 22 ++++++++++++----
> >  migration/vmstate.c         | 61 ++++++++++++++++++++++++++++++-------------
> >  tests/unit/test-vmstate.c   | 63 ++++++++++++++++++++++++++++++++++++++-------
> >  ui/vdagent.c                |  3 ++-
> >  15 files changed, 136 insertions(+), 47 deletions(-)
> >
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> > index 0a1a625b0ea6cf26cb0d799171a57ed3d3ab2442..5dc31bc6bfb0272e29a4364ab10de2595a4bedf7 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -1343,7 +1343,7 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
> >      }
> >  
> >      /* load & apply scanout state */
> > -    vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1);
> > +    vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, &error_fatal);
> >  
> >      return 0;
> >  }
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index c70b5ceebaf1f2b10768bd030526cbb518da2b8d..6be932d3bb67ff0c4808707db2a7b6378a90e82b 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -934,7 +934,8 @@ void pci_device_save(PCIDevice *s, QEMUFile *f)
> >  int pci_device_load(PCIDevice *s, QEMUFile *f)
> >  {
> >      int ret;
> > -    ret = vmstate_load_state(f, &vmstate_pci_device, s, s->version_id);
> > +    ret = vmstate_load_state(f, &vmstate_pci_device, s, s->version_id,
> > +                             &error_fatal);
> >      /* Restore the interrupt status bit. */
> >      pci_update_irq_status(s);
> >      return ret;
> > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> > index d2f85b39f30f7fc82e0c600144c0a958e1269b2c..6a9641a03d5d3a38a4de7ceb9deffc0cc303bcff 100644
> > --- a/hw/s390x/virtio-ccw.c
> > +++ b/hw/s390x/virtio-ccw.c
> > @@ -1136,7 +1136,7 @@ static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f)
> >  static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
> >  {
> >      VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
> > -    return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1);
> > +    return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1, &error_fatal);
> >  }
> >  
> >  static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
> > diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
> > index 20f70fb2729de78b9636a6b8c869695dab4f8902..8c896022d324f51962605288d6d6df1648c83cde 100644
> > --- a/hw/scsi/spapr_vscsi.c
> > +++ b/hw/scsi/spapr_vscsi.c
> > @@ -648,7 +648,7 @@ static void *vscsi_load_request(QEMUFile *f, SCSIRequest *sreq)
> >      assert(!req->active);
> >  
> >      memset(req, 0, sizeof(*req));
> > -    rc = vmstate_load_state(f, &vmstate_spapr_vscsi_req, req, 1);
> > +    rc = vmstate_load_state(f, &vmstate_spapr_vscsi_req, req, 1, &error_fatal);
> >      if (rc) {
> >          fprintf(stderr, "VSCSI: failed loading request tag#%u\n", sreq->tag);
> >          return NULL;
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index 4fa692c1a32bcfa4e4939e5fcb64f2bf19905b3b..0be54762cdcbdb4780b8228b0bdf7fc6bd74dd57 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -2795,7 +2795,8 @@ static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
> >          old_addr[bar] = pdev->io_regions[bar].addr;
> >      }
> >  
> > -    ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1);
> > +    ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1,
> > +                             &error_fatal);
> >      if (ret) {
> >          return ret;
> >      }
> > diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> > index 532c67107ba1d2978a76cf49f9cdc1de1dea3e11..0a688909fc606a3c9fde933667ae8c309ab527d0 100644
> > --- a/hw/virtio/virtio-mmio.c
> > +++ b/hw/virtio/virtio-mmio.c
> > @@ -34,6 +34,7 @@
> >  #include "qemu/error-report.h"
> >  #include "qemu/log.h"
> >  #include "trace.h"
> > +#include "qapi/error.h"
> >  
> >  static bool virtio_mmio_ioeventfd_enabled(DeviceState *d)
> >  {
> > @@ -619,7 +620,7 @@ static int virtio_mmio_load_extra_state(DeviceState *opaque, QEMUFile *f)
> >  {
> >      VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
> >  
> > -    return vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1);
> > +    return vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1, &error_fatal);
> >  }
> >  
> >  static bool virtio_mmio_has_extra_state(DeviceState *opaque)
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index 767216d795998708f5716a23ae16c79cd90ff489..b04faa1e5c91b5cef40e54ec41d92422d16bfc13 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -161,7 +161,7 @@ static int virtio_pci_load_extra_state(DeviceState *d, QEMUFile *f)
> >  {
> >      VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
> >  
> > -    return vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1);
> > +    return vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1, &error_fatal);
> >  }
> >  
> >  static void virtio_pci_save_queue(DeviceState *d, int n, QEMUFile *f)
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 9a81ad912e013fc254899c4e55cff1f76a6112a4..6bcafb338d1b5becadcacf092ba33a6ae4c3d194 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -3327,14 +3327,14 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
> >      }
> >  
> >      if (vdc->vmsd) {
> > -        ret = vmstate_load_state(f, vdc->vmsd, vdev, version_id);
> > +        ret = vmstate_load_state(f, vdc->vmsd, vdev, version_id, &error_fatal);
> >          if (ret) {
> >              return ret;
> >          }
> >      }
> >  
> >      /* Subsections */
> > -    ret = vmstate_load_state(f, &vmstate_virtio, vdev, 1);
> > +    ret = vmstate_load_state(f, &vmstate_virtio, vdev, 1, &error_fatal);
> >      if (ret) {
> >          return ret;
> >      }
> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > index 1ff7bd9ac425ba67cd5ca7ad97bcf570f9e19abe..056781b1c21e737583f081594d9f88b32adfd674 100644
> > --- a/include/migration/vmstate.h
> > +++ b/include/migration/vmstate.h
> > @@ -1196,7 +1196,7 @@ extern const VMStateInfo vmstate_info_qlist;
> >      }
> >  
> >  int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> > -                       void *opaque, int version_id);
> > +                       void *opaque, int version_id, Error **errp);
> >  int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> >                         void *opaque, JSONWriter *vmdesc);
> >  int vmstate_save_state_with_err(QEMUFile *f, const VMStateDescription *vmsd,
> > diff --git a/migration/cpr.c b/migration/cpr.c
> > index 42ad0b0d500e5de57faf0c6517e216b2d1c0cacf..bdb24736f44e91ba59b6e622a315597c97e7f64d 100644
> > --- a/migration/cpr.c
> > +++ b/migration/cpr.c
> > @@ -202,6 +202,7 @@ int cpr_state_save(MigrationChannel *channel, Error **errp)
> >  
> >  int cpr_state_load(MigrationChannel *channel, Error **errp)
> >  {
> > +    ERRP_GUARD();
> >      int ret;
> >      uint32_t v;
> >      QEMUFile *f;
> > @@ -233,9 +234,9 @@ int cpr_state_load(MigrationChannel *channel, Error **errp)
> >          return -ENOTSUP;
> >      }
> >  
> > -    ret = vmstate_load_state(f, &vmstate_cpr_state, &cpr_state, 1);
> > +    ret = vmstate_load_state(f, &vmstate_cpr_state, &cpr_state, 1, errp);
> >      if (ret) {
> > -        error_setg(errp, "vmstate_load_state error %d", ret);
> > +        error_prepend(errp, "vmstate_load_state error %d: ", ret);
> >          qemu_fclose(f);
> >          return ret;
> >      }
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index fabbeb296ae987d0c06ba6dafda63720205fecfd..cb64f2855d46aaa7c617b3e4079a2c9e566079b2 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -969,7 +969,8 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se)
> >      if (!se->vmsd) {         /* Old style */
> >          return se->ops->load_state(f, se->opaque, se->load_version_id);
> >      }
> > -    return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id);
> > +    return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id,
> > +                              &error_fatal);
> >  }
> >  
> >  static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
> > @@ -2839,7 +2840,8 @@ static int qemu_loadvm_state_header(QEMUFile *f)
> >              error_report("Configuration section missing");
> >              return -EINVAL;
> >          }
> > -        ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0);
> > +        ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0,
> > +                                 &error_fatal);
> >  
> >          if (ret) {
> >              return ret;
> > diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> > index 741a588b7e18c6d37724b08a0101edc8bc74a0a5..f41670cc853c5b41ccc8def354886a8e5c1451fd 100644
> > --- a/migration/vmstate-types.c
> > +++ b/migration/vmstate-types.c
> > @@ -19,6 +19,7 @@
> >  #include "qemu/error-report.h"
> >  #include "qemu/queue.h"
> >  #include "trace.h"
> > +#include "qapi/error.h"
> >  
> >  /* bool */
> >  
> > @@ -543,13 +544,17 @@ static int get_tmp(QEMUFile *f, void *pv, size_t size,
> >                     const VMStateField *field)
> >  {
> >      int ret;
> > +    Error *local_err = NULL;
> >      const VMStateDescription *vmsd = field->vmsd;
> >      int version_id = field->version_id;
> >      void *tmp = g_malloc(size);
> >  
> >      /* Writes the parent field which is at the start of the tmp */
> >      *(void **)tmp = pv;
> > -    ret = vmstate_load_state(f, vmsd, tmp, version_id);
> > +    ret = vmstate_load_state(f, vmsd, tmp, version_id, &local_err);
> > +    if (ret < 0) {
> > +        warn_report_err(local_err);
> > +    }
> >      g_free(tmp);
> >      return ret;
> >  }
> > @@ -626,6 +631,7 @@ static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size,
> >                        const VMStateField *field)
> >  {
> >      int ret = 0;
> > +    Error *local_err = NULL;
> >      const VMStateDescription *vmsd = field->vmsd;
> >      /* size of a QTAILQ element */
> >      size_t size = field->size;
> > @@ -649,8 +655,9 @@ static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size,
> >  
> >      while (qemu_get_byte(f)) {
> >          elm = g_malloc(size);
> > -        ret = vmstate_load_state(f, vmsd, elm, version_id);
> > +        ret = vmstate_load_state(f, vmsd, elm, version_id, &local_err);
> >          if (ret) {
> > +            warn_report_err(local_err);
> >              return ret;
> >          }
> >          QTAILQ_RAW_INSERT_TAIL(pv, elm, entry_offset);
> > @@ -772,6 +779,7 @@ static int get_gtree(QEMUFile *f, void *pv, size_t unused_size,
> >      GTree *tree = *pval;
> >      void *key, *val;
> >      int ret = 0;
> > +    Error *local_err = NULL;
> >  
> >      /* in case of direct key, the key vmsd can be {}, ie. check fields */
> >      if (!direct_key && version_id > key_vmsd->version_id) {
> > @@ -803,18 +811,20 @@ static int get_gtree(QEMUFile *f, void *pv, size_t unused_size,
> >              key = (void *)(uintptr_t)qemu_get_be64(f);
> >          } else {
> >              key = g_malloc0(key_size);
> > -            ret = vmstate_load_state(f, key_vmsd, key, version_id);
> > +            ret = vmstate_load_state(f, key_vmsd, key, version_id, &local_err);
> >              if (ret) {
> >                  error_report("%s : failed to load %s (%d)",
> >                               field->name, key_vmsd->name, ret);
> > +                warn_report_err(local_err);
> >                  goto key_error;
> >              }
> >          }
> >          val = g_malloc0(val_size);
> > -        ret = vmstate_load_state(f, val_vmsd, val, version_id);
> > +        ret = vmstate_load_state(f, val_vmsd, val, version_id, &local_err);
> >          if (ret) {
> >              error_report("%s : failed to load %s (%d)",
> >                           field->name, val_vmsd->name, ret);
> > +            warn_report_err(local_err);
> >              goto val_error;
> >          }
> >          g_tree_insert(tree, key, val);
> > @@ -872,6 +882,7 @@ static int get_qlist(QEMUFile *f, void *pv, size_t unused_size,
> >                       const VMStateField *field)
> >  {
> >      int ret = 0;
> > +    Error *local_err = NULL;
> >      const VMStateDescription *vmsd = field->vmsd;
> >      /* size of a QLIST element */
> >      size_t size = field->size;
> > @@ -892,10 +903,11 @@ static int get_qlist(QEMUFile *f, void *pv, size_t unused_size,
> >  
> >      while (qemu_get_byte(f)) {
> >          elm = g_malloc(size);
> > -        ret = vmstate_load_state(f, vmsd, elm, version_id);
> > +        ret = vmstate_load_state(f, vmsd, elm, version_id, &local_err);
> >          if (ret) {
> >              error_report("%s: failed to load %s (%d)", field->name,
> >                           vmsd->name, ret);
> > +            warn_report_err(local_err);
> >              g_free(elm);
> >              return ret;
> >          }
> > diff --git a/migration/vmstate.c b/migration/vmstate.c
> > index 6108c7fe283a5013ce42ea9987723c489aef26a2..1cd609a1d598995af1e51d1f4d58d68133f1426d 100644
> > --- a/migration/vmstate.c
> > +++ b/migration/vmstate.c
> > @@ -132,29 +132,34 @@ 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)
> >  {
> > +    ERRP_GUARD();
> >      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 for: '%s', "
> 
> "VM" pre load is a little ambiguous. Simply "pre load" or "pre load
> hook" is better.
Sure, will do this.
> 
> > +                       "version_id: %d, minimum version_id: %d, ret: %d",
> > +                       vmsd->name, vmsd->version_id, vmsd->minimum_version_id,
> > +                       ret);
> >              return ret;
> >          }
> >      }
> > @@ -192,13 +197,20 @@ 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);
> > +                    if (ret < 0) {
> > +                        error_setg(errp,
> > +                                   "Failed to get info for %s: %d",
> > +                                   inner_field->name, ret);
> 
> "get info" is not correct. This is the type-specific getter
> invocation. Because the migration (for the most part) is a stream, each
> type provides it's own getter which knows about the size of the field
> and any particularities such as magic values. So:
> 
> error_setg(errp, "Failed to load element of type %s for %s: %d",
>            info->name, inner_field->name, ret);
yes, will change the message. Thanks.
> 
> > +                    }
> >                  }
> >  
> >                  /* If we used a fake temp field.. free it now */
> > @@ -208,30 +220,42 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> >  
> >                  if (ret >= 0) {
> >                      ret = qemu_file_get_error(f);
> > +                    if (ret < 0) {
> > +                        error_setg(errp, "Failed to load %s state: %d",
> > +                                   vmsd->name, ret);
> 
> We could go a little more specific here, it's useful to know whether the
> error was on the transport side, rather than something logical with the
> migrated data. I don't really care about the actual string, but one
> suggestion is "stream error":
> 
> error_setg(errp, "Failed to load %s state: stream error: %d",
>            vmsd->name, ret);
yes, will add stream error. Thanks.
> 
> > +                    }
> >                  }
> >                  if (ret < 0) {
> >                      qemu_file_set_error(f, ret);
> > -                    error_report("Failed to load %s:%s", vmsd->name,
> > -                                 field->name);
> > +                    error_prepend(errp,
> > +                                  "Failed to load %s:%s version_id: %d: ",
> 
> Usage of : is inconsistent with below /
Since we used error_prepend here, we need an extra space after the colon ':'
That way the next error string looks readable.
> 
> > +                                   vmsd->name, field->name, vmsd->version_id);
> >                      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 version_id: %d",
> 
> here
> 
> > +                       vmsd->name, field->name, vmsd->version_id);
> >              return -1;
> >          }
> >          field++;
> >      }
> >      assert(field->flags == VMS_END);
> > -    ret = vmstate_subsection_load(f, vmsd, opaque, &error_fatal);
> > +    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 for: %s, version_id: %d, "
> > +                       "minimum_version: %d, ret: %d",
> > +                       vmsd->name, vmsd->version_id, vmsd->minimum_version_id,
> > +                       ret);
> > +        }
> >      }
> >      trace_vmstate_load_state_end(vmsd->name, "end", ret);
> >      return ret;
> > @@ -568,6 +592,7 @@ vmstate_get_subsection(const VMStateDescription * const *sub,
> >  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
> >                                     void *opaque, Error **errp)
> >  {
> > +    ERRP_GUARD();
> >      trace_vmstate_subsection_load(vmsd->name);
> >  
> >      while (qemu_peek_byte(f, 0) == QEMU_VM_SUBSECTION) {
> > @@ -607,12 +632,12 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
> >          qemu_file_skip(f, len); /* idstr */
> >          version_id = qemu_get_be32(f);
> >  
> > -        ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
> > +        ret = vmstate_load_state(f, sub_vmsd, opaque, version_id, errp);
> >          if (ret) {
> >              trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(child)");
> > -            error_setg(errp,
> > -                       "Loading VM subsection '%s' in '%s' failed: %d",
> > -                       idstr, vmsd->name, ret);
> > +            error_prepend(errp,
> > +                          "Loading VM subsection '%s' in '%s' failed: %d: ",
> > +                          idstr, vmsd->name, ret);
> >              return ret;
> >          }
> >      }
> > diff --git a/tests/unit/test-vmstate.c b/tests/unit/test-vmstate.c
> > index 63f28f26f45691a70936d33e7341d16477a3471f..cfab58c7f45ba50f70af164c3e58b01aaf9cc656 100644
> > --- a/tests/unit/test-vmstate.c
> > +++ b/tests/unit/test-vmstate.c
> > @@ -30,6 +30,7 @@
> >  #include "../migration/savevm.h"
> >  #include "qemu/module.h"
> >  #include "io/channel-file.h"
> > +#include "qapi/error.h"
> >  
> >  static int temp_fd;
> >  
> > @@ -108,14 +109,16 @@ static int load_vmstate_one(const VMStateDescription *desc, void *obj,
> >  {
> >      QEMUFile *f;
> >      int ret;
> > +    Error *local_err = NULL;
> >  
> >      f = open_test_file(true);
> >      qemu_put_buffer(f, wire, size);
> >      qemu_fclose(f);
> >  
> >      f = open_test_file(false);
> > -    ret = vmstate_load_state(f, desc, obj, version);
> > +    ret = vmstate_load_state(f, desc, obj, version, &local_err);
> >      if (ret) {
> > +        warn_report_err(local_err);
> >          g_assert(qemu_file_get_error(f));
> >      } else{
> >          g_assert(!qemu_file_get_error(f));
> > @@ -355,6 +358,8 @@ static const VMStateDescription vmstate_versioned = {
> >  
> >  static void test_load_v1(void)
> >  {
> > +    Error *local_err = NULL;
> > +    int ret;
> >      uint8_t buf[] = {
> >          0, 0, 0, 10,             /* a */
> >          0, 0, 0, 30,             /* c */
> > @@ -365,7 +370,10 @@ static void test_load_v1(void)
> >  
> >      QEMUFile *loading = open_test_file(false);
> >      TestStruct obj = { .b = 200, .e = 500, .f = 600 };
> > -    vmstate_load_state(loading, &vmstate_versioned, &obj, 1);
> > +    ret = vmstate_load_state(loading, &vmstate_versioned, &obj, 1, &local_err);
> > +    if (ret < 0) {
> > +        warn_report_err(local_err);
> > +    }
> >      g_assert(!qemu_file_get_error(loading));
> >      g_assert_cmpint(obj.a, ==, 10);
> >      g_assert_cmpint(obj.b, ==, 200);
> > @@ -378,6 +386,8 @@ static void test_load_v1(void)
> >  
> >  static void test_load_v2(void)
> >  {
> > +    Error *local_err = NULL;
> > +    int ret;
> >      uint8_t buf[] = {
> >          0, 0, 0, 10,             /* a */
> >          0, 0, 0, 20,             /* b */
> > @@ -391,7 +401,10 @@ static void test_load_v2(void)
> >  
> >      QEMUFile *loading = open_test_file(false);
> >      TestStruct obj;
> > -    vmstate_load_state(loading, &vmstate_versioned, &obj, 2);
> > +    ret = vmstate_load_state(loading, &vmstate_versioned, &obj, 2, &local_err);
> > +    if (ret < 0) {
> > +        warn_report_err(local_err);
> > +    }
> >      g_assert_cmpint(obj.a, ==, 10);
> >      g_assert_cmpint(obj.b, ==, 20);
> >      g_assert_cmpint(obj.c, ==, 30);
> > @@ -467,6 +480,8 @@ static void test_save_skip(void)
> >  
> >  static void test_load_noskip(void)
> >  {
> > +    Error *local_err = NULL;
> > +    int ret;
> >      uint8_t buf[] = {
> >          0, 0, 0, 10,             /* a */
> >          0, 0, 0, 20,             /* b */
> > @@ -480,7 +495,10 @@ static void test_load_noskip(void)
> >  
> >      QEMUFile *loading = open_test_file(false);
> >      TestStruct obj = { .skip_c_e = false };
> > -    vmstate_load_state(loading, &vmstate_skipping, &obj, 2);
> > +    ret = vmstate_load_state(loading, &vmstate_skipping, &obj, 2, &local_err);
> > +    if (ret < 0) {
> > +        warn_report_err(local_err);
> > +    }
> >      g_assert(!qemu_file_get_error(loading));
> >      g_assert_cmpint(obj.a, ==, 10);
> >      g_assert_cmpint(obj.b, ==, 20);
> > @@ -493,6 +511,8 @@ static void test_load_noskip(void)
> >  
> >  static void test_load_skip(void)
> >  {
> > +    Error *local_err = NULL;
> > +    int ret;
> >      uint8_t buf[] = {
> >          0, 0, 0, 10,             /* a */
> >          0, 0, 0, 20,             /* b */
> > @@ -504,7 +524,10 @@ static void test_load_skip(void)
> >  
> >      QEMUFile *loading = open_test_file(false);
> >      TestStruct obj = { .skip_c_e = true, .c = 300, .e = 500 };
> > -    vmstate_load_state(loading, &vmstate_skipping, &obj, 2);
> > +    ret = vmstate_load_state(loading, &vmstate_skipping, &obj, 2, &local_err);
> > +    if (ret < 0) {
> > +        warn_report_err(local_err);
> > +    }
> >      g_assert(!qemu_file_get_error(loading));
> >      g_assert_cmpint(obj.a, ==, 10);
> >      g_assert_cmpint(obj.b, ==, 20);
> > @@ -744,6 +767,8 @@ static void test_save_q(void)
> >  
> >  static void test_load_q(void)
> >  {
> > +    int ret;
> > +    Error *local_err = NULL;
> >      TestQtailq obj_q = {
> >          .i16 = -512,
> >          .i32 = 70000,
> > @@ -773,7 +798,10 @@ static void test_load_q(void)
> >      TestQtailq tgt;
> >  
> >      QTAILQ_INIT(&tgt.q);
> > -    vmstate_load_state(fload, &vmstate_q, &tgt, 1);
> > +    ret = vmstate_load_state(fload, &vmstate_q, &tgt, 1, &local_err);
> > +    if (ret < 0) {
> > +        warn_report_err(local_err);
> > +    }
> >      char eof = qemu_get_byte(fload);
> >      g_assert(!qemu_file_get_error(fload));
> >      g_assert_cmpint(tgt.i16, ==, obj_q.i16);
> > @@ -1115,6 +1143,8 @@ static void diff_iommu(TestGTreeIOMMU *iommu1, TestGTreeIOMMU *iommu2)
> >  
> >  static void test_gtree_load_domain(void)
> >  {
> > +    Error *local_err = NULL;
> > +    int ret;
> >      TestGTreeDomain *dest_domain = g_new0(TestGTreeDomain, 1);
> >      TestGTreeDomain *orig_domain = create_first_domain();
> >      QEMUFile *fload, *fsave;
> > @@ -1127,7 +1157,11 @@ static void test_gtree_load_domain(void)
> >  
> >      fload = open_test_file(false);
> >  
> > -    vmstate_load_state(fload, &vmstate_domain, dest_domain, 1);
> > +    ret = vmstate_load_state(fload, &vmstate_domain, dest_domain, 1,
> > +                             &local_err);
> > +    if (ret < 0) {
> > +        warn_report_err(local_err);
> > +    }
> >      eof = qemu_get_byte(fload);
> >      g_assert(!qemu_file_get_error(fload));
> >      g_assert_cmpint(orig_domain->id, ==, dest_domain->id);
> > @@ -1230,6 +1264,8 @@ static void test_gtree_save_iommu(void)
> >  
> >  static void test_gtree_load_iommu(void)
> >  {
> > +    Error *local_err = NULL;
> > +    int ret;
> >      TestGTreeIOMMU *dest_iommu = g_new0(TestGTreeIOMMU, 1);
> >      TestGTreeIOMMU *orig_iommu = create_iommu();
> >      QEMUFile *fsave, *fload;
> > @@ -1241,7 +1277,10 @@ static void test_gtree_load_iommu(void)
> >      qemu_fclose(fsave);
> >  
> >      fload = open_test_file(false);
> > -    vmstate_load_state(fload, &vmstate_iommu, dest_iommu, 1);
> > +    ret = vmstate_load_state(fload, &vmstate_iommu, dest_iommu, 1, &local_err);
> > +    if (ret < 0) {
> > +        warn_report_err(local_err);
> > +    }
> >      eof = qemu_get_byte(fload);
> >      g_assert(!qemu_file_get_error(fload));
> >      g_assert_cmpint(orig_iommu->id, ==, dest_iommu->id);
> > @@ -1363,6 +1402,8 @@ static void test_save_qlist(void)
> >  
> >  static void test_load_qlist(void)
> >  {
> > +    Error *local_err = NULL;
> > +    int ret;
> >      QEMUFile *fsave, *fload;
> >      TestQListContainer *orig_container = alloc_container();
> >      TestQListContainer *dest_container = g_new0(TestQListContainer, 1);
> > @@ -1376,7 +1417,11 @@ static void test_load_qlist(void)
> >      qemu_fclose(fsave);
> >  
> >      fload = open_test_file(false);
> > -    vmstate_load_state(fload, &vmstate_container, dest_container, 1);
> > +    ret = vmstate_load_state(fload, &vmstate_container, dest_container, 1,
> > +                             &local_err);
> > +    if (ret < 0) {
> > +        warn_report_err(local_err);
> > +    }
> >      eof = qemu_get_byte(fload);
> >      g_assert(!qemu_file_get_error(fload));
> >      g_assert_cmpint(eof, ==, QEMU_VM_EOF);
> > diff --git a/ui/vdagent.c b/ui/vdagent.c
> > index c0746fe5b168fdc7aeb4866de2ba0c3387566649..b699658c06cc3765bae2e5effa34f66b7cfd4ead 100644
> > --- a/ui/vdagent.c
> > +++ b/ui/vdagent.c
> > @@ -1008,7 +1008,8 @@ static int get_cbinfo(QEMUFile *f, void *pv, size_t size,
> >  
> >      vdagent_clipboard_peer_register(vd);
> >  
> > -    ret = vmstate_load_state(f, &vmstate_cbinfo_array, &cbinfo, 0);
> > +    ret = vmstate_load_state(f, &vmstate_cbinfo_array, &cbinfo, 0,
> > +                             &error_fatal);
> >      if (ret) {
> >          return ret;
> >      }
> 
> Unreachable?
Yes it is unreachable here. All of these functions change to passing errp later
in the series. One way to avoid using &error_fatal is to pass &local_err and
warn_err_report() after we check the return value.
We can do that here and in places where the return is checked and depending on
it some more processing is done.
> 


Regards,
Arun

Re: [PATCH v11 05/27] migration: push Error **errp into loadvm_process_command()

[Arun Menon]

Hi Fabiano,

Thanks for the review.
On Fri, Aug 15, 2025 at 03:35:40PM -0300, Fabiano Rosas wrote:
> Arun Menon <armenon@redhat.com> writes:
> 
> > 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 loadvm_process_command() must report an error
> > in errp, in case of failure.
> >
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Signed-off-by: Arun Menon <armenon@redhat.com>
> > ---
> >  migration/savevm.c | 82 +++++++++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 60 insertions(+), 22 deletions(-)
> >
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 7f79461844105bf672314c3325caee9cdb654c27..715cc35394cac5fe225ef88cf448714a02321bd7 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2546,32 +2546,35 @@ static int loadvm_postcopy_handle_switchover_start(void)
> >   * LOADVM_QUIT All good, but exit the loop
> >   * <0          Error
> >   */
> > -static int loadvm_process_command(QEMUFile *f)
> > +static int loadvm_process_command(QEMUFile *f, Error **errp)
> >  {
> >      MigrationIncomingState *mis = migration_incoming_get_current();
> >      uint16_t cmd;
> >      uint16_t len;
> >      uint32_t tmp32;
> > +    int ret;
> >  
> >      cmd = qemu_get_be16(f);
> >      len = qemu_get_be16(f);
> >  
> >      /* Check validity before continue processing of cmds */
> > -    if (qemu_file_get_error(f)) {
> > -        return qemu_file_get_error(f);
> > +    ret = qemu_file_get_error(f);
> > +    if (ret) {
> > +        error_setg(errp, "Failed to load VM process command: %d", ret);
> 
> "Failed to process command: stream error: %d"
Thanks, I will add this.
> 
> > +        return ret;
> >      }
> >  
> >      if (cmd >= MIG_CMD_MAX || cmd == MIG_CMD_INVALID) {
> > -        error_report("MIG_CMD 0x%x unknown (len 0x%x)", cmd, len);
> > +        error_setg(errp, "MIG_CMD 0x%x unknown (len 0x%x)", cmd, len);
> >          return -EINVAL;
> >      }
> >  
> >      trace_loadvm_process_command(mig_cmd_args[cmd].name, len);
> >  
> >      if (mig_cmd_args[cmd].len != -1 && mig_cmd_args[cmd].len != len) {
> > -        error_report("%s received with bad length - expecting %zu, got %d",
> > -                     mig_cmd_args[cmd].name,
> > -                     (size_t)mig_cmd_args[cmd].len, len);
> > +        error_setg(errp, "%s received with bad length - expecting %zu, got %d",
> > +                   mig_cmd_args[cmd].name,
> > +                   (size_t)mig_cmd_args[cmd].len, len);
> >          return -ERANGE;
> >      }
> >  
> 
> Where's MIG_CMD_OPEN_RETURN_PATH?
I am sorry, I did not understand this. Do you mean the first switch case?
> 
> > @@ -2594,11 +2597,10 @@ static int loadvm_process_command(QEMUFile *f)
> >           * been created.
> >           */
> >          if (migrate_switchover_ack() && !mis->switchover_ack_pending_num) {
> > -            int ret = migrate_send_rp_switchover_ack(mis);
> > +            ret = migrate_send_rp_switchover_ack(mis);
> >              if (ret) {
> > -                error_report(
> > -                    "Could not send switchover ack RP MSG, err %d (%s)", ret,
> > -                    strerror(-ret));
> > +                error_setg_errno(errp, -ret,
> > +                                 "Could not send switchover ack RP MSG");
> >                  return ret;
> >              }
> >          }
> > @@ -2608,39 +2610,71 @@ static int loadvm_process_command(QEMUFile *f)
> >          tmp32 = qemu_get_be32(f);
> >          trace_loadvm_process_command_ping(tmp32);
> >          if (!mis->to_src_file) {
> > -            error_report("CMD_PING (0x%x) received with no return path",
> > -                         tmp32);
> > +            error_setg(errp, "CMD_PING (0x%x) received with no return path",
> > +                       tmp32);
> >              return -1;
> >          }
> >          migrate_send_rp_pong(mis, tmp32);
> >          break;
> >  
> >      case MIG_CMD_PACKAGED:
> > -        return loadvm_handle_cmd_packaged(mis);
> > +        ret = loadvm_handle_cmd_packaged(mis);
> 
> I missed a lot of the discussion in this series, but I assume there's a
> good reason to not put the conversion of each command first in the
> series, so there's no need for temporary code in this patch.
There was a cyclic dependency between few functions.
I have the link to the discussion on cyclic dependency here,
https://lore.kernel.org/qemu-devel/822162a2-c80e-47ae-ba4e-26beb7241216@rsg.ci.i.u-tokyo.ac.jp/
IMO, in both cases, the code will have some extra lines to deal with the handling.
> 
> > +        if (ret < 0) {
> > +            error_setg(errp, "Failed to load device state command: %d", ret);
> > +        }
> > +        return ret;
> >  
> >      case MIG_CMD_POSTCOPY_ADVISE:
> > -        return loadvm_postcopy_handle_advise(mis, len);
> > +        ret = loadvm_postcopy_handle_advise(mis, len);
> > +        if (ret < 0) {
> > +            error_setg(errp, "Failed to load device state command: %d", ret);
> > +        }
> > +        return ret;
> >  
> >      case MIG_CMD_POSTCOPY_LISTEN:
> > -        return loadvm_postcopy_handle_listen(mis);
> > +        ret = loadvm_postcopy_handle_listen(mis);
> > +        if (ret < 0) {
> > +            error_setg(errp, "Failed to load device state command: %d", ret);
> > +        }
> > +        return ret;
> >  
> >      case MIG_CMD_POSTCOPY_RUN:
> > -        return loadvm_postcopy_handle_run(mis);
> > +        ret = loadvm_postcopy_handle_run(mis);
> > +        if (ret < 0) {
> > +            error_setg(errp, "Failed to load device state command: %d", ret);
> > +        }
> > +        return ret;
> >  
> >      case MIG_CMD_POSTCOPY_RAM_DISCARD:
> > -        return loadvm_postcopy_ram_handle_discard(mis, len);
> > +        ret = loadvm_postcopy_ram_handle_discard(mis, len);
> > +        if (ret < 0) {
> > +            error_setg(errp, "Failed to load device state command: %d", ret);
> > +        }
> > +        return ret;
> >  
> >      case MIG_CMD_POSTCOPY_RESUME:
> >          return loadvm_postcopy_handle_resume(mis);
> >  
> >      case MIG_CMD_RECV_BITMAP:
> > -        return loadvm_handle_recv_bitmap(mis, len);
> > +        ret = loadvm_handle_recv_bitmap(mis, len);
> > +        if (ret < 0) {
> > +            error_setg(errp, "Failed to load device state command: %d", ret);
> > +        }
> > +        return ret;
> >  
> >      case MIG_CMD_ENABLE_COLO:
> > -        return loadvm_process_enable_colo(mis);
> > +        ret = loadvm_process_enable_colo(mis);
> > +        if (ret < 0) {
> > +            error_setg(errp, "Failed to load device state command: %d", ret);
> > +        }
> > +        return ret;
> >  
> >      case MIG_CMD_SWITCHOVER_START:
> > -        return loadvm_postcopy_handle_switchover_start();
> > +        ret = loadvm_postcopy_handle_switchover_start();
> > +        if (ret < 0) {
> > +            error_setg(errp, "Failed to load device state command: %d", ret);
> > +        }
> > +        return ret;
> >      }
> >  
> >      return 0;
> > @@ -3051,6 +3085,7 @@ int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> >  {
> >      uint8_t section_type;
> >      int ret = 0;
> > +    Error *local_err = NULL;
> >  
> >  retry:
> >      while (true) {
> > @@ -3078,7 +3113,10 @@ retry:
> >              }
> >              break;
> >          case QEMU_VM_COMMAND:
> > -            ret = loadvm_process_command(f);
> > +            ret = loadvm_process_command(f, &local_err);
> > +            if (ret < 0) {
> > +                warn_report_err(local_err);
> 
> Again, some throwaway code here. Commit message could have made this
> clear: "For now, report the error with warn_report until all callers
> have been converted to pass errp".
Yes, I shall add the resoning for warn_report_err() in the commit message
in all the patches wherever it is introduced.
> 
> > +            }
> >              trace_qemu_loadvm_state_section_command(ret);
> >              if ((ret < 0) || (ret == LOADVM_QUIT)) {
> >                  goto out;
> 

Regards,
Arun

Re: [PATCH v11 05/27] migration: push Error **errp into loadvm_process_command()

[Arun Menon]

On Fri, Aug 15, 2025 at 03:35:40PM -0300, Fabiano Rosas wrote:
> Arun Menon <armenon@redhat.com> writes:
> 
> > 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 loadvm_process_command() must report an error
> > in errp, in case of failure.
> >
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Signed-off-by: Arun Menon <armenon@redhat.com>
> > ---
> >  migration/savevm.c | 82 +++++++++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 60 insertions(+), 22 deletions(-)
> >
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 7f79461844105bf672314c3325caee9cdb654c27..715cc35394cac5fe225ef88cf448714a02321bd7 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2546,32 +2546,35 @@ static int loadvm_postcopy_handle_switchover_start(void)
> >   * LOADVM_QUIT All good, but exit the loop
> >   * <0          Error
> >   */
> > -static int loadvm_process_command(QEMUFile *f)
> > +static int loadvm_process_command(QEMUFile *f, Error **errp)
> >  {
> >      MigrationIncomingState *mis = migration_incoming_get_current();
> >      uint16_t cmd;
> >      uint16_t len;
> >      uint32_t tmp32;
> > +    int ret;
> >  
> >      cmd = qemu_get_be16(f);
> >      len = qemu_get_be16(f);
> >  
> >      /* Check validity before continue processing of cmds */
> > -    if (qemu_file_get_error(f)) {
> > -        return qemu_file_get_error(f);
> > +    ret = qemu_file_get_error(f);
> > +    if (ret) {
> > +        error_setg(errp, "Failed to load VM process command: %d", ret);
> 
> "Failed to process command: stream error: %d"
> 
> > +        return ret;
> >      }
> >  
> >      if (cmd >= MIG_CMD_MAX || cmd == MIG_CMD_INVALID) {
> > -        error_report("MIG_CMD 0x%x unknown (len 0x%x)", cmd, len);
> > +        error_setg(errp, "MIG_CMD 0x%x unknown (len 0x%x)", cmd, len);
> >          return -EINVAL;
> >      }
> >  
> >      trace_loadvm_process_command(mig_cmd_args[cmd].name, len);
> >  
> >      if (mig_cmd_args[cmd].len != -1 && mig_cmd_args[cmd].len != len) {
> > -        error_report("%s received with bad length - expecting %zu, got %d",
> > -                     mig_cmd_args[cmd].name,
> > -                     (size_t)mig_cmd_args[cmd].len, len);
> > +        error_setg(errp, "%s received with bad length - expecting %zu, got %d",
> > +                   mig_cmd_args[cmd].name,
> > +                   (size_t)mig_cmd_args[cmd].len, len);
> >          return -ERANGE;
> >      }
> >  
> 
> Where's MIG_CMD_OPEN_RETURN_PATH?
I see, you mean the qemu_file_get_return_path() failure to be reported.
> 
> > @@ -2594,11 +2597,10 @@ static int loadvm_process_command(QEMUFile *f)
> >           * been created.
> >           */
> >          if (migrate_switchover_ack() && !mis->switchover_ack_pending_num) {
> > -            int ret = migrate_send_rp_switchover_ack(mis);
> > +            ret = migrate_send_rp_switchover_ack(mis);
> >              if (ret) {
> > -                error_report(
> > -                    "Could not send switchover ack RP MSG, err %d (%s)", ret,
> > -                    strerror(-ret));
> > +                error_setg_errno(errp, -ret,
> > +                                 "Could not send switchover ack RP MSG");
> >                  return ret;
> >              }
> >          }
> > @@ -2608,39 +2610,71 @@ static int loadvm_process_command(QEMUFile *f)
> >          tmp32 = qemu_get_be32(f);
> >          trace_loadvm_process_command_ping(tmp32);
> >          if (!mis->to_src_file) {
> > -            error_report("CMD_PING (0x%x) received with no return path",
> > -                         tmp32);
> > +            error_setg(errp, "CMD_PING (0x%x) received with no return path",
> > +                       tmp32);
> >              return -1;
> >          }
> >          migrate_send_rp_pong(mis, tmp32);
> >          break;
> >  
> >      case MIG_CMD_PACKAGED:
> > -        return loadvm_handle_cmd_packaged(mis);
> > +        ret = loadvm_handle_cmd_packaged(mis);
> 
> I missed a lot of the discussion in this series, but I assume there's a
> good reason to not put the conversion of each command first in the
> series, so there's no need for temporary code in this patch.
> 
> > +        if (ret < 0) {
> > +            error_setg(errp, "Failed to load device state command: %d", ret);
> > +        }
> > +        return ret;
> >  
> >      case MIG_CMD_POSTCOPY_ADVISE:
> > -        return loadvm_postcopy_handle_advise(mis, len);
> > +        ret = loadvm_postcopy_handle_advise(mis, len);
> > +        if (ret < 0) {
> > +            error_setg(errp, "Failed to load device state command: %d", ret);
> > +        }
> > +        return ret;
> >  
> >      case MIG_CMD_POSTCOPY_LISTEN:
> > -        return loadvm_postcopy_handle_listen(mis);
> > +        ret = loadvm_postcopy_handle_listen(mis);
> > +        if (ret < 0) {
> > +            error_setg(errp, "Failed to load device state command: %d", ret);
> > +        }
> > +        return ret;
> >  
> >      case MIG_CMD_POSTCOPY_RUN:
> > -        return loadvm_postcopy_handle_run(mis);
> > +        ret = loadvm_postcopy_handle_run(mis);
> > +        if (ret < 0) {
> > +            error_setg(errp, "Failed to load device state command: %d", ret);
> > +        }
> > +        return ret;
> >  
> >      case MIG_CMD_POSTCOPY_RAM_DISCARD:
> > -        return loadvm_postcopy_ram_handle_discard(mis, len);
> > +        ret = loadvm_postcopy_ram_handle_discard(mis, len);
> > +        if (ret < 0) {
> > +            error_setg(errp, "Failed to load device state command: %d", ret);
> > +        }
> > +        return ret;
> >  
> >      case MIG_CMD_POSTCOPY_RESUME:
> >          return loadvm_postcopy_handle_resume(mis);
> >  
> >      case MIG_CMD_RECV_BITMAP:
> > -        return loadvm_handle_recv_bitmap(mis, len);
> > +        ret = loadvm_handle_recv_bitmap(mis, len);
> > +        if (ret < 0) {
> > +            error_setg(errp, "Failed to load device state command: %d", ret);
> > +        }
> > +        return ret;
> >  
> >      case MIG_CMD_ENABLE_COLO:
> > -        return loadvm_process_enable_colo(mis);
> > +        ret = loadvm_process_enable_colo(mis);
> > +        if (ret < 0) {
> > +            error_setg(errp, "Failed to load device state command: %d", ret);
> > +        }
> > +        return ret;
> >  
> >      case MIG_CMD_SWITCHOVER_START:
> > -        return loadvm_postcopy_handle_switchover_start();
> > +        ret = loadvm_postcopy_handle_switchover_start();
> > +        if (ret < 0) {
> > +            error_setg(errp, "Failed to load device state command: %d", ret);
> > +        }
> > +        return ret;
> >      }
> >  
> >      return 0;
> > @@ -3051,6 +3085,7 @@ int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> >  {
> >      uint8_t section_type;
> >      int ret = 0;
> > +    Error *local_err = NULL;
> >  
> >  retry:
> >      while (true) {
> > @@ -3078,7 +3113,10 @@ retry:
> >              }
> >              break;
> >          case QEMU_VM_COMMAND:
> > -            ret = loadvm_process_command(f);
> > +            ret = loadvm_process_command(f, &local_err);
> > +            if (ret < 0) {
> > +                warn_report_err(local_err);
> 
> Again, some throwaway code here. Commit message could have made this
> clear: "For now, report the error with warn_report until all callers
> have been converted to pass errp".
> 
> > +            }
> >              trace_qemu_loadvm_state_section_command(ret);
> >              if ((ret < 0) || (ret == LOADVM_QUIT)) {
> >                  goto out;
> 

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

[Fabiano Rosas]

Arun Menon <armenon@redhat.com> writes:

> Hi Fabiano,
>
> Thanks for the review.
>
> On Fri, Aug 15, 2025 at 12:41:50PM -0300, Fabiano Rosas wrote:
>> Arun Menon <armenon@redhat.com> writes:
>> 
>> > 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.
>> >
>> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > Signed-off-by: Arun Menon <armenon@redhat.com>
>> > ---
>> >  hw/display/virtio-gpu.c     |  2 +-
>> >  hw/pci/pci.c                |  3 ++-
>> >  hw/s390x/virtio-ccw.c       |  2 +-
>> >  hw/scsi/spapr_vscsi.c       |  2 +-
>> >  hw/vfio/pci.c               |  3 ++-
>> >  hw/virtio/virtio-mmio.c     |  3 ++-
>> >  hw/virtio/virtio-pci.c      |  2 +-
>> >  hw/virtio/virtio.c          |  4 +--
>> >  include/migration/vmstate.h |  2 +-
>> >  migration/cpr.c             |  5 ++--
>> >  migration/savevm.c          |  6 +++--
>> >  migration/vmstate-types.c   | 22 ++++++++++++----
>> >  migration/vmstate.c         | 61 ++++++++++++++++++++++++++++++-------------
>> >  tests/unit/test-vmstate.c   | 63 ++++++++++++++++++++++++++++++++++++++-------
>> >  ui/vdagent.c                |  3 ++-
>> >  15 files changed, 136 insertions(+), 47 deletions(-)
>> >
>> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>> > index 0a1a625b0ea6cf26cb0d799171a57ed3d3ab2442..5dc31bc6bfb0272e29a4364ab10de2595a4bedf7 100644
>> > --- a/hw/display/virtio-gpu.c
>> > +++ b/hw/display/virtio-gpu.c
>> > @@ -1343,7 +1343,7 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
>> >      }
>> >  
>> >      /* load & apply scanout state */
>> > -    vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1);
>> > +    vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, &error_fatal);
>> >  
>> >      return 0;
>> >  }
>> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> > index c70b5ceebaf1f2b10768bd030526cbb518da2b8d..6be932d3bb67ff0c4808707db2a7b6378a90e82b 100644
>> > --- a/hw/pci/pci.c
>> > +++ b/hw/pci/pci.c
>> > @@ -934,7 +934,8 @@ void pci_device_save(PCIDevice *s, QEMUFile *f)
>> >  int pci_device_load(PCIDevice *s, QEMUFile *f)
>> >  {
>> >      int ret;
>> > -    ret = vmstate_load_state(f, &vmstate_pci_device, s, s->version_id);
>> > +    ret = vmstate_load_state(f, &vmstate_pci_device, s, s->version_id,
>> > +                             &error_fatal);
>> >      /* Restore the interrupt status bit. */
>> >      pci_update_irq_status(s);
>> >      return ret;
>> > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>> > index d2f85b39f30f7fc82e0c600144c0a958e1269b2c..6a9641a03d5d3a38a4de7ceb9deffc0cc303bcff 100644
>> > --- a/hw/s390x/virtio-ccw.c
>> > +++ b/hw/s390x/virtio-ccw.c
>> > @@ -1136,7 +1136,7 @@ static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f)
>> >  static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
>> >  {
>> >      VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
>> > -    return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1);
>> > +    return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1, &error_fatal);
>> >  }
>> >  
>> >  static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
>> > diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
>> > index 20f70fb2729de78b9636a6b8c869695dab4f8902..8c896022d324f51962605288d6d6df1648c83cde 100644
>> > --- a/hw/scsi/spapr_vscsi.c
>> > +++ b/hw/scsi/spapr_vscsi.c
>> > @@ -648,7 +648,7 @@ static void *vscsi_load_request(QEMUFile *f, SCSIRequest *sreq)
>> >      assert(!req->active);
>> >  
>> >      memset(req, 0, sizeof(*req));
>> > -    rc = vmstate_load_state(f, &vmstate_spapr_vscsi_req, req, 1);
>> > +    rc = vmstate_load_state(f, &vmstate_spapr_vscsi_req, req, 1, &error_fatal);
>> >      if (rc) {
>> >          fprintf(stderr, "VSCSI: failed loading request tag#%u\n", sreq->tag);
>> >          return NULL;
>> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> > index 4fa692c1a32bcfa4e4939e5fcb64f2bf19905b3b..0be54762cdcbdb4780b8228b0bdf7fc6bd74dd57 100644
>> > --- a/hw/vfio/pci.c
>> > +++ b/hw/vfio/pci.c
>> > @@ -2795,7 +2795,8 @@ static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
>> >          old_addr[bar] = pdev->io_regions[bar].addr;
>> >      }
>> >  
>> > -    ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1);
>> > +    ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1,
>> > +                             &error_fatal);
>> >      if (ret) {
>> >          return ret;
>> >      }
>> > diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
>> > index 532c67107ba1d2978a76cf49f9cdc1de1dea3e11..0a688909fc606a3c9fde933667ae8c309ab527d0 100644
>> > --- a/hw/virtio/virtio-mmio.c
>> > +++ b/hw/virtio/virtio-mmio.c
>> > @@ -34,6 +34,7 @@
>> >  #include "qemu/error-report.h"
>> >  #include "qemu/log.h"
>> >  #include "trace.h"
>> > +#include "qapi/error.h"
>> >  
>> >  static bool virtio_mmio_ioeventfd_enabled(DeviceState *d)
>> >  {
>> > @@ -619,7 +620,7 @@ static int virtio_mmio_load_extra_state(DeviceState *opaque, QEMUFile *f)
>> >  {
>> >      VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
>> >  
>> > -    return vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1);
>> > +    return vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1, &error_fatal);
>> >  }
>> >  
>> >  static bool virtio_mmio_has_extra_state(DeviceState *opaque)
>> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> > index 767216d795998708f5716a23ae16c79cd90ff489..b04faa1e5c91b5cef40e54ec41d92422d16bfc13 100644
>> > --- a/hw/virtio/virtio-pci.c
>> > +++ b/hw/virtio/virtio-pci.c
>> > @@ -161,7 +161,7 @@ static int virtio_pci_load_extra_state(DeviceState *d, QEMUFile *f)
>> >  {
>> >      VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
>> >  
>> > -    return vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1);
>> > +    return vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1, &error_fatal);
>> >  }
>> >  
>> >  static void virtio_pci_save_queue(DeviceState *d, int n, QEMUFile *f)
>> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> > index 9a81ad912e013fc254899c4e55cff1f76a6112a4..6bcafb338d1b5becadcacf092ba33a6ae4c3d194 100644
>> > --- a/hw/virtio/virtio.c
>> > +++ b/hw/virtio/virtio.c
>> > @@ -3327,14 +3327,14 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
>> >      }
>> >  
>> >      if (vdc->vmsd) {
>> > -        ret = vmstate_load_state(f, vdc->vmsd, vdev, version_id);
>> > +        ret = vmstate_load_state(f, vdc->vmsd, vdev, version_id, &error_fatal);
>> >          if (ret) {
>> >              return ret;
>> >          }
>> >      }
>> >  
>> >      /* Subsections */
>> > -    ret = vmstate_load_state(f, &vmstate_virtio, vdev, 1);
>> > +    ret = vmstate_load_state(f, &vmstate_virtio, vdev, 1, &error_fatal);
>> >      if (ret) {
>> >          return ret;
>> >      }
>> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
>> > index 1ff7bd9ac425ba67cd5ca7ad97bcf570f9e19abe..056781b1c21e737583f081594d9f88b32adfd674 100644
>> > --- a/include/migration/vmstate.h
>> > +++ b/include/migration/vmstate.h
>> > @@ -1196,7 +1196,7 @@ extern const VMStateInfo vmstate_info_qlist;
>> >      }
>> >  
>> >  int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>> > -                       void *opaque, int version_id);
>> > +                       void *opaque, int version_id, Error **errp);
>> >  int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
>> >                         void *opaque, JSONWriter *vmdesc);
>> >  int vmstate_save_state_with_err(QEMUFile *f, const VMStateDescription *vmsd,
>> > diff --git a/migration/cpr.c b/migration/cpr.c
>> > index 42ad0b0d500e5de57faf0c6517e216b2d1c0cacf..bdb24736f44e91ba59b6e622a315597c97e7f64d 100644
>> > --- a/migration/cpr.c
>> > +++ b/migration/cpr.c
>> > @@ -202,6 +202,7 @@ int cpr_state_save(MigrationChannel *channel, Error **errp)
>> >  
>> >  int cpr_state_load(MigrationChannel *channel, Error **errp)
>> >  {
>> > +    ERRP_GUARD();
>> >      int ret;
>> >      uint32_t v;
>> >      QEMUFile *f;
>> > @@ -233,9 +234,9 @@ int cpr_state_load(MigrationChannel *channel, Error **errp)
>> >          return -ENOTSUP;
>> >      }
>> >  
>> > -    ret = vmstate_load_state(f, &vmstate_cpr_state, &cpr_state, 1);
>> > +    ret = vmstate_load_state(f, &vmstate_cpr_state, &cpr_state, 1, errp);
>> >      if (ret) {
>> > -        error_setg(errp, "vmstate_load_state error %d", ret);
>> > +        error_prepend(errp, "vmstate_load_state error %d: ", ret);
>> >          qemu_fclose(f);
>> >          return ret;
>> >      }
>> > diff --git a/migration/savevm.c b/migration/savevm.c
>> > index fabbeb296ae987d0c06ba6dafda63720205fecfd..cb64f2855d46aaa7c617b3e4079a2c9e566079b2 100644
>> > --- a/migration/savevm.c
>> > +++ b/migration/savevm.c
>> > @@ -969,7 +969,8 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se)
>> >      if (!se->vmsd) {         /* Old style */
>> >          return se->ops->load_state(f, se->opaque, se->load_version_id);
>> >      }
>> > -    return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id);
>> > +    return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id,
>> > +                              &error_fatal);
>> >  }
>> >  
>> >  static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
>> > @@ -2839,7 +2840,8 @@ static int qemu_loadvm_state_header(QEMUFile *f)
>> >              error_report("Configuration section missing");
>> >              return -EINVAL;
>> >          }
>> > -        ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0);
>> > +        ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0,
>> > +                                 &error_fatal);
>> >  
>> >          if (ret) {
>> >              return ret;
>> > diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
>> > index 741a588b7e18c6d37724b08a0101edc8bc74a0a5..f41670cc853c5b41ccc8def354886a8e5c1451fd 100644
>> > --- a/migration/vmstate-types.c
>> > +++ b/migration/vmstate-types.c
>> > @@ -19,6 +19,7 @@
>> >  #include "qemu/error-report.h"
>> >  #include "qemu/queue.h"
>> >  #include "trace.h"
>> > +#include "qapi/error.h"
>> >  
>> >  /* bool */
>> >  
>> > @@ -543,13 +544,17 @@ static int get_tmp(QEMUFile *f, void *pv, size_t size,
>> >                     const VMStateField *field)
>> >  {
>> >      int ret;
>> > +    Error *local_err = NULL;
>> >      const VMStateDescription *vmsd = field->vmsd;
>> >      int version_id = field->version_id;
>> >      void *tmp = g_malloc(size);
>> >  
>> >      /* Writes the parent field which is at the start of the tmp */
>> >      *(void **)tmp = pv;
>> > -    ret = vmstate_load_state(f, vmsd, tmp, version_id);
>> > +    ret = vmstate_load_state(f, vmsd, tmp, version_id, &local_err);
>> > +    if (ret < 0) {
>> > +        warn_report_err(local_err);
>> > +    }
>> >      g_free(tmp);
>> >      return ret;
>> >  }
>> > @@ -626,6 +631,7 @@ static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size,
>> >                        const VMStateField *field)
>> >  {
>> >      int ret = 0;
>> > +    Error *local_err = NULL;
>> >      const VMStateDescription *vmsd = field->vmsd;
>> >      /* size of a QTAILQ element */
>> >      size_t size = field->size;
>> > @@ -649,8 +655,9 @@ static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size,
>> >  
>> >      while (qemu_get_byte(f)) {
>> >          elm = g_malloc(size);
>> > -        ret = vmstate_load_state(f, vmsd, elm, version_id);
>> > +        ret = vmstate_load_state(f, vmsd, elm, version_id, &local_err);
>> >          if (ret) {
>> > +            warn_report_err(local_err);
>> >              return ret;
>> >          }
>> >          QTAILQ_RAW_INSERT_TAIL(pv, elm, entry_offset);
>> > @@ -772,6 +779,7 @@ static int get_gtree(QEMUFile *f, void *pv, size_t unused_size,
>> >      GTree *tree = *pval;
>> >      void *key, *val;
>> >      int ret = 0;
>> > +    Error *local_err = NULL;
>> >  
>> >      /* in case of direct key, the key vmsd can be {}, ie. check fields */
>> >      if (!direct_key && version_id > key_vmsd->version_id) {
>> > @@ -803,18 +811,20 @@ static int get_gtree(QEMUFile *f, void *pv, size_t unused_size,
>> >              key = (void *)(uintptr_t)qemu_get_be64(f);
>> >          } else {
>> >              key = g_malloc0(key_size);
>> > -            ret = vmstate_load_state(f, key_vmsd, key, version_id);
>> > +            ret = vmstate_load_state(f, key_vmsd, key, version_id, &local_err);
>> >              if (ret) {
>> >                  error_report("%s : failed to load %s (%d)",
>> >                               field->name, key_vmsd->name, ret);
>> > +                warn_report_err(local_err);
>> >                  goto key_error;
>> >              }
>> >          }
>> >          val = g_malloc0(val_size);
>> > -        ret = vmstate_load_state(f, val_vmsd, val, version_id);
>> > +        ret = vmstate_load_state(f, val_vmsd, val, version_id, &local_err);
>> >          if (ret) {
>> >              error_report("%s : failed to load %s (%d)",
>> >                           field->name, val_vmsd->name, ret);
>> > +            warn_report_err(local_err);
>> >              goto val_error;
>> >          }
>> >          g_tree_insert(tree, key, val);
>> > @@ -872,6 +882,7 @@ static int get_qlist(QEMUFile *f, void *pv, size_t unused_size,
>> >                       const VMStateField *field)
>> >  {
>> >      int ret = 0;
>> > +    Error *local_err = NULL;
>> >      const VMStateDescription *vmsd = field->vmsd;
>> >      /* size of a QLIST element */
>> >      size_t size = field->size;
>> > @@ -892,10 +903,11 @@ static int get_qlist(QEMUFile *f, void *pv, size_t unused_size,
>> >  
>> >      while (qemu_get_byte(f)) {
>> >          elm = g_malloc(size);
>> > -        ret = vmstate_load_state(f, vmsd, elm, version_id);
>> > +        ret = vmstate_load_state(f, vmsd, elm, version_id, &local_err);
>> >          if (ret) {
>> >              error_report("%s: failed to load %s (%d)", field->name,
>> >                           vmsd->name, ret);
>> > +            warn_report_err(local_err);
>> >              g_free(elm);
>> >              return ret;
>> >          }
>> > diff --git a/migration/vmstate.c b/migration/vmstate.c
>> > index 6108c7fe283a5013ce42ea9987723c489aef26a2..1cd609a1d598995af1e51d1f4d58d68133f1426d 100644
>> > --- a/migration/vmstate.c
>> > +++ b/migration/vmstate.c
>> > @@ -132,29 +132,34 @@ 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)
>> >  {
>> > +    ERRP_GUARD();
>> >      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 for: '%s', "
>> 
>> "VM" pre load is a little ambiguous. Simply "pre load" or "pre load
>> hook" is better.
> Sure, will do this.
>> 
>> > +                       "version_id: %d, minimum version_id: %d, ret: %d",
>> > +                       vmsd->name, vmsd->version_id, vmsd->minimum_version_id,
>> > +                       ret);
>> >              return ret;
>> >          }
>> >      }
>> > @@ -192,13 +197,20 @@ 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);
>> > +                    if (ret < 0) {
>> > +                        error_setg(errp,
>> > +                                   "Failed to get info for %s: %d",
>> > +                                   inner_field->name, ret);
>> 
>> "get info" is not correct. This is the type-specific getter
>> invocation. Because the migration (for the most part) is a stream, each
>> type provides it's own getter which knows about the size of the field
>> and any particularities such as magic values. So:
>> 
>> error_setg(errp, "Failed to load element of type %s for %s: %d",
>>            info->name, inner_field->name, ret);
> yes, will change the message. Thanks.
>> 
>> > +                    }
>> >                  }
>> >  
>> >                  /* If we used a fake temp field.. free it now */
>> > @@ -208,30 +220,42 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
>> >  
>> >                  if (ret >= 0) {
>> >                      ret = qemu_file_get_error(f);
>> > +                    if (ret < 0) {
>> > +                        error_setg(errp, "Failed to load %s state: %d",
>> > +                                   vmsd->name, ret);
>> 
>> We could go a little more specific here, it's useful to know whether the
>> error was on the transport side, rather than something logical with the
>> migrated data. I don't really care about the actual string, but one
>> suggestion is "stream error":
>> 
>> error_setg(errp, "Failed to load %s state: stream error: %d",
>>            vmsd->name, ret);
> yes, will add stream error. Thanks.
>> 
>> > +                    }
>> >                  }
>> >                  if (ret < 0) {
>> >                      qemu_file_set_error(f, ret);
>> > -                    error_report("Failed to load %s:%s", vmsd->name,
>> > -                                 field->name);
>> > +                    error_prepend(errp,
>> > +                                  "Failed to load %s:%s version_id: %d: ",
>> 
>> Usage of : is inconsistent with below /
> Since we used error_prepend here, we need an extra space after the colon ':'
> That way the next error string looks readable.

Right, that's not what I'm talking about. What's inconsistent is: %s:%s
vs. %s/%s

>> 
>> > +                                   vmsd->name, field->name, vmsd->version_id);
>> >                      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 version_id: %d",

here^

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

[Arun Menon]

On Wed, Aug 20, 2025 at 12:24:49PM -0300, Fabiano Rosas wrote:
> Arun Menon <armenon@redhat.com> writes:
> 
> > Hi Fabiano,
> >
> > Thanks for the review.
> >
> > On Fri, Aug 15, 2025 at 12:41:50PM -0300, Fabiano Rosas wrote:
> >> Arun Menon <armenon@redhat.com> writes:
> >> 
> >> > 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.
> >> >
> >> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> > Signed-off-by: Arun Menon <armenon@redhat.com>
> >> > ---
> >> >  hw/display/virtio-gpu.c     |  2 +-
> >> >  hw/pci/pci.c                |  3 ++-
> >> >  hw/s390x/virtio-ccw.c       |  2 +-
> >> >  hw/scsi/spapr_vscsi.c       |  2 +-
> >> >  hw/vfio/pci.c               |  3 ++-
> >> >  hw/virtio/virtio-mmio.c     |  3 ++-
> >> >  hw/virtio/virtio-pci.c      |  2 +-
> >> >  hw/virtio/virtio.c          |  4 +--
> >> >  include/migration/vmstate.h |  2 +-
> >> >  migration/cpr.c             |  5 ++--
> >> >  migration/savevm.c          |  6 +++--
> >> >  migration/vmstate-types.c   | 22 ++++++++++++----
> >> >  migration/vmstate.c         | 61 ++++++++++++++++++++++++++++++-------------
> >> >  tests/unit/test-vmstate.c   | 63 ++++++++++++++++++++++++++++++++++++++-------
> >> >  ui/vdagent.c                |  3 ++-
> >> >  15 files changed, 136 insertions(+), 47 deletions(-)
> >> >
> >> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> >> > index 0a1a625b0ea6cf26cb0d799171a57ed3d3ab2442..5dc31bc6bfb0272e29a4364ab10de2595a4bedf7 100644
> >> > --- a/hw/display/virtio-gpu.c
> >> > +++ b/hw/display/virtio-gpu.c
> >> > @@ -1343,7 +1343,7 @@ static int virtio_gpu_load(QEMUFile *f, void *opaque, size_t size,
> >> >      }
> >> >  
> >> >      /* load & apply scanout state */
> >> > -    vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1);
> >> > +    vmstate_load_state(f, &vmstate_virtio_gpu_scanouts, g, 1, &error_fatal);
> >> >  
> >> >      return 0;
> >> >  }
> >> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >> > index c70b5ceebaf1f2b10768bd030526cbb518da2b8d..6be932d3bb67ff0c4808707db2a7b6378a90e82b 100644
> >> > --- a/hw/pci/pci.c
> >> > +++ b/hw/pci/pci.c
> >> > @@ -934,7 +934,8 @@ void pci_device_save(PCIDevice *s, QEMUFile *f)
> >> >  int pci_device_load(PCIDevice *s, QEMUFile *f)
> >> >  {
> >> >      int ret;
> >> > -    ret = vmstate_load_state(f, &vmstate_pci_device, s, s->version_id);
> >> > +    ret = vmstate_load_state(f, &vmstate_pci_device, s, s->version_id,
> >> > +                             &error_fatal);
> >> >      /* Restore the interrupt status bit. */
> >> >      pci_update_irq_status(s);
> >> >      return ret;
> >> > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> >> > index d2f85b39f30f7fc82e0c600144c0a958e1269b2c..6a9641a03d5d3a38a4de7ceb9deffc0cc303bcff 100644
> >> > --- a/hw/s390x/virtio-ccw.c
> >> > +++ b/hw/s390x/virtio-ccw.c
> >> > @@ -1136,7 +1136,7 @@ static void virtio_ccw_save_config(DeviceState *d, QEMUFile *f)
> >> >  static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
> >> >  {
> >> >      VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
> >> > -    return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1);
> >> > +    return vmstate_load_state(f, &vmstate_virtio_ccw_dev, dev, 1, &error_fatal);
> >> >  }
> >> >  
> >> >  static void virtio_ccw_pre_plugged(DeviceState *d, Error **errp)
> >> > diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
> >> > index 20f70fb2729de78b9636a6b8c869695dab4f8902..8c896022d324f51962605288d6d6df1648c83cde 100644
> >> > --- a/hw/scsi/spapr_vscsi.c
> >> > +++ b/hw/scsi/spapr_vscsi.c
> >> > @@ -648,7 +648,7 @@ static void *vscsi_load_request(QEMUFile *f, SCSIRequest *sreq)
> >> >      assert(!req->active);
> >> >  
> >> >      memset(req, 0, sizeof(*req));
> >> > -    rc = vmstate_load_state(f, &vmstate_spapr_vscsi_req, req, 1);
> >> > +    rc = vmstate_load_state(f, &vmstate_spapr_vscsi_req, req, 1, &error_fatal);
> >> >      if (rc) {
> >> >          fprintf(stderr, "VSCSI: failed loading request tag#%u\n", sreq->tag);
> >> >          return NULL;
> >> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> >> > index 4fa692c1a32bcfa4e4939e5fcb64f2bf19905b3b..0be54762cdcbdb4780b8228b0bdf7fc6bd74dd57 100644
> >> > --- a/hw/vfio/pci.c
> >> > +++ b/hw/vfio/pci.c
> >> > @@ -2795,7 +2795,8 @@ static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
> >> >          old_addr[bar] = pdev->io_regions[bar].addr;
> >> >      }
> >> >  
> >> > -    ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1);
> >> > +    ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1,
> >> > +                             &error_fatal);
> >> >      if (ret) {
> >> >          return ret;
> >> >      }
> >> > diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
> >> > index 532c67107ba1d2978a76cf49f9cdc1de1dea3e11..0a688909fc606a3c9fde933667ae8c309ab527d0 100644
> >> > --- a/hw/virtio/virtio-mmio.c
> >> > +++ b/hw/virtio/virtio-mmio.c
> >> > @@ -34,6 +34,7 @@
> >> >  #include "qemu/error-report.h"
> >> >  #include "qemu/log.h"
> >> >  #include "trace.h"
> >> > +#include "qapi/error.h"
> >> >  
> >> >  static bool virtio_mmio_ioeventfd_enabled(DeviceState *d)
> >> >  {
> >> > @@ -619,7 +620,7 @@ static int virtio_mmio_load_extra_state(DeviceState *opaque, QEMUFile *f)
> >> >  {
> >> >      VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
> >> >  
> >> > -    return vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1);
> >> > +    return vmstate_load_state(f, &vmstate_virtio_mmio, proxy, 1, &error_fatal);
> >> >  }
> >> >  
> >> >  static bool virtio_mmio_has_extra_state(DeviceState *opaque)
> >> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >> > index 767216d795998708f5716a23ae16c79cd90ff489..b04faa1e5c91b5cef40e54ec41d92422d16bfc13 100644
> >> > --- a/hw/virtio/virtio-pci.c
> >> > +++ b/hw/virtio/virtio-pci.c
> >> > @@ -161,7 +161,7 @@ static int virtio_pci_load_extra_state(DeviceState *d, QEMUFile *f)
> >> >  {
> >> >      VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
> >> >  
> >> > -    return vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1);
> >> > +    return vmstate_load_state(f, &vmstate_virtio_pci, proxy, 1, &error_fatal);
> >> >  }
> >> >  
> >> >  static void virtio_pci_save_queue(DeviceState *d, int n, QEMUFile *f)
> >> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >> > index 9a81ad912e013fc254899c4e55cff1f76a6112a4..6bcafb338d1b5becadcacf092ba33a6ae4c3d194 100644
> >> > --- a/hw/virtio/virtio.c
> >> > +++ b/hw/virtio/virtio.c
> >> > @@ -3327,14 +3327,14 @@ virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
> >> >      }
> >> >  
> >> >      if (vdc->vmsd) {
> >> > -        ret = vmstate_load_state(f, vdc->vmsd, vdev, version_id);
> >> > +        ret = vmstate_load_state(f, vdc->vmsd, vdev, version_id, &error_fatal);
> >> >          if (ret) {
> >> >              return ret;
> >> >          }
> >> >      }
> >> >  
> >> >      /* Subsections */
> >> > -    ret = vmstate_load_state(f, &vmstate_virtio, vdev, 1);
> >> > +    ret = vmstate_load_state(f, &vmstate_virtio, vdev, 1, &error_fatal);
> >> >      if (ret) {
> >> >          return ret;
> >> >      }
> >> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> >> > index 1ff7bd9ac425ba67cd5ca7ad97bcf570f9e19abe..056781b1c21e737583f081594d9f88b32adfd674 100644
> >> > --- a/include/migration/vmstate.h
> >> > +++ b/include/migration/vmstate.h
> >> > @@ -1196,7 +1196,7 @@ extern const VMStateInfo vmstate_info_qlist;
> >> >      }
> >> >  
> >> >  int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> >> > -                       void *opaque, int version_id);
> >> > +                       void *opaque, int version_id, Error **errp);
> >> >  int vmstate_save_state(QEMUFile *f, const VMStateDescription *vmsd,
> >> >                         void *opaque, JSONWriter *vmdesc);
> >> >  int vmstate_save_state_with_err(QEMUFile *f, const VMStateDescription *vmsd,
> >> > diff --git a/migration/cpr.c b/migration/cpr.c
> >> > index 42ad0b0d500e5de57faf0c6517e216b2d1c0cacf..bdb24736f44e91ba59b6e622a315597c97e7f64d 100644
> >> > --- a/migration/cpr.c
> >> > +++ b/migration/cpr.c
> >> > @@ -202,6 +202,7 @@ int cpr_state_save(MigrationChannel *channel, Error **errp)
> >> >  
> >> >  int cpr_state_load(MigrationChannel *channel, Error **errp)
> >> >  {
> >> > +    ERRP_GUARD();
> >> >      int ret;
> >> >      uint32_t v;
> >> >      QEMUFile *f;
> >> > @@ -233,9 +234,9 @@ int cpr_state_load(MigrationChannel *channel, Error **errp)
> >> >          return -ENOTSUP;
> >> >      }
> >> >  
> >> > -    ret = vmstate_load_state(f, &vmstate_cpr_state, &cpr_state, 1);
> >> > +    ret = vmstate_load_state(f, &vmstate_cpr_state, &cpr_state, 1, errp);
> >> >      if (ret) {
> >> > -        error_setg(errp, "vmstate_load_state error %d", ret);
> >> > +        error_prepend(errp, "vmstate_load_state error %d: ", ret);
> >> >          qemu_fclose(f);
> >> >          return ret;
> >> >      }
> >> > diff --git a/migration/savevm.c b/migration/savevm.c
> >> > index fabbeb296ae987d0c06ba6dafda63720205fecfd..cb64f2855d46aaa7c617b3e4079a2c9e566079b2 100644
> >> > --- a/migration/savevm.c
> >> > +++ b/migration/savevm.c
> >> > @@ -969,7 +969,8 @@ static int vmstate_load(QEMUFile *f, SaveStateEntry *se)
> >> >      if (!se->vmsd) {         /* Old style */
> >> >          return se->ops->load_state(f, se->opaque, se->load_version_id);
> >> >      }
> >> > -    return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id);
> >> > +    return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id,
> >> > +                              &error_fatal);
> >> >  }
> >> >  
> >> >  static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
> >> > @@ -2839,7 +2840,8 @@ static int qemu_loadvm_state_header(QEMUFile *f)
> >> >              error_report("Configuration section missing");
> >> >              return -EINVAL;
> >> >          }
> >> > -        ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0);
> >> > +        ret = vmstate_load_state(f, &vmstate_configuration, &savevm_state, 0,
> >> > +                                 &error_fatal);
> >> >  
> >> >          if (ret) {
> >> >              return ret;
> >> > diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
> >> > index 741a588b7e18c6d37724b08a0101edc8bc74a0a5..f41670cc853c5b41ccc8def354886a8e5c1451fd 100644
> >> > --- a/migration/vmstate-types.c
> >> > +++ b/migration/vmstate-types.c
> >> > @@ -19,6 +19,7 @@
> >> >  #include "qemu/error-report.h"
> >> >  #include "qemu/queue.h"
> >> >  #include "trace.h"
> >> > +#include "qapi/error.h"
> >> >  
> >> >  /* bool */
> >> >  
> >> > @@ -543,13 +544,17 @@ static int get_tmp(QEMUFile *f, void *pv, size_t size,
> >> >                     const VMStateField *field)
> >> >  {
> >> >      int ret;
> >> > +    Error *local_err = NULL;
> >> >      const VMStateDescription *vmsd = field->vmsd;
> >> >      int version_id = field->version_id;
> >> >      void *tmp = g_malloc(size);
> >> >  
> >> >      /* Writes the parent field which is at the start of the tmp */
> >> >      *(void **)tmp = pv;
> >> > -    ret = vmstate_load_state(f, vmsd, tmp, version_id);
> >> > +    ret = vmstate_load_state(f, vmsd, tmp, version_id, &local_err);
> >> > +    if (ret < 0) {
> >> > +        warn_report_err(local_err);
> >> > +    }
> >> >      g_free(tmp);
> >> >      return ret;
> >> >  }
> >> > @@ -626,6 +631,7 @@ static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size,
> >> >                        const VMStateField *field)
> >> >  {
> >> >      int ret = 0;
> >> > +    Error *local_err = NULL;
> >> >      const VMStateDescription *vmsd = field->vmsd;
> >> >      /* size of a QTAILQ element */
> >> >      size_t size = field->size;
> >> > @@ -649,8 +655,9 @@ static int get_qtailq(QEMUFile *f, void *pv, size_t unused_size,
> >> >  
> >> >      while (qemu_get_byte(f)) {
> >> >          elm = g_malloc(size);
> >> > -        ret = vmstate_load_state(f, vmsd, elm, version_id);
> >> > +        ret = vmstate_load_state(f, vmsd, elm, version_id, &local_err);
> >> >          if (ret) {
> >> > +            warn_report_err(local_err);
> >> >              return ret;
> >> >          }
> >> >          QTAILQ_RAW_INSERT_TAIL(pv, elm, entry_offset);
> >> > @@ -772,6 +779,7 @@ static int get_gtree(QEMUFile *f, void *pv, size_t unused_size,
> >> >      GTree *tree = *pval;
> >> >      void *key, *val;
> >> >      int ret = 0;
> >> > +    Error *local_err = NULL;
> >> >  
> >> >      /* in case of direct key, the key vmsd can be {}, ie. check fields */
> >> >      if (!direct_key && version_id > key_vmsd->version_id) {
> >> > @@ -803,18 +811,20 @@ static int get_gtree(QEMUFile *f, void *pv, size_t unused_size,
> >> >              key = (void *)(uintptr_t)qemu_get_be64(f);
> >> >          } else {
> >> >              key = g_malloc0(key_size);
> >> > -            ret = vmstate_load_state(f, key_vmsd, key, version_id);
> >> > +            ret = vmstate_load_state(f, key_vmsd, key, version_id, &local_err);
> >> >              if (ret) {
> >> >                  error_report("%s : failed to load %s (%d)",
> >> >                               field->name, key_vmsd->name, ret);
> >> > +                warn_report_err(local_err);
> >> >                  goto key_error;
> >> >              }
> >> >          }
> >> >          val = g_malloc0(val_size);
> >> > -        ret = vmstate_load_state(f, val_vmsd, val, version_id);
> >> > +        ret = vmstate_load_state(f, val_vmsd, val, version_id, &local_err);
> >> >          if (ret) {
> >> >              error_report("%s : failed to load %s (%d)",
> >> >                           field->name, val_vmsd->name, ret);
> >> > +            warn_report_err(local_err);
> >> >              goto val_error;
> >> >          }
> >> >          g_tree_insert(tree, key, val);
> >> > @@ -872,6 +882,7 @@ static int get_qlist(QEMUFile *f, void *pv, size_t unused_size,
> >> >                       const VMStateField *field)
> >> >  {
> >> >      int ret = 0;
> >> > +    Error *local_err = NULL;
> >> >      const VMStateDescription *vmsd = field->vmsd;
> >> >      /* size of a QLIST element */
> >> >      size_t size = field->size;
> >> > @@ -892,10 +903,11 @@ static int get_qlist(QEMUFile *f, void *pv, size_t unused_size,
> >> >  
> >> >      while (qemu_get_byte(f)) {
> >> >          elm = g_malloc(size);
> >> > -        ret = vmstate_load_state(f, vmsd, elm, version_id);
> >> > +        ret = vmstate_load_state(f, vmsd, elm, version_id, &local_err);
> >> >          if (ret) {
> >> >              error_report("%s: failed to load %s (%d)", field->name,
> >> >                           vmsd->name, ret);
> >> > +            warn_report_err(local_err);
> >> >              g_free(elm);
> >> >              return ret;
> >> >          }
> >> > diff --git a/migration/vmstate.c b/migration/vmstate.c
> >> > index 6108c7fe283a5013ce42ea9987723c489aef26a2..1cd609a1d598995af1e51d1f4d58d68133f1426d 100644
> >> > --- a/migration/vmstate.c
> >> > +++ b/migration/vmstate.c
> >> > @@ -132,29 +132,34 @@ 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)
> >> >  {
> >> > +    ERRP_GUARD();
> >> >      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 for: '%s', "
> >> 
> >> "VM" pre load is a little ambiguous. Simply "pre load" or "pre load
> >> hook" is better.
> > Sure, will do this.
> >> 
> >> > +                       "version_id: %d, minimum version_id: %d, ret: %d",
> >> > +                       vmsd->name, vmsd->version_id, vmsd->minimum_version_id,
> >> > +                       ret);
> >> >              return ret;
> >> >          }
> >> >      }
> >> > @@ -192,13 +197,20 @@ 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);
> >> > +                    if (ret < 0) {
> >> > +                        error_setg(errp,
> >> > +                                   "Failed to get info for %s: %d",
> >> > +                                   inner_field->name, ret);
> >> 
> >> "get info" is not correct. This is the type-specific getter
> >> invocation. Because the migration (for the most part) is a stream, each
> >> type provides it's own getter which knows about the size of the field
> >> and any particularities such as magic values. So:
> >> 
> >> error_setg(errp, "Failed to load element of type %s for %s: %d",
> >>            info->name, inner_field->name, ret);
> > yes, will change the message. Thanks.
> >> 
> >> > +                    }
> >> >                  }
> >> >  
> >> >                  /* If we used a fake temp field.. free it now */
> >> > @@ -208,30 +220,42 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> >> >  
> >> >                  if (ret >= 0) {
> >> >                      ret = qemu_file_get_error(f);
> >> > +                    if (ret < 0) {
> >> > +                        error_setg(errp, "Failed to load %s state: %d",
> >> > +                                   vmsd->name, ret);
> >> 
> >> We could go a little more specific here, it's useful to know whether the
> >> error was on the transport side, rather than something logical with the
> >> migrated data. I don't really care about the actual string, but one
> >> suggestion is "stream error":
> >> 
> >> error_setg(errp, "Failed to load %s state: stream error: %d",
> >>            vmsd->name, ret);
> > yes, will add stream error. Thanks.
> >> 
> >> > +                    }
> >> >                  }
> >> >                  if (ret < 0) {
> >> >                      qemu_file_set_error(f, ret);
> >> > -                    error_report("Failed to load %s:%s", vmsd->name,
> >> > -                                 field->name);
> >> > +                    error_prepend(errp,
> >> > +                                  "Failed to load %s:%s version_id: %d: ",
> >> 
> >> Usage of : is inconsistent with below /
> > Since we used error_prepend here, we need an extra space after the colon ':'
> > That way the next error string looks readable.
> 
> Right, that's not what I'm talking about. What's inconsistent is: %s:%s
> vs. %s/%s
> 
> >> 
> >> > +                                   vmsd->name, field->name, vmsd->version_id);
> >> >                      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 version_id: %d",
> 
> here^
I see, I shall amend. It was copied as is from error_report().
Thanks.
> 


Regards,
Arun

Re: [PATCH v11 09/27] migration: push Error **errp into qemu_loadvm_state_main()

[Arun Menon]

Hi Fabiano,
Thanks for the review.
On Fri, Aug 15, 2025 at 04:23:42PM -0300, Fabiano Rosas wrote:
> Arun Menon <armenon@redhat.com> writes:
> 
> > 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 qemu_loadvm_state_main() must report an error
> > in errp, in case of failure.
> > loadvm_process_command also sets the errp object explicitly.
> >
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > Signed-off-by: Arun Menon <armenon@redhat.com>
> > ---
> >  migration/colo.c   |  5 +++--
> >  migration/savevm.c | 36 +++++++++++++++++++-----------------
> >  migration/savevm.h |  3 ++-
> >  3 files changed, 24 insertions(+), 20 deletions(-)
> >
> > diff --git a/migration/colo.c b/migration/colo.c
> > index 0ba22ee76a13e313793f653f43a728e3c433bbc1..a96e4dba15516b71d1b315c736c3b4879ff04e58 100644
> > --- a/migration/colo.c
> > +++ b/migration/colo.c
> > @@ -659,6 +659,7 @@ void migrate_start_colo_process(MigrationState *s)
> >  static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
> >                        QEMUFile *fb, QIOChannelBuffer *bioc, Error **errp)
> >  {
> > +    ERRP_GUARD();
> 
> With my suggestion below, this goes away.
Yeah.
> 
> >      uint64_t total_size;
> >      uint64_t value;
> >      Error *local_err = NULL;
> > @@ -686,11 +687,11 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
> >  
> >      bql_lock();
> >      cpu_synchronize_all_states();
> > -    ret = qemu_loadvm_state_main(mis->from_src_file, mis);
> > +    ret = qemu_loadvm_state_main(mis->from_src_file, mis, errp);
> >      bql_unlock();
> >  
> >      if (ret < 0) {
> > -        error_setg(errp, "Load VM's live state (ram) error");
> > +        error_prepend(errp, "Load VM's live state (ram) error: ");
> 
> Another one to leave out. There's enough information downstream
> already. Also, this "(ram)" doesn't look right.
Yes, shall remove it.
> 
> >          return;
> >      }
> >  
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 70e021597d884030c4a0dc2a7bc27d42a7371797..9ec07892cd6ea666431410657c840b6325377d97 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2105,7 +2105,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> >      qemu_file_set_blocking(f, true);
> >  
> >      /* TODO: sanity check that only postcopiable data will be loaded here */
> > -    load_res = qemu_loadvm_state_main(f, mis);
> > +    load_res = qemu_loadvm_state_main(f, mis, &error_fatal);
> >  
> >      /*
> >       * This is tricky, but, mis->from_src_file can change after it
> > @@ -2407,6 +2407,7 @@ static int loadvm_postcopy_handle_resume(MigrationIncomingState *mis)
> >   */
> >  static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis, Error **errp)
> >  {
> > +    ERRP_GUARD();
> >      int ret;
> >      size_t length;
> >      QIOChannelBuffer *bioc;
> > @@ -2456,9 +2457,9 @@ static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis, Error **errp)
> >          qemu_coroutine_yield();
> >      } while (1);
> >  
> > -    ret = qemu_loadvm_state_main(packf, mis);
> > +    ret = qemu_loadvm_state_main(packf, mis, errp);
> >      if (ret < 0) {
> > -        error_setg(errp, "VM state load failed: %d", ret);
> > +        error_prepend(errp, "Loading VM state failed: %d: ", ret);
> 
> This is getting out of hand for code review, may I suggest you
> artificially trigger these errors, look at the resulting message and
> remove all the unnecessary wrapping? Each error_prepend is a candidate
> for removal if it will just state "load failed".
> 
> Using error_prepend partly defeats the purpose of propagating errp. We
> should only use it when there's valuable information to be provided.
I understand. I shall remove error_prepend from where it is not required.
> 
> >      }
> >      trace_loadvm_handle_cmd_packaged_main(ret);
> >      qemu_fclose(packf);
> > @@ -3080,18 +3081,21 @@ static bool postcopy_pause_incoming(MigrationIncomingState *mis)
> >      return true;
> >  }
> >  
> > -int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> > +int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis,
> > +                           Error **errp)
> >  {
> > +    ERRP_GUARD();
> >      uint8_t section_type;
> >      int ret = 0;
> > -    Error *local_err = NULL;
> >  
> >  retry:
> >      while (true) {
> >          section_type = qemu_get_byte(f);
> >  
> > -        ret = qemu_file_get_error_obj_any(f, mis->postcopy_qemufile_dst, NULL);
> > +        ret = qemu_file_get_error_obj_any(f, mis->postcopy_qemufile_dst, errp);
> >          if (ret) {
> > +            error_prepend(errp, "Failed to load device state section ID: %d: ",
> > +                          ret);
> 
> We could drop some extra words here, the term 'section' is already quite
> representative.
> 
> "Failed to load section ID: stream error: %d: "
Thanks, will do.
> 
> 
> >              break;
> >          }
> >  
> > @@ -3112,10 +3116,7 @@ retry:
> >              }
> >              break;
> >          case QEMU_VM_COMMAND:
> > -            ret = loadvm_process_command(f, &local_err);
> > -            if (ret < 0) {
> > -                warn_report_err(local_err);
> > -            }
> > +            ret = loadvm_process_command(f, errp);
> 
> Good.
> 
> >              trace_qemu_loadvm_state_section_command(ret);
> >              if ((ret < 0) || (ret == LOADVM_QUIT)) {
> >                  goto out;
> > @@ -3125,7 +3126,7 @@ retry:
> >              /* This is the end of migration */
> >              goto out;
> >          default:
> > -            error_report("Unknown savevm section type %d", section_type);
> > +            error_setg(errp, "Unknown savevm section type %d", section_type);
> 
> Not sure if they're referring to "savevm" here as a generic term for
> vmstate/migration or if it was intended to say: "savevm wrote a section
> type that this loadvm instance doesn't understand".
> 
> Since you're here, could you fix this? Migration errors from source and
> destination are often interleaved in logs, we don't want to see the
> "savevm" word in a destination-side error message. Just put a small note
> in the commit message, no need for another patch.
Yes, shall remove savevm and amend the commit message.
> 
> >              ret = -EINVAL;
> >              goto out;
> >          }
> > @@ -3133,6 +3134,9 @@ retry:
> >  
> >  out:
> >      if (ret < 0) {
> > +        if (*errp == NULL) {
> > +            error_setg(errp, "Loading VM state failed: %d", ret);
> > +        }
> 
> Another candidate for removal, then we avoid having to dereference errp.
This is added so that errp is set in case of failure in paths that are not
converted yet in the series. The out label comes in handy.
This is removed in the future patch 4648a1849a
> 
> >          qemu_file_set_error(f, ret);
> >  
> >          /* Cancel bitmaps incoming regardless of recovery */
> > @@ -3153,6 +3157,7 @@ out:
> >              migrate_postcopy_ram() && postcopy_pause_incoming(mis)) {
> >              /* Reset f to point to the newly created channel */
> >              f = mis->from_src_file;
> > +            error_free_or_abort(errp);
> 
> What's this about?
The errp needs to be freed and reset to NULL if we go into retry and
intend to use the variable again.
> 
> >              goto retry;
> >          }
> >      }
> > @@ -3186,10 +3191,7 @@ int qemu_loadvm_state(QEMUFile *f, Error **errp)
> >  
> >      cpu_synchronize_all_pre_loadvm();
> >  
> > -    ret = qemu_loadvm_state_main(f, mis);
> > -    if (ret < 0) {
> > -        error_setg(errp, "Load VM state failed: %d", ret);
> > -    }
> > +    ret = qemu_loadvm_state_main(f, mis, errp);
> >      qemu_event_set(&mis->main_thread_load_event);
> >  
> >      trace_qemu_loadvm_state_post_main(ret);
> > @@ -3270,9 +3272,9 @@ int qemu_load_device_state(QEMUFile *f, Error **errp)
> >      int ret;
> >  
> >      /* Load QEMU_VM_SECTION_FULL section */
> > -    ret = qemu_loadvm_state_main(f, mis);
> > +    ret = qemu_loadvm_state_main(f, mis, errp);
> >      if (ret < 0) {
> > -        error_setg(errp, "Failed to load device state: %d", ret);
> > +        error_prepend(errp, "Failed to load device state: %d: ", ret);
Maybe this can also be removed.
> >          return ret;
> >      }
> >  
> > diff --git a/migration/savevm.h b/migration/savevm.h
> > index b12681839f0b1afa3255e45215d99c13a224b19f..c337e3e3d111a7f28a57b90f61e8f70b71803d4e 100644
> > --- a/migration/savevm.h
> > +++ b/migration/savevm.h
> > @@ -66,7 +66,8 @@ int qemu_save_device_state(QEMUFile *f);
> >  
> >  int qemu_loadvm_state(QEMUFile *f, Error **errp);
> >  void qemu_loadvm_state_cleanup(MigrationIncomingState *mis);
> > -int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis);
> > +int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis,
> > +                           Error **errp);
> >  int qemu_load_device_state(QEMUFile *f, Error **errp);
> >  int qemu_loadvm_approve_switchover(void);
> >  int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
> 

Regards,
Arun

Re: [PATCH v11 04/27] migration: push Error **errp into vmstate_load()

[Arun Menon]

Hi Fabiano,
Thanks for the review.

On Fri, Aug 15, 2025 at 01:41:40PM -0300, Fabiano Rosas wrote:
> Arun Menon <armenon@redhat.com> writes:
> 
> > 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() must report an error
> > in errp, in case of failure.
> >
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Signed-off-by: Arun Menon <armenon@redhat.com>
> > ---
> >  migration/savevm.c | 20 +++++++++++++++-----
> >  1 file changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 0c445a957fc99f826e6753ed3795bcdd51f1e3f5..7f79461844105bf672314c3325caee9cdb654c27 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -963,14 +963,20 @@ void vmstate_unregister(VMStateIf *obj, const VMStateDescription *vmsd,
> >      }
> >  }
> >  
> > -static int vmstate_load(QEMUFile *f, SaveStateEntry *se)
> > +static int vmstate_load(QEMUFile *f, SaveStateEntry *se, Error **errp)
> >  {
> > +    int ret;
> >      trace_vmstate_load(se->idstr, se->vmsd ? se->vmsd->name : "(old)");
> >      if (!se->vmsd) {         /* Old style */
> > -        return se->ops->load_state(f, se->opaque, se->load_version_id);
> > +        ret = se->ops->load_state(f, se->opaque, se->load_version_id);
> > +        if (ret < 0) {
> > +            error_setg(errp, "Failed to load VM version_id: %d, ret: %d",
> 
> "VM" is ambiguous. I'd use "vmstate".
Sure, will do this.
> 
> > +                       se->load_version_id, ret);
> > +        }
> > +        return ret;
> >      }
> >      return vmstate_load_state(f, se->vmsd, se->opaque, se->load_version_id,
> > -                              &error_fatal);
> > +                              errp);
> >  }
> >  
> >  static void vmstate_save_old_style(QEMUFile *f, SaveStateEntry *se,
> > @@ -2692,6 +2698,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
> >      SaveStateEntry *se;
> >      char idstr[256];
> >      int ret;
> > +    Error *local_err = NULL;
> >  
> >      /* Read section start */
> >      section_id = qemu_get_be32(f);
> > @@ -2741,10 +2748,11 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
> >          start_ts = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
> >      }
> >  
> > -    ret = vmstate_load(f, se);
> > +    ret = vmstate_load(f, se, &local_err);
> >      if (ret < 0) {
> >          error_report("error while loading state for instance 0x%"PRIx32" of"
> >                       " device '%s'", instance_id, idstr);
> > +        warn_report_err(local_err);
> 
> I was about to ask why you're using a warning here, but I see you remove
> it futher in the series. That's fine, but this kind of thing should be
> mentioned in the commit message, otherwise it gets confusing to review.
Sure, I will add it in the commit message.
> 
> >          return ret;
> >      }
> >  
> > @@ -2769,6 +2777,7 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
> >      uint32_t section_id;
> >      SaveStateEntry *se;
> >      int ret;
> > +    Error *local_err = NULL;
> >  
> >      section_id = qemu_get_be32(f);
> >  
> > @@ -2794,10 +2803,11 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
> >          start_ts = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
> >      }
> >  
> > -    ret = vmstate_load(f, se);
> > +    ret = vmstate_load(f, se, &local_err);
> >      if (ret < 0) {
> >          error_report("error while loading state section id %d(%s)",
> >                       section_id, se->idstr);
> > +        warn_report_err(local_err);
> >          return ret;
> >      }
> 
Regards,
Arun Menon

Re: [PATCH v11 19/27] migration: push Error **errp into loadvm_handle_recv_bitmap()

[Arun Menon]

Hi Fabiano,
Thanks for the review.

On Fri, Aug 15, 2025 at 04:51:04PM -0300, Fabiano Rosas wrote:
> Arun Menon <armenon@redhat.com> writes:
> 
> > 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 loadvm_handle_recv_bitmap() must report an error
> > in errp, in case of failure.
> >
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > Signed-off-by: Arun Menon <armenon@redhat.com>
> > ---
> >  migration/savevm.c | 21 ++++++++++-----------
> >  1 file changed, 10 insertions(+), 11 deletions(-)
> >
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 9098c4bd3394d7b9ed77e20afbb26fd9c9be6550..a7aede1b3df9164e322e68f3889df7c4166876f5 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2480,32 +2480,35 @@ static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis, Error **errp)
> >   * len (1 byte) + ramblock_name (<255 bytes)
> >   */
> >  static int loadvm_handle_recv_bitmap(MigrationIncomingState *mis,
> > -                                     uint16_t len)
> > +                                     uint16_t len, Error **errp)
> >  {
> >      QEMUFile *file = mis->from_src_file;
> >      RAMBlock *rb;
> >      char block_name[256];
> >      size_t cnt;
> > +    int ret;
> >  
> >      cnt = qemu_get_counted_string(file, block_name);
> >      if (!cnt) {
> > -        error_report("%s: failed to read block name", __func__);
> > +        error_setg(errp, "failed to read block name: %s", block_name);
> 
> Could we not print the buffer that's just failed to be written? As a
> matter of principle =)
yes, we must not, its content will be empty. Thanks
> 
> >          return -EINVAL;
> >      }
> >  
> >      /* Validate before using the data */
> > -    if (qemu_file_get_error(file)) {
> > -        return qemu_file_get_error(file);
> > +    ret = qemu_file_get_error(file);
> > +    if (ret < 0) {
> > +        error_setg(errp, "migration stream has error: %d", ret);
> 
> I've been suggesting "stream error:", probably best to keep it uniform.
Sure, will do.
> 
> > +        return ret;
> >      }
> >  
> >      if (len != cnt + 1) {
> > -        error_report("%s: invalid payload length (%d)", __func__, len);
> > +        error_setg(errp, "invalid payload length (%d)", len);
> >          return -EINVAL;
> >      }
> >  
> >      rb = qemu_ram_block_by_name(block_name);
> >      if (!rb) {
> > -        error_report("%s: block '%s' not found", __func__, block_name);
> > +        error_setg(errp, "block '%s' not found", block_name);
> >          return -EINVAL;
> >      }
> >  
> > @@ -2642,11 +2645,7 @@ static int loadvm_process_command(QEMUFile *f, Error **errp)
> >          return 0;
> >  
> >      case MIG_CMD_RECV_BITMAP:
> > -        ret = loadvm_handle_recv_bitmap(mis, len);
> > -        if (ret < 0) {
> > -            error_setg(errp, "Failed to load device state command: %d", ret);
> > -        }
> > -        return ret;
> > +        return loadvm_handle_recv_bitmap(mis, len, errp);
> >  
> >      case MIG_CMD_ENABLE_COLO:
> >          ret = loadvm_process_enable_colo(mis);
> 

Regards,
Arun

Re: [PATCH v11 11/27] migration: push Error **errp into qemu_loadvm_section_part_end()

[Arun Menon]

Hi Fabiano,
Thanks for the review.

On Fri, Aug 15, 2025 at 04:35:36PM -0300, Fabiano Rosas wrote:
> Arun Menon <armenon@redhat.com> writes:
> 
> > 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 qemu_loadvm_section_part_end() must report an error
> > in errp, in case of failure.
> >
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Signed-off-by: Arun Menon <armenon@redhat.com>
> > ---
> >  migration/savevm.c | 23 ++++++++++-------------
> >  1 file changed, 10 insertions(+), 13 deletions(-)
> >
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 77408347c1f1ca7eb3a04f8f130c20a5a81f6db2..ff2e4f75e070d0f452414f28435905928b1480a7 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2806,21 +2806,20 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type, Error **errp)
> >  }
> >  
> >  static int
> > -qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
> > +qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type, Error **errp)
> >  {
> > +    ERRP_GUARD();
> >      bool trace_downtime = (type == QEMU_VM_SECTION_END);
> >      int64_t start_ts, end_ts;
> >      uint32_t section_id;
> >      SaveStateEntry *se;
> >      int ret;
> > -    Error *local_err = NULL;
> >  
> >      section_id = qemu_get_be32(f);
> >  
> >      ret = qemu_file_get_error(f);
> >      if (ret) {
> > -        error_report("%s: Failed to read section ID: %d",
> > -                     __func__, ret);
> > +        error_setg(errp, "Failed to read section ID: %d", ret);
> >          return ret;
> >      }
> >  
> > @@ -2831,7 +2830,7 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
> >          }
> >      }
> >      if (se == NULL) {
> > -        error_report("Unknown savevm section %d", section_id);
> > +        error_setg(errp, "Unknown savevm section %d", section_id);
> 
> Drop "savevm" please.
yes, will do.
> 
> >          return -EINVAL;
> >      }
> >  
> > @@ -2839,11 +2838,10 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
> >          start_ts = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
> >      }
> >  
> > -    ret = vmstate_load(f, se, &local_err);
> > +    ret = vmstate_load(f, se, errp);
> >      if (ret < 0) {
> > -        error_report("error while loading state section id %d(%s)",
> > -                     section_id, se->idstr);
> > -        warn_report_err(local_err);
> > +        error_prepend(errp, "error while loading state section id %d(%s): ",
> > +                      section_id, se->idstr);
> >          return ret;
> >      }
> >  
> > @@ -2854,6 +2852,8 @@ qemu_loadvm_section_part_end(QEMUFile *f, uint8_t type)
> >      }
> >  
> >      if (!check_section_footer(f, se)) {
> > +        error_setg(errp, "Check section footer error, section_id: %d",
> 
> This became not very grammatical, maybe drop the "Check" word.
sure, will do.
> 
> > +                   section_id);
> >          return -EINVAL;
> >      }
> >  
> > @@ -3112,7 +3112,7 @@ retry:
> >              break;
> >          case QEMU_VM_SECTION_PART:
> >          case QEMU_VM_SECTION_END:
> > -            ret = qemu_loadvm_section_part_end(f, section_type);
> > +            ret = qemu_loadvm_section_part_end(f, section_type, errp);
> >              if (ret < 0) {
> >                  goto out;
> >              }
> > @@ -3136,9 +3136,6 @@ retry:
> >  
> >  out:
> >      if (ret < 0) {
> > -        if (*errp == NULL) {
> > -            error_setg(errp, "Loading VM state failed: %d", ret);
> > -        }
> 
> Good. Could have been mentioned in that patch's commit message, or even
> a /* temporary */ comment in the code.
Yes, I shall add it in the commit message.
> 
> >          qemu_file_set_error(f, ret);
> >  
> >          /* Cancel bitmaps incoming regardless of recovery */
> 
Regards,
Arun

Re: [PATCH v11 10/27] migration: push Error **errp into qemu_loadvm_section_start_full()

[Arun Menon]

Hi Fabiano,
Thanks for the review.
On Fri, Aug 15, 2025 at 04:29:49PM -0300, Fabiano Rosas wrote:
> Arun Menon <armenon@redhat.com> writes:
> 
> > 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 qemu_loadvm_section_start_full() must report an error
> > in errp, in case of failure.
> >
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Signed-off-by: Arun Menon <armenon@redhat.com>
> > ---
> >  migration/savevm.c | 38 ++++++++++++++++++++------------------
> >  1 file changed, 20 insertions(+), 18 deletions(-)
> >
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 9ec07892cd6ea666431410657c840b6325377d97..77408347c1f1ca7eb3a04f8f130c20a5a81f6db2 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2724,21 +2724,21 @@ static bool check_section_footer(QEMUFile *f, SaveStateEntry *se)
> >  }
> >  
> >  static int
> > -qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
> > +qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type, Error **errp)
> >  {
> > +    ERRP_GUARD();
> >      bool trace_downtime = (type == QEMU_VM_SECTION_FULL);
> >      uint32_t instance_id, version_id, section_id;
> >      int64_t start_ts, end_ts;
> >      SaveStateEntry *se;
> >      char idstr[256];
> >      int ret;
> > -    Error *local_err = NULL;
> >  
> >      /* Read section start */
> >      section_id = qemu_get_be32(f);
> >      if (!qemu_get_counted_string(f, idstr)) {
> > -        error_report("Unable to read ID string for section %u",
> > -                     section_id);
> > +        error_setg(errp, "Unable to read ID string for section %u",
> > +                   section_id);
> >          return -EINVAL;
> >      }
> >      instance_id = qemu_get_be32(f);
> > @@ -2746,8 +2746,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
> >  
> >      ret = qemu_file_get_error(f);
> >      if (ret) {
> > -        error_report("%s: Failed to read instance/version ID: %d",
> > -                     __func__, ret);
> > +        error_setg(errp, "Failed to read instance/version ID: %d", ret);
> >          return ret;
> >      }
> >  
> > @@ -2756,17 +2755,17 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
> >      /* Find savevm section */
> >      se = find_se(idstr, instance_id);
> >      if (se == NULL) {
> > -        error_report("Unknown savevm section or instance '%s' %"PRIu32". "
> > -                     "Make sure that your current VM setup matches your "
> > -                     "saved VM setup, including any hotplugged devices",
> > -                     idstr, instance_id);
> > +        error_setg(errp, "Unknown savevm section or instance '%s' %"PRIu32". "
> 
> Drop the "savevm" please.
yes. Thanks.
> 
> > +                   "Make sure that your current VM setup matches your "
> > +                   "saved VM setup, including any hotplugged devices",
> > +                   idstr, instance_id);
> >          return -EINVAL;
> >      }
> >  
> >      /* Validate version */
> >      if (version_id > se->version_id) {
> > -        error_report("savevm: unsupported version %d for '%s' v%d",
> > -                     version_id, idstr, se->version_id);
> > +        error_setg(errp, "savevm: unsupported version %d for '%s' v%d",
> 
> Same.
yes
> 
> > +                   version_id, idstr, se->version_id);
> >          return -EINVAL;
> >      }
> >      se->load_version_id = version_id;
> > @@ -2774,7 +2773,7 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
> >  
> >      /* Validate if it is a device's state */
> >      if (xen_enabled() && se->is_ram) {
> > -        error_report("loadvm: %s RAM loading not allowed on Xen", idstr);
> > +        error_setg(errp, "loadvm: %s RAM loading not allowed on Xen", idstr);
> >          return -EINVAL;
> >      }
> >  
> > @@ -2782,11 +2781,11 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
> >          start_ts = qemu_clock_get_us(QEMU_CLOCK_REALTIME);
> >      }
> >  
> > -    ret = vmstate_load(f, se, &local_err);
> > +    ret = vmstate_load(f, se, errp);
> >      if (ret < 0) {
> > -        error_report("error while loading state for instance 0x%"PRIx32" of"
> > -                     " device '%s'", instance_id, idstr);
> > -        warn_report_err(local_err);
> > +        error_prepend(errp,
> > +                      "error while loading state for instance 0x%"PRIx32" of"
> > +                      " device '%s': ", instance_id, idstr);
> >          return ret;
> >      }
> >  
> > @@ -2797,6 +2796,9 @@ qemu_loadvm_section_start_full(QEMUFile *f, uint8_t type)
> >      }
> >  
> >      if (!check_section_footer(f, se)) {
> > +        error_setg(errp, "Reading footer section of instance "
> > +                   "0x%"PRIx32" of device '%s' for version_id: %d failed",
> > +                   instance_id, idstr, version_id);
> 
> check_section_footer() already has messages saying something went wrong
> with the footer. Make sure you're not duplicating information. If
> necessary, trim it either here or there.
yes. The function indeed reports error. I needed to set errp though.
I will trim the message.
> 
> >          return -EINVAL;
> >      }
> >  
> > @@ -3103,7 +3105,7 @@ retry:
> >          switch (section_type) {
> >          case QEMU_VM_SECTION_START:
> >          case QEMU_VM_SECTION_FULL:
> > -            ret = qemu_loadvm_section_start_full(f, section_type);
> > +            ret = qemu_loadvm_section_start_full(f, section_type, errp);
> >              if (ret < 0) {
> >                  goto out;
> >              }
> 

Regards,
Arun

Re: [PATCH v11 08/27] migration: push Error **errp into qemu_load_device_state()

[Arun Menon]

Hi Fabiano,
Thanks for the review.

On Fri, Aug 15, 2025 at 03:58:50PM -0300, Fabiano Rosas wrote:
> Arun Menon <armenon@redhat.com> writes:
> 
> > 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 qemu_load_device_state() must report an error
> > in errp, in case of failure.
> >
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Signed-off-by: Arun Menon <armenon@redhat.com>
> > ---
> >  migration/colo.c   | 4 ++--
> >  migration/savevm.c | 5 +++--
> >  migration/savevm.h | 2 +-
> >  3 files changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/migration/colo.c b/migration/colo.c
> > index e0f713c837f5da25d67afbd02ceb6c54024ca3af..0ba22ee76a13e313793f653f43a728e3c433bbc1 100644
> > --- a/migration/colo.c
> > +++ b/migration/colo.c
> > @@ -729,9 +729,9 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
> >      bql_lock();
> >      vmstate_loading = true;
> >      colo_flush_ram_cache();
> > -    ret = qemu_load_device_state(fb);
> > +    ret = qemu_load_device_state(fb, errp);
> >      if (ret < 0) {
> > -        error_setg(errp, "COLO: load device state failed");
> > +        error_prepend(errp, "COLO: load device state failed: ");
> 
> This will say: "COLO: load device state failed: Failed to load device
> state: %d"
> 
> Just remove it.
sure, will do.
> 
> >          vmstate_loading = false;
> >          bql_unlock();
> >          return;
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 05dc392bdf4e19f340bc72bf66ba0543d56868a5..70e021597d884030c4a0dc2a7bc27d42a7371797 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -3263,15 +3263,16 @@ int qemu_loadvm_state(QEMUFile *f, Error **errp)
> >      return ret;
> >  }
> >  
> > -int qemu_load_device_state(QEMUFile *f)
> > +int qemu_load_device_state(QEMUFile *f, Error **errp)
> >  {
> > +    ERRP_GUARD();
> 
> Is this needed here?
Not needed. Will remove, as we are not using error_prepend()
> 
> >      MigrationIncomingState *mis = migration_incoming_get_current();
> >      int ret;
> >  
> >      /* Load QEMU_VM_SECTION_FULL section */
> >      ret = qemu_loadvm_state_main(f, mis);
> >      if (ret < 0) {
> > -        error_report("Failed to load device state: %d", ret);
> > +        error_setg(errp, "Failed to load device state: %d", ret);
> >          return ret;
> >      }
> >  
> > diff --git a/migration/savevm.h b/migration/savevm.h
> > index b80770b7461a60e2ad6ba5e24a7baeae73d90955..b12681839f0b1afa3255e45215d99c13a224b19f 100644
> > --- a/migration/savevm.h
> > +++ b/migration/savevm.h
> > @@ -67,7 +67,7 @@ int qemu_save_device_state(QEMUFile *f);
> >  int qemu_loadvm_state(QEMUFile *f, Error **errp);
> >  void qemu_loadvm_state_cleanup(MigrationIncomingState *mis);
> >  int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis);
> > -int qemu_load_device_state(QEMUFile *f);
> > +int qemu_load_device_state(QEMUFile *f, Error **errp);
> >  int qemu_loadvm_approve_switchover(void);
> >  int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
> >          bool in_postcopy);
> 

Regards,
Arun

Re: [PATCH v11 07/27] migration: push Error **errp into qemu_loadvm_state()

[Arun Menon]

Hi Fabiano,
Thanks for the review.

On Fri, Aug 15, 2025 at 03:55:19PM -0300, Fabiano Rosas wrote:
> Arun Menon <armenon@redhat.com> writes:
> 
> > 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 qemu_loadvm_state() must report an error
> > in errp, in case of failure.
> >
> > When postcopy live migration runs, the device states are loaded by
> > both the qemu coroutine process_incoming_migration_co() and the
> > postcopy_ram_listen_thread(). Therefore, it is important that the
> > coroutine also reports the error in case of failure, with
> > error_report_err(). Otherwise, the source qemu will not display
> > any errors before going into the postcopy pause state.
> >
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Signed-off-by: Arun Menon <armenon@redhat.com>
> > ---
> >  migration/migration.c |  9 +++++----
> >  migration/savevm.c    | 31 +++++++++++++++++++------------
> >  migration/savevm.h    |  2 +-
> >  3 files changed, 25 insertions(+), 17 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 10c216d25dec01f206eacad2edd24d21f00e614c..c6768d88f45c870c7fad9b9957300766ff69effc 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -881,7 +881,7 @@ process_incoming_migration_co(void *opaque)
> >                        MIGRATION_STATUS_ACTIVE);
> >  
> >      mis->loadvm_co = qemu_coroutine_self();
> > -    ret = qemu_loadvm_state(mis->from_src_file);
> > +    ret = qemu_loadvm_state(mis->from_src_file, &local_err);
> >      mis->loadvm_co = NULL;
> >  
> >      trace_vmstate_downtime_checkpoint("dst-precopy-loadvm-completed");
> > @@ -908,7 +908,8 @@ process_incoming_migration_co(void *opaque)
> >      }
> >  
> >      if (ret < 0) {
> > -        error_setg(&local_err, "load of migration failed: %s", strerror(-ret));
> > +        error_prepend(&local_err, "load of migration failed: %s: ",
> > +                      strerror(-ret));
> >          goto fail;
> >      }
> >  
> > @@ -924,13 +925,13 @@ fail:
> >      migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
> >                        MIGRATION_STATUS_FAILED);
> >      migrate_set_error(s, local_err);
> > -    error_free(local_err);
> > +    error_report_err(local_err);
> >  
> >      migration_incoming_state_destroy();
> >  
> >      if (mis->exit_on_error) {
> >          WITH_QEMU_LOCK_GUARD(&s->error_mutex) {
> > -            error_report_err(s->error);
> > +            error_free(s->error);
> >              s->error = NULL;
> >          }
> >  
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 4f8c40e284a1b199d12f3c7dd61212b3e0e057c9..05dc392bdf4e19f340bc72bf66ba0543d56868a5 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -3159,28 +3159,24 @@ out:
> >      return ret;
> >  }
> >  
> > -int qemu_loadvm_state(QEMUFile *f)
> > +int qemu_loadvm_state(QEMUFile *f, Error **errp)
> >  {
> >      MigrationState *s = migrate_get_current();
> >      MigrationIncomingState *mis = migration_incoming_get_current();
> > -    Error *local_err = NULL;
> >      int ret;
> >  
> > -    if (qemu_savevm_state_blocked(&local_err)) {
> > -        error_report_err(local_err);
> > +    if (qemu_savevm_state_blocked(errp)) {
> >          return -EINVAL;
> >      }
> >  
> >      qemu_loadvm_thread_pool_create(mis);
> >  
> > -    ret = qemu_loadvm_state_header(f, &local_err);
> > +    ret = qemu_loadvm_state_header(f, errp);
> >      if (ret) {
> > -        error_report_err(local_err);
> >          return ret;
> >      }
> >  
> > -    if (qemu_loadvm_state_setup(f, &local_err) != 0) {
> > -        error_report_err(local_err);
> > +    if (qemu_loadvm_state_setup(f, errp) != 0) {
> >          return -EINVAL;
> >      }
> >  
> > @@ -3191,6 +3187,9 @@ int qemu_loadvm_state(QEMUFile *f)
> >      cpu_synchronize_all_pre_loadvm();
> >  
> >      ret = qemu_loadvm_state_main(f, mis);
> > +    if (ret < 0) {
> > +        error_setg(errp, "Load VM state failed: %d", ret);
> > +    }
> >      qemu_event_set(&mis->main_thread_load_event);
> >  
> >      trace_qemu_loadvm_state_post_main(ret);
> > @@ -3208,8 +3207,14 @@ int qemu_loadvm_state(QEMUFile *f)
> >          if (migrate_has_error(migrate_get_current()) ||
> >              !qemu_loadvm_thread_pool_wait(s, mis)) {
> >              ret = -EINVAL;
> > +            error_setg(errp,
> > +                       "Error while loading VM state: "
> > +                       "Migration stream has error");
> 
> The stream error is the one below. Just keep a generic message here
> because we'll propagate the error from qemu_loadvm_state_main() later in
> the series.
Thanks for clarifying. Will do.
> 
> >          } else {
> >              ret = qemu_file_get_error(f);
> > +            if (ret < 0) {
> > +                error_setg(errp, "Error while loading vmstate : %d", ret);
> 
> + stream error:
Thanks.
> 
> > +            }
> >          }
> >      }
> >      /*
> > @@ -3474,6 +3479,7 @@ void qmp_xen_save_devices_state(const char *filename, bool has_live, bool live,
> >  
> >  void qmp_xen_load_devices_state(const char *filename, Error **errp)
> >  {
> > +    ERRP_GUARD();
> >      QEMUFile *f;
> >      QIOChannelFile *ioc;
> >      int ret;
> > @@ -3495,10 +3501,10 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
> >      f = qemu_file_new_input(QIO_CHANNEL(ioc));
> >      object_unref(OBJECT(ioc));
> >  
> > -    ret = qemu_loadvm_state(f);
> > +    ret = qemu_loadvm_state(f, errp);
> >      qemu_fclose(f);
> >      if (ret < 0) {
> > -        error_setg(errp, "loading Xen device state failed");
> > +        error_prepend(errp, "loading Xen device state failed: ");
> >      }
> >      migration_incoming_state_destroy();
> >  }
> > @@ -3506,6 +3512,7 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
> >  bool load_snapshot(const char *name, const char *vmstate,
> >                     bool has_devices, strList *devices, Error **errp)
> >  {
> > +    ERRP_GUARD();
> >      BlockDriverState *bs_vm_state;
> >      QEMUSnapshotInfo sn;
> >      QEMUFile *f;
> > @@ -3569,13 +3576,13 @@ bool load_snapshot(const char *name, const char *vmstate,
> >          ret = -EINVAL;
> >          goto err_drain;
> >      }
> > -    ret = qemu_loadvm_state(f);
> > +    ret = qemu_loadvm_state(f, errp);
> >      migration_incoming_state_destroy();
> >  
> >      bdrv_drain_all_end();
> >  
> >      if (ret < 0) {
> > -        error_setg(errp, "Error %d while loading VM state", ret);
> > +        error_prepend(errp, "Error %d while loading VM state: ", ret);
> 
> This message will get redundant, leave it out.
Sure, will do.
> 
> >          return false;
> >      }
> >  
> > diff --git a/migration/savevm.h b/migration/savevm.h
> > index 2d5e9c716686f06720325e82fe90c75335ced1de..b80770b7461a60e2ad6ba5e24a7baeae73d90955 100644
> > --- a/migration/savevm.h
> > +++ b/migration/savevm.h
> > @@ -64,7 +64,7 @@ void qemu_savevm_send_colo_enable(QEMUFile *f);
> >  void qemu_savevm_live_state(QEMUFile *f);
> >  int qemu_save_device_state(QEMUFile *f);
> >  
> > -int qemu_loadvm_state(QEMUFile *f);
> > +int qemu_loadvm_state(QEMUFile *f, Error **errp);
> >  void qemu_loadvm_state_cleanup(MigrationIncomingState *mis);
> >  int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis);
> >  int qemu_load_device_state(QEMUFile *f);
>
Regards,
Arun 

Re: [PATCH v11 23/27] migration: Capture error in postcopy_ram_listen_thread()

[Arun Menon]

Hi Fabiano,
Thanks for the review.

On Fri, Aug 15, 2025 at 04:57:38PM -0300, Fabiano Rosas wrote:
> Arun Menon <armenon@redhat.com> writes:
> 
> > This is an incremental step in converting vmstate loading
> > code to report error via Error objects instead of directly
> > printing it to console/monitor.
> > postcopy_ram_listen_thread() calls qemu_loadvm_state_main()
> > to load the vm, and in case of a failure, it should set the error
> > in the migration object.
> >
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Signed-off-by: Arun Menon <armenon@redhat.com>
> > ---
> >  migration/savevm.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index de2bce276faa863a0f25deedafb0b784f10559d7..b85620a03654c214f4e771fa3b2bcfdf48661214 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2095,6 +2095,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> >      QEMUFile *f = mis->from_src_file;
> >      int load_res;
> >      MigrationState *migr = migrate_get_current();
> > +    Error *local_err = NULL;
> >  
> >      object_ref(OBJECT(migr));
> >  
> > @@ -2111,7 +2112,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> >      qemu_file_set_blocking(f, true);
> >  
> >      /* TODO: sanity check that only postcopiable data will be loaded here */
> > -    load_res = qemu_loadvm_state_main(f, mis, &error_fatal);
> > +    load_res = qemu_loadvm_state_main(f, mis, &local_err);
> >  
> >      /*
> >       * This is tricky, but, mis->from_src_file can change after it
> > @@ -2137,9 +2138,12 @@ static void *postcopy_ram_listen_thread(void *opaque)
> >                           __func__, load_res);
> >              load_res = 0; /* prevent further exit() */
> >          } else {
> > -            error_report("%s: loadvm failed: %d", __func__, load_res);
> > +            error_prepend(&local_err,
> > +                          "loadvm failed during postcopy: %d: ", load_res);
> > +            migrate_set_error(migr, local_err);
> > +            error_report_err(local_err);
> >              migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> > -                                           MIGRATION_STATUS_FAILED);
> > +                              MIGRATION_STATUS_FAILED);
> 
> This should be left alone. Having this patch (git) conflict with
> something else just because of this line would be really annoying.

Sure, will do.

> 
> >          }
> >      }
> >      if (load_res >= 0) {
> 

Regards,
Arun Menon

Re: [PATCH v11 01/27] migration: push Error **errp into vmstate_subsection_load()

[Arun Menon]

Hi Fabiano,
Thanks for the review.

On Fri, Aug 15, 2025 at 05:06:11PM -0300, Fabiano Rosas wrote:
> Fabiano Rosas <farosas@suse.de> writes:
> 
> > Arun Menon <armenon@redhat.com> writes:
> >
> >> 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_subsection_load() must report an error
> >> in errp, in case of failure.
> >>
> >> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> Signed-off-by: Arun Menon <armenon@redhat.com>
> >> ---
> >>  migration/vmstate.c | 11 ++++++++---
> >>  1 file changed, 8 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/migration/vmstate.c b/migration/vmstate.c
> >> index 5feaa3244d259874f03048326b2497e7db32e47c..6108c7fe283a5013ce42ea9987723c489aef26a2 100644
> >> --- a/migration/vmstate.c
> >> +++ b/migration/vmstate.c
> >> @@ -25,7 +25,7 @@ static int vmstate_subsection_save(QEMUFile *f, const VMStateDescription *vmsd,
> >>                                     void *opaque, JSONWriter *vmdesc,
> >>                                     Error **errp);
> >>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
> >> -                                   void *opaque);
> >> +                                   void *opaque, Error **errp);
> >>  
> >>  /* Whether this field should exist for either save or load the VM? */
> >>  static bool
> >> @@ -225,7 +225,7 @@ int vmstate_load_state(QEMUFile *f, const VMStateDescription *vmsd,
> >>          field++;
> >>      }
> >>      assert(field->flags == VMS_END);
> >> -    ret = vmstate_subsection_load(f, vmsd, opaque);
> >> +    ret = vmstate_subsection_load(f, vmsd, opaque, &error_fatal);
> >>      if (ret != 0) {
> >>          qemu_file_set_error(f, ret);
> >>          return ret;
> >
> > This is now unreachable, no?
It is.
> >
> 
> Also, this temporary &error_fatal here and throughout the series will
> break bisect badly, won't it? Imagine we have a bug in the code past
> this point (once the future patch from this series removes the
> &error_fatal), now every time this commit shows up in bisect, it will
> abort earlier.
> 
> I get that having error_fatal here helps ensure the series is correct,
> but maybe we should do without it.
That is a valid point.
I think, we can pass local_err and then report it using
warn_report_err() whenever there are operations past the function call.
If we are calling the function in the return statement, then we can pass
error_fatal. If that is ok, I shall amend the series to pass local_err and
temporarily report it.
> 
> Do others have an opinion here?
> 
> >> @@ -566,7 +566,7 @@ vmstate_get_subsection(const VMStateDescription * const *sub,
> >>  }
> >>  
> >>  static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
> >> -                                   void *opaque)
> >> +                                   void *opaque, Error **errp)
> >>  {
> >>      trace_vmstate_subsection_load(vmsd->name);
> >>  
> >> @@ -598,6 +598,8 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
> >>          sub_vmsd = vmstate_get_subsection(vmsd->subsections, idstr);
> >>          if (sub_vmsd == NULL) {
> >>              trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(lookup)");
> >> +            error_setg(errp, "VM subsection '%s' in '%s' does not exist",
> >> +                       idstr, vmsd->name);
> >>              return -ENOENT;
> >>          }
> >>          qemu_file_skip(f, 1); /* subsection */
> >> @@ -608,6 +610,9 @@ static int vmstate_subsection_load(QEMUFile *f, const VMStateDescription *vmsd,
> >>          ret = vmstate_load_state(f, sub_vmsd, opaque, version_id);
> >>          if (ret) {
> >>              trace_vmstate_subsection_load_bad(vmsd->name, idstr, "(child)");
> >> +            error_setg(errp,
> >> +                       "Loading VM subsection '%s' in '%s' failed: %d",
> >> +                       idstr, vmsd->name, ret);
> >>              return ret;
> >>          }
> >>      }
> 
Regards,
Arun