Re: [PATCH v5 21/23] migration: Capture error in postcopy_ram_listen_thread()

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

On Thu, Jul 17, 2025 at 06:07:44AM +0530, Arun Menon wrote:
> This is an incremental step in converting vmstate loading
> code to report error via Error objects instead of directly
> printing it to console/monitor.
> 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.
> 
> 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.
> 
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  migration/migration.c |  2 +-
>  migration/savevm.c    | 10 ++++++++--
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 


> diff --git a/migration/savevm.c b/migration/savevm.c
> index 0fff65c96344c65191353311e72730cd6e3bfb23..4f67eebe5321c175d51e8029e36ceb336c98ad1f 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2086,6 +2086,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));
>  
> @@ -2102,7 +2103,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, NULL);
> +    load_res = qemu_loadvm_state_main(f, mis, &local_err);
>  
>      /*
>       * This is tricky, but, mis->from_src_file can change after it
> @@ -2128,7 +2129,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);
> +            if (local_err != NULL) {
> +                error_prepend(&local_err, "%s: loadvm failed: %d", __func__,
> +                              load_res);

IMHO __func__ is redundant if you change the message
to be 'loadvm failed during postcopy' like the earlier part
fo the "if { " does.

> +                migrate_set_error(migr, local_err);
> +                error_report_err(local_err);
> +            }
>              migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
>                                             MIGRATION_STATUS_FAILED);

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 :|