Re: [PATCH v3 1/3] migration: Pass Error object errp into vm state loading functions

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

On Wed, Jul 02, 2025 at 05:06:50PM +0530, Arun Menon wrote:
> - This is an incremental step in converting vmstate loading
>   code to report errors.
> - Minimal changes to the signature and body of the following
>   functions are done,
>   - vmstate_load()
>   - vmstate_load_state()
>   - vmstate_subsection_load()
>   - qemu_load_device_state()
>   - qemu_loadvm_state()
>   - qemu_loadvm_section_start_full()
>   - qemu_loadvm_section_part_end()
>   - qemu_loadvm_state_header()
>   - qemu_loadvm_state_main()
> - error_report() has been replaced with error_setg();
>   and in places where error has been already set,
>   error_prepend() is used to not lose information.
> 
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  hw/display/virtio-gpu.c     |   2 +-
>  hw/pci/pci.c                |   2 +-
>  hw/s390x/virtio-ccw.c       |   2 +-
>  hw/scsi/spapr_vscsi.c       |   2 +-
>  hw/vfio/pci.c               |   2 +-
>  hw/virtio/virtio-mmio.c     |   2 +-
>  hw/virtio/virtio-pci.c      |   2 +-
>  hw/virtio/virtio.c          |   4 +-
>  include/migration/vmstate.h |   2 +-
>  migration/colo.c            |  13 +++--
>  migration/cpr.c             |   2 +-
>  migration/migration.c       |  19 ++++--
>  migration/savevm.c          | 137 +++++++++++++++++++++++++++-----------------
>  migration/savevm.h          |   7 ++-
>  migration/vmstate-types.c   |  10 ++--
>  migration/vmstate.c         |  40 +++++++------
>  tests/unit/test-vmstate.c   |  18 +++---
>  17 files changed, 158 insertions(+), 108 deletions(-)
> 

> diff --git a/migration/colo.c b/migration/colo.c
> index e0f713c837f5da25d67afbd02ceb6c54024ca3af..808c980b4af3199968771cdc6cca3c2451a2f4a6 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();
>      uint64_t total_size;
>      uint64_t value;
>      Error *local_err = NULL;
> @@ -686,11 +687,13 @@ 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, &local_err);
>      bql_unlock();
>  
>      if (ret < 0) {
> -        error_setg(errp, "Load VM's live state (ram) error");
> +        if (local_err != NULL) {
> +            error_prepend(errp, "Load VM's live state (ram) error");
> +        }

This doesn't look right to me.

The old code would unconditionally report an error, but this code
now only appends an error if 'qemu_loadvm_state_main' already
reported an error - if 'qemu_loadvm_state_main' was silent this
method reports nothing too.

IMHO this is a bad design for qemu_loadvm_state_main - when we
add an 'errp' to that method we *MUST* ensure it fills that
in *all* possible error code paths.

This code should then unconditionally use error_prepend().

>          return;
>      }
>  
> @@ -729,9 +732,11 @@ 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, &local_err);
>      if (ret < 0) {
> -        error_setg(errp, "COLO: load device state failed");
> +        if (*errp != NULL) {
> +            error_prepend(errp, "COLO: load device state failed");
> +        }

Same flawed design here - qemu_load_device_state must guarantee it always
fills its 'errp' parameter on failure code paths.

There are more instances of this general error reporting problem through
this patch

IMHO this patch is changing too many methods at once to be confident when
reviewing it.

This would be better changing 1 single method and its *immediate* callers
only.

IOW, if we have a sequence a() -> b() -> c() -> d() all of which
currently use 'error_report', don't try to change the whole call
chain at once.

First add an "errp' to 'd()', and make 'c()' use 'error_report_err'.
Then add an "errp' to 'c'()', and make 'b()' use 'error_report_err',
...repeat..

This is what I tried todo previously to address this problem in
migration code

  https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg01727.html

Though those patches are horribly outdated now, so can't be used as
is, IMHO they show the right kind of incremental conversion

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