Hi Akihiko, Thanks for the review.
On Wed, Aug 06, 2025 at 02:17:02PM +0900, Akihiko Odaki wrote: > On 2025/08/06 3:25, 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. > > > > Signed-off-by: Arun Menon <arme...@redhat.com> > > --- > > migration/migration.c | 7 ++++--- > > migration/savevm.c | 31 +++++++++++++++++++------------ > > migration/savevm.h | 2 +- > > 3 files changed, 24 insertions(+), 16 deletions(-) > > > > diff --git a/migration/migration.c b/migration/migration.c > > index > > 10c216d25dec01f206eacad2edd24d21f00e614c..bb7d5e1dee52692cbea1d2c8fdca541e6a75bedb > > 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,7 +925,7 @@ 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); > > There is "error_report_err(s->error)" after this so this can result in > duplicate error printing. Yes, we can replace this with error_free(s->error). > > > migration_incoming_state_destroy(); > > diff --git a/migration/savevm.c b/migration/savevm.c > > index > > 1bd27efe437d4d911728d776e995490d0a45dcf5..ca166ebd397ad80836ed2f9cb20a92f704fd4ed5 > > 100644 > > --- a/migration/savevm.c > > +++ b/migration/savevm.c > > @@ -3149,28 +3149,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; > > } > > @@ -3181,6 +3177,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); > > @@ -3198,8 +3197,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"); > > } else { > > ret = qemu_file_get_error(f); > > + if (ret < 0) { > > + error_setg(errp, "Error while loading vmstate : %d", ret); > > + } > > } > > } > > /* > > @@ -3464,6 +3469,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; > > @@ -3485,10 +3491,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(); > > } > > @@ -3496,6 +3502,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; > > @@ -3559,13 +3566,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); > > 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