Hi Akihiko,
Thanks for the review.
On Sat, Aug 30, 2025 at 02:58:05PM +0900, Akihiko Odaki wrote:
> On 2025/08/30 5:01, Arun Menon wrote:
> > This is an incremental step in converting vmstate loading
> > code to report error via Error objects instead of directly
> > printing it to console/monitor.
> > It is ensured that 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 <[email protected]>
> > Reviewed-by: Fabiano Rosas <[email protected]>
> > Signed-off-by: Arun Menon <[email protected]>
> > ---
> > migration/migration.c | 9 +++++----
> > migration/savevm.c | 30 ++++++++++++++++++------------
> > migration/savevm.h | 2 +-
> > 3 files changed, 24 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);
>
> This is problematic because it results in duplicate error reports when
> !mis->exit_on_error; in that case the query-migrate QMP command reports the
> error and this error reporting is redundant.
If I comment this change, then all of the errors propagated up to now, using
error_setg() will not be reported. This is the place where it is finally
reported,
when qemu_loadvm_state() fails. In other words, all the error_reports() we
removed
from all the files, replacing them with error_setg(), will finally be reported
here
using error_report_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);
>
> This change is problematic because s->error set somewhere else here will be
> ignored.
This is specific to the case when mis->exit_on_error is set.
since we did a migrate_set_error(s, local_err) before, we free the
error in s->error and set it to NULL, before an exit(EXIT_FAILURE)
>
> I think the two changes I commented can be simply removed without causing
> other problems.
Please correct me if I am wrong.
>
> > s->error = NULL;
> > }
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index
> > de5671ffd1cd06e728227a3056c3f895d3a6e6f3..0087fca15ce108685667d3808350d80d37b807b1
> > 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,15 @@ 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 vmstate");
> > } else {
> > ret = qemu_file_get_error(f);
> > + if (ret < 0) {
> > + error_setg(errp,
> > + "Error while loading vmstate: stream error: %d",
> > + ret);
> > + }
> > }
> > }
> > /*
> > @@ -3474,6 +3480,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 +3502,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();
> > }
> > @@ -3569,13 +3576,12 @@ 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);
> > 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 Menon
