Hi Marc-André,
Thanks for the review.
On Wed, Aug 06, 2025 at 11:34:23AM +0400, Marc-André Lureau wrote:
> Hi
>
> On Tue, Aug 5, 2025 at 10:29 PM Arun Menon <arme...@redhat.com> 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 | 33 ++++++++++++++++++++-------------
> > migration/savevm.h | 3 ++-
> > 3 files changed, 25 insertions(+), 16 deletions(-)
> >
> > diff --git a/migration/colo.c b/migration/colo.c
> > index
> > 0ba22ee76a13e313793f653f43a728e3c433bbc1..a96e4dba15516b71d1b315c736c3b4879ff04e58
> > 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
> > f3b91c8ae0eee6078406081f0bd7f686fed28601..ad3dd9b172afc541f104d2187a79bafa8e380466
> > 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -2105,7 +2105,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);
> >
>
> What's the rationale to make the function silent or not gather the error?
> It seems error reporting could be improved below this. Maybe pass
> &error_warn here, and add a second TODO to improve error handling?
There is a separate patch which handles postcopy_ram_listen_thread
- PATCH 22/27
Therefore I left this as is. But yes, we can pass &error_warn here.
>
>
> >
> > /*
> > * This is tricky, but, mis->from_src_file can change after it
> > @@ -2407,6 +2407,7 @@ static int
> > 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;
> > @@ -2456,9 +2457,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);
> > @@ -3074,8 +3075,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;
> >
> > @@ -3083,8 +3086,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);
> > break;
> > }
> >
> > @@ -3105,7 +3110,7 @@ retry:
> > }
> > 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;
> > @@ -3115,7 +3120,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;
> > }
> > @@ -3123,6 +3128,9 @@ retry:
> >
> > out:
> > if (ret < 0) {
> > + if (*errp == NULL) {
> > + error_setg(errp, "Loading VM state failed: %d", ret);
> > + }
> > qemu_file_set_error(f, ret);
> >
> > /* Cancel bitmaps incoming regardless of recovery */
> > @@ -3143,6 +3151,8 @@ out:
> > migrate_postcopy_ram() && postcopy_pause_incoming(mis)) {
> > /* Reset f to point to the newly created channel */
> > f = mis->from_src_file;
> > + error_free(*errp);
> > + *errp = NULL;
> > goto retry;
> > }
> > }
> > @@ -3176,10 +3186,7 @@ int qemu_loadvm_state(QEMUFile *f, Error **errp)
> >
> > cpu_synchronize_all_pre_loadvm();
> >
> > - ret = qemu_loadvm_state_main(f, mis);
> > - if (ret < 0) {
> > - error_setg(errp, "Load VM state failed: %d", ret);
> > - }
> > + ret = qemu_loadvm_state_main(f, mis, errp);
> > qemu_event_set(&mis->main_thread_load_event);
> >
> > trace_qemu_loadvm_state_post_main(ret);
> > @@ -3260,9 +3267,9 @@ int qemu_load_device_state(QEMUFile *f, Error **errp)
> > int ret;
> >
> > /* Load QEMU_VM_SECTION_FULL section */
> > - ret = qemu_loadvm_state_main(f, mis);
> > + ret = qemu_loadvm_state_main(f, mis, errp);
> > if (ret < 0) {
> > - error_setg(errp, "Failed to load device state: %d", ret);
> > + error_prepend(errp, "Failed to load device state: %d: ", ret);
> > return ret;
> > }
> >
> > diff --git a/migration/savevm.h b/migration/savevm.h
> > index
> > b12681839f0b1afa3255e45215d99c13a224b19f..c337e3e3d111a7f28a57b90f61e8f70b71803d4e
> > 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, Error **errp);
> > 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, Error **errp);
> > int qemu_loadvm_approve_switchover(void);
> > int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
> >
> > --
> > 2.50.1
> >
> >
Regards,
Arun
