* Daniel P. Berrangé (berra...@redhat.com) wrote:
> This is an incremental step in converting vmstate loading code to report
> via Error objects instead of printing directly to the console/monitor.
>
> Signed-off-by: Daniel P. Berrangé <berra...@redhat.com>
> ---
> migration/colo.c | 3 +-
> migration/savevm.c | 73 +++++++++++++++++++++++++++++++---------------
> migration/savevm.h | 3 +-
> 3 files changed, 52 insertions(+), 27 deletions(-)
>
> diff --git a/migration/colo.c b/migration/colo.c
> index e344b7cf32..4a050ac579 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -705,11 +705,10 @@ static void
> colo_incoming_process_checkpoint(MigrationIncomingState *mis,
>
> qemu_mutex_lock_iothread();
> 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);
> qemu_mutex_unlock_iothread();
>
> if (ret < 0) {
> - error_setg(errp, "Load VM's live state (ram) error");
> return;
> }
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index dd41292d4e..e47aec435c 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1819,6 +1819,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> QEMUFile *f = mis->from_src_file;
> int load_res;
> MigrationState *migr = migrate_get_current();
> + Error *local_err = NULL;
>
> object_ref(OBJECT(migr));
>
> @@ -1833,7 +1834,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> * in qemu_file, and thus we must be blocking now.
> */
> qemu_file_set_blocking(f, true);
> - load_res = qemu_loadvm_state_main(f, mis);
> + load_res = qemu_loadvm_state_main(f, mis, &local_err);
>
> /*
> * This is tricky, but, mis->from_src_file can change after it
> @@ -1849,6 +1850,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> if (load_res < 0) {
> qemu_file_set_error(f, load_res);
> dirty_bitmap_mig_cancel_incoming();
> + error_report_err(local_err);
> if (postcopy_state_get() == POSTCOPY_INCOMING_RUNNING &&
> !migrate_postcopy_ram() && migrate_dirty_bitmaps())
> {
> @@ -1859,12 +1861,10 @@ static void *postcopy_ram_listen_thread(void *opaque)
> __func__, load_res);
> load_res = 0; /* prevent further exit() */
> } else {
> - error_report("%s: loadvm failed: %d", __func__, load_res);
> migrate_set_state(&mis->state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> MIGRATION_STATUS_FAILED);
> }
> - }
> - if (load_res >= 0) {
> + } else {
> /*
> * This looks good, but it's possible that the device loading in the
> * main thread hasn't finished yet, and so we might not be in 'RUN'
> @@ -2116,14 +2116,17 @@ static int
> loadvm_postcopy_handle_resume(MigrationIncomingState *mis)
> * @mis: Incoming state
> * @length: Length of packaged data to read
> *
> - * Returns: Negative values on error
> - *
> + * Returns:
> + * 0: success
> + * LOADVM_QUIT: success, but stop
> + * -1: error
> */
> static int loadvm_handle_cmd_packaged(MigrationIncomingState *mis)
> {
> int ret;
> size_t length;
> QIOChannelBuffer *bioc;
> + Error *local_err = NULL;
>
> length = qemu_get_be32(mis->from_src_file);
> trace_loadvm_handle_cmd_packaged(length);
> @@ -2149,8 +2152,11 @@ static int
> loadvm_handle_cmd_packaged(MigrationIncomingState *mis)
>
> QEMUFile *packf = qemu_fopen_channel_input(QIO_CHANNEL(bioc));
>
> - ret = qemu_loadvm_state_main(packf, mis);
> + ret = qemu_loadvm_state_main(packf, mis, &local_err);
> trace_loadvm_handle_cmd_packaged_main(ret);
> + if (ret < 0) {
> + error_report_err(local_err);
> + }
> qemu_fclose(packf);
> object_unref(OBJECT(bioc));
>
> @@ -2568,7 +2574,14 @@ static bool
> postcopy_pause_incoming(MigrationIncomingState *mis)
> return true;
> }
>
> -int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis)
> +/*
> + * Returns:
> + * 0: success
> + * LOADVM_QUIT: success, but stop
> + * -1: error
> + */
> +int qemu_loadvm_state_main(QEMUFile *f, MigrationIncomingState *mis,
> + Error **errp)
> {
> uint8_t section_type;
> int ret = 0;
> @@ -2579,7 +2592,9 @@ retry:
>
> if (qemu_file_get_error(f)) {
> ret = qemu_file_get_error(f);
> - break;
> + error_setg(errp,
> + "Failed to load device state section ID: %d", ret);
Can I ask why these don't use strerror(ret) ?
The test I'm running is, start a VM with an actual guest and a useful
amount of ram:
./x86_64-softmmu/qemu-system-x86_64 -M pc,accel=kvm -nographic -m 8G -drive
if=virtio,file=/home/vmimages/fedora-33-nest.qcow
./x86_64-softmmu/qemu-system-x86_64 -M pc,accel=kvm -nographic -m 8G -drive
if=virtio,file=/home/vmimages/fedora-33-nest.qcow -incoming tcp:0:4444
source:
migrate_set_speed 1m
migrate -d tcp:0:4444
<Now quickly>
migrate_cancel
In the old world I get:
qemu-system-x86_64: load of migration failed: Input/output error
In your world I get:
qemu-system-x86_64: Failed to load device state section ID: -5
(5 being EIO)
Dave
> + goto out;
> }
>
> trace_qemu_loadvm_state_section(section_type);
> @@ -2588,6 +2603,9 @@ retry:
> case QEMU_VM_SECTION_FULL:
> ret = qemu_loadvm_section_start_full(f, mis);
> if (ret < 0) {
> + error_setg(errp,
> + "Failed to load device state section start: %d",
> + ret);
> goto out;
> }
> break;
> @@ -2595,29 +2613,38 @@ retry:
> case QEMU_VM_SECTION_END:
> ret = qemu_loadvm_section_part_end(f, mis);
> if (ret < 0) {
> + error_setg(errp,
> + "Failed to load device state section end: %d",
> ret);
> goto out;
> }
> break;
> case QEMU_VM_COMMAND:
> ret = loadvm_process_command(f);
> trace_qemu_loadvm_state_section_command(ret);
> - if ((ret < 0) || (ret == LOADVM_QUIT)) {
> + if (ret < 0) {
> + error_setg(errp,
> + "Failed to load device state command: %d", ret);
> + goto out;
> + }
> + if (ret == LOADVM_QUIT) {
> goto out;
> }
> break;
> case QEMU_VM_EOF:
> /* This is the end of migration */
> + ret = 0;
> goto out;
> default:
> - error_report("Unknown savevm section type %d", section_type);
> - ret = -EINVAL;
> + error_setg(errp,
> + "Unknown savevm section type %d", section_type);
> + ret = -1;
> goto out;
> }
> }
>
> out:
> if (ret < 0) {
> - qemu_file_set_error(f, ret);
> + qemu_file_set_error(f, -EINVAL);
>
> /* Cancel bitmaps incoming regardless of recovery */
> dirty_bitmap_mig_cancel_incoming();
> @@ -2643,6 +2670,12 @@ out:
> return ret;
> }
>
> +/*
> + * Returns:
> + * 0: success
> + * LOADVM_QUIT: success, but stop
> + * -1: error
> + */
> int qemu_loadvm_state(QEMUFile *f, Error **errp)
> {
> MigrationIncomingState *mis = migration_incoming_get_current();
> @@ -2662,17 +2695,12 @@ 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, "Error %d while loading VM state", ret);
> - ret = -1;
> - }
> + ret = qemu_loadvm_state_main(f, mis, errp);
> qemu_event_set(&mis->main_thread_load_event);
>
> trace_qemu_loadvm_state_post_main(ret);
>
> if (mis->have_listen_thread) {
> - error_setg(errp, "Error %d while loading VM state", ret);
> /* Listen thread still going, can't clean up yet */
> return ret;
> }
> @@ -2729,13 +2757,10 @@ int qemu_loadvm_state(QEMUFile *f, Error **errp)
> int qemu_load_device_state(QEMUFile *f, Error **errp)
> {
> MigrationIncomingState *mis = migration_incoming_get_current();
> - int ret;
>
> /* Load QEMU_VM_SECTION_FULL section */
> - ret = qemu_loadvm_state_main(f, mis);
> - if (ret < 0) {
> - error_setg(errp, "Failed to load device state: %d", ret);
> - return ret;
> + if (qemu_loadvm_state_main(f, mis, errp) < 0) {
> + return -1;
> }
>
> cpu_synchronize_all_post_init();
> diff --git a/migration/savevm.h b/migration/savevm.h
> index c727bc103e..1cec83c729 100644
> --- a/migration/savevm.h
> +++ b/migration/savevm.h
> @@ -62,7 +62,8 @@ int qemu_save_device_state(QEMUFile *f);
>
> int qemu_loadvm_state(QEMUFile *f, Error **errp);
> void qemu_loadvm_state_cleanup(void);
> -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);
>
> #endif
> --
> 2.29.2
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
