Hi Akihiko,
Thanks for the review.
On Fri, Aug 01, 2025 at 04:17:04PM +0900, Akihiko Odaki wrote:
> On 2025/07/31 22:20, 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_main() must report an error
> > in errp, in case of failure.
> > loadvm_process_command also sets the errp object explicitly.
> >
> > Reviewed-by: Daniel P. Berrangé <berra...@redhat.com>
> > Signed-off-by: Arun Menon <arme...@redhat.com>
> > ---
> > migration/colo.c | 5 +++--
> > migration/savevm.c | 27 ++++++++++++++++-----------
> > migration/savevm.h | 3 ++-
> > 3 files changed, 21 insertions(+), 14 deletions(-)
> >
> > diff --git a/migration/colo.c b/migration/colo.c
> > index
> > 981bd4bf9ced8b45b4c5d494acae119a174ee974..529794dfc8d943b8ba3a25391ee2132c0c3f312e
> > 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,11 @@ 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, errp);
> > bql_unlock();
> > if (ret < 0) {
> > - error_setg(errp, "Load VM's live state (ram) error");
> > + error_prepend(errp, "Load VM's live state (ram) error: ");
> > return;
> > }
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index
> > e885f1724f223771d60081fea199320abc549d2f..f5903995edd2b4c4f6c1a214c7126d831f10c9f1
> > 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2111,7 +2111,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);
> > + load_res = qemu_loadvm_state_main(f, mis, NULL);
> > /*
> > * This is tricky, but, mis->from_src_file can change after it
> > @@ -2412,6 +2412,7 @@ static void
> > loadvm_postcopy_handle_resume(MigrationIncomingState *mis)
> > */
> > static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis, Error
> > **errp)
> > {
> > + ERRP_GUARD();
> > int ret;
> > size_t length;
> > QIOChannelBuffer *bioc;
> > @@ -2461,9 +2462,9 @@ static int
> > loadvm_handle_cmd_packaged(MigrationIncomingState *mis, Error **errp)
> > qemu_coroutine_yield();
> > } while (1);
> > - ret = qemu_loadvm_state_main(packf, mis);
> > + ret = qemu_loadvm_state_main(packf, mis, errp);
> > if (ret < 0) {
> > - error_setg(errp, "VM state load failed: %d", ret);
> > + error_prepend(errp, "Loading VM state failed: %d: ", ret);
> > }
> > trace_loadvm_handle_cmd_packaged_main(ret);
> > qemu_fclose(packf);
> > @@ -3066,8 +3067,10 @@ static bool
> > postcopy_pause_incoming(MigrationIncomingState *mis)
> > return true;
> > }
> > -int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> > +int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis,
> > + Error **errp)
> > {
> > + ERRP_GUARD();
> > uint8_t section_type;
> > int ret = 0;
> > @@ -3075,8 +3078,10 @@ retry:
> > while (true) {
> > section_type = qemu_get_byte(f);
> > - ret = qemu_file_get_error_obj_any(f, mis->postcopy_qemufile_dst,
> > NULL);
> > + ret = qemu_file_get_error_obj_any(f, mis->postcopy_qemufile_dst,
> > errp);
> > if (ret) {
> > + error_prepend(errp, "Failed to load device state section ID:
> > %d: ",
> > + ret);
>
> I noticed this is after the "retry" label; errp already contains an error
> when retried so the error needs to be properly reported and freed.
Yes, I shall check how to report error, because there is an error_report() in
the
default case. And yes, errp needs to be freed and set to NULL before retry.
>
> > break;
> > }
> > @@ -3084,20 +3089,20 @@ retry:
> > switch (section_type) {
> > case QEMU_VM_SECTION_START:
> > case QEMU_VM_SECTION_FULL:
> > - ret = qemu_loadvm_section_start_full(f, section_type, NULL);
> > + ret = qemu_loadvm_section_start_full(f, section_type, errp);
> > if (ret < 0) {
> > goto out;
> > }
> > break;
> > case QEMU_VM_SECTION_PART:
> > case QEMU_VM_SECTION_END:
> > - ret = qemu_loadvm_section_part_end(f, section_type, NULL);
> > + ret = qemu_loadvm_section_part_end(f, section_type, errp);
> > if (ret < 0) {
> > goto out;
> > }
> > break;
> > case QEMU_VM_COMMAND:
> > - ret = loadvm_process_command(f, NULL);
> > + ret = loadvm_process_command(f, errp);
> > trace_qemu_loadvm_state_section_command(ret);
> > if ((ret < 0) || (ret == LOADVM_QUIT)) {
> > goto out;
> > @@ -3107,7 +3112,7 @@ retry:
> > /* This is the end of migration */
> > goto out;
> > default:
> > - error_report("Unknown savevm section type %d", section_type);
> > + error_setg(errp, "Unknown savevm section type %d",
> > section_type);
> > ret = -EINVAL;
> > goto out;
> > }
> > @@ -3171,7 +3176,7 @@ int qemu_loadvm_state(QEMUFile *f)
> > cpu_synchronize_all_pre_loadvm();
> > - ret = qemu_loadvm_state_main(f, mis);
> > + ret = qemu_loadvm_state_main(f, mis, NULL);
> > qemu_event_set(&mis->main_thread_load_event);
> > trace_qemu_loadvm_state_post_main(ret);
> > @@ -3245,7 +3250,7 @@ int qemu_load_device_state(QEMUFile *f)
> > int ret;
> > /* Load QEMU_VM_SECTION_FULL section */
> > - ret = qemu_loadvm_state_main(f, mis);
> > + ret = qemu_loadvm_state_main(f, mis, NULL);
> > if (ret < 0) {
> > error_report("Failed to load device state: %d", ret);
> > return ret;
> > diff --git a/migration/savevm.h b/migration/savevm.h
> > index
> > 2d5e9c716686f06720325e82fe90c75335ced1de..fd7419e6ff90062970ed246b3ea71e6d49a6e372
> > 100644
> > --- a/migration/savevm.h
> > +++ b/migration/savevm.h
> > @@ -66,7 +66,8 @@ int qemu_save_device_state(QEMUFile *f);
> > int qemu_loadvm_state(QEMUFile *f);
> > void qemu_loadvm_state_cleanup(MigrationIncomingState *mis);
> > -int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis);
> > +int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis,
> > + Error **errp);
> > int qemu_load_device_state(QEMUFile *f);
> > int qemu_loadvm_approve_switchover(void);
> > int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
> >
>
Regards,
Arun
