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?
>
> /*
> * 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
>
>
