Arun Menon <arme...@redhat.com> writes:
> 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 | 36 +++++++++++++++++++-----------------
> migration/savevm.h | 3 ++-
> 3 files changed, 24 insertions(+), 20 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();
With my suggestion below, this goes away.
> 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: ");
Another one to leave out. There's enough information downstream
already. Also, this "(ram)" doesn't look right.
> return;
> }
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index
> 70e021597d884030c4a0dc2a7bc27d42a7371797..9ec07892cd6ea666431410657c840b6325377d97
> 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, &error_fatal);
>
> /*
> * 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);
This is getting out of hand for code review, may I suggest you
artificially trigger these errors, look at the resulting message and
remove all the unnecessary wrapping? Each error_prepend is a candidate
for removal if it will just state "load failed".
Using error_prepend partly defeats the purpose of propagating errp. We
should only use it when there's valuable information to be provided.
> }
> trace_loadvm_handle_cmd_packaged_main(ret);
> qemu_fclose(packf);
> @@ -3080,18 +3081,21 @@ 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;
> - Error *local_err = NULL;
>
> 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);
We could drop some extra words here, the term 'section' is already quite
representative.
"Failed to load section ID: stream error: %d: "
> break;
> }
>
> @@ -3112,10 +3116,7 @@ retry:
> }
> break;
> case QEMU_VM_COMMAND:
> - ret = loadvm_process_command(f, &local_err);
> - if (ret < 0) {
> - warn_report_err(local_err);
> - }
> + ret = loadvm_process_command(f, errp);
Good.
> trace_qemu_loadvm_state_section_command(ret);
> if ((ret < 0) || (ret == LOADVM_QUIT)) {
> goto out;
> @@ -3125,7 +3126,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);
Not sure if they're referring to "savevm" here as a generic term for
vmstate/migration or if it was intended to say: "savevm wrote a section
type that this loadvm instance doesn't understand".
Since you're here, could you fix this? Migration errors from source and
destination are often interleaved in logs, we don't want to see the
"savevm" word in a destination-side error message. Just put a small note
in the commit message, no need for another patch.
> ret = -EINVAL;
> goto out;
> }
> @@ -3133,6 +3134,9 @@ retry:
>
> out:
> if (ret < 0) {
> + if (*errp == NULL) {
> + error_setg(errp, "Loading VM state failed: %d", ret);
> + }
Another candidate for removal, then we avoid having to dereference errp.
> qemu_file_set_error(f, ret);
>
> /* Cancel bitmaps incoming regardless of recovery */
> @@ -3153,6 +3157,7 @@ out:
> migrate_postcopy_ram() && postcopy_pause_incoming(mis)) {
> /* Reset f to point to the newly created channel */
> f = mis->from_src_file;
> + error_free_or_abort(errp);
What's this about?
> goto retry;
> }
> }
> @@ -3186,10 +3191,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);
> @@ -3270,9 +3272,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,
