Hi,
Thanks for the review
On Mon, Jul 21, 2025 at 10:01:24PM +0900, Akihiko Odaki wrote:
> On 2025/07/21 20:29, 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.
> >
> > Signed-off-by: Arun Menon <arme...@redhat.com>
> > ---
> > migration/migration.c | 5 +++--
> > migration/savevm.c | 25 +++++++++++++------------
> > migration/savevm.h | 2 +-
> > 3 files changed, 17 insertions(+), 15 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index
> > d748a02712dc4ebc2de6b0488fb199c92c4d4079..09fadf36dbbbd2f68df1e4cafbf3a51b18531978
> > 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;
> > }
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index
> > ba146f91427f2a36880aadeb16b11ab2b7df099a..1261e81c86f836e6b5a155ba1880b5823a779567
> > 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -3137,27 +3137,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, NULL);
> > + ret = qemu_loadvm_state_header(f, errp);
> > if (ret) {
> > 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;
> > }
> > @@ -3167,7 +3164,7 @@ int qemu_loadvm_state(QEMUFile *f)
> > cpu_synchronize_all_pre_loadvm();
> > - ret = qemu_loadvm_state_main(f, mis, NULL);
> > + ret = qemu_loadvm_state_main(f, mis, errp);
> > qemu_event_set(&mis->main_thread_load_event);
> > trace_qemu_loadvm_state_post_main(ret);
> > @@ -3185,8 +3182,12 @@ 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 : %d", ret);
>
> Printing ret here does not help much as ret is always -EINVAL here.
>
> Having an error message different from qemu_file_get_error() will help more
> as it will allow reliably distinguishing these two error conditions.
Yes, maybe I can add "Migration stream has error" to the error message.
>
> > } else {
> > ret = qemu_file_get_error(f);
> > + if (ret < 0) {
> > + error_setg(errp, "Error while loading vmstate : %d", ret);
> > + }
> > }
> > }
> > /*
> > @@ -3472,10 +3473,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();
> > }
> > @@ -3546,13 +3547,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
> > fd7419e6ff90062970ed246b3ea71e6d49a6e372..a6df5198f3fe1a39fc0e6ce3e79cf7a5d8e032db
> > 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,
> > Error **errp);
> >
>
Regards,
Arun
