Re: [PATCH v11 07/27] migration: push Error **errp into qemu_loadvm_state()

[Fabiano Rosas <farosas@suse.de>]

Arun Menon <armenon@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() must report an error
> in errp, in case of failure.
>
> When postcopy live migration runs, the device states are loaded by
> both the qemu coroutine process_incoming_migration_co() and the
> postcopy_ram_listen_thread(). Therefore, it is important that the
> coroutine also reports the error in case of failure, with
> error_report_err(). Otherwise, the source qemu will not display
> any errors before going into the postcopy pause state.
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Arun Menon <armenon@redhat.com>
> ---
>  migration/migration.c |  9 +++++----
>  migration/savevm.c    | 31 +++++++++++++++++++------------
>  migration/savevm.h    |  2 +-
>  3 files changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 10c216d25dec01f206eacad2edd24d21f00e614c..c6768d88f45c870c7fad9b9957300766ff69effc 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -881,7 +881,7 @@ process_incoming_migration_co(void *opaque)
>                        MIGRATION_STATUS_ACTIVE);
>  
>      mis->loadvm_co = qemu_coroutine_self();
> -    ret = qemu_loadvm_state(mis->from_src_file);
> +    ret = qemu_loadvm_state(mis->from_src_file, &local_err);
>      mis->loadvm_co = NULL;
>  
>      trace_vmstate_downtime_checkpoint("dst-precopy-loadvm-completed");
> @@ -908,7 +908,8 @@ process_incoming_migration_co(void *opaque)
>      }
>  
>      if (ret < 0) {
> -        error_setg(&local_err, "load of migration failed: %s", strerror(-ret));
> +        error_prepend(&local_err, "load of migration failed: %s: ",
> +                      strerror(-ret));
>          goto fail;
>      }
>  
> @@ -924,13 +925,13 @@ fail:
>      migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
>                        MIGRATION_STATUS_FAILED);
>      migrate_set_error(s, local_err);
> -    error_free(local_err);
> +    error_report_err(local_err);
>  
>      migration_incoming_state_destroy();
>  
>      if (mis->exit_on_error) {
>          WITH_QEMU_LOCK_GUARD(&s->error_mutex) {
> -            error_report_err(s->error);
> +            error_free(s->error);
>              s->error = NULL;
>          }
>  
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 4f8c40e284a1b199d12f3c7dd61212b3e0e057c9..05dc392bdf4e19f340bc72bf66ba0543d56868a5 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -3159,28 +3159,24 @@ out:
>      return ret;
>  }
>  
> -int qemu_loadvm_state(QEMUFile *f)
> +int qemu_loadvm_state(QEMUFile *f, Error **errp)
>  {
>      MigrationState *s = migrate_get_current();
>      MigrationIncomingState *mis = migration_incoming_get_current();
> -    Error *local_err = NULL;
>      int ret;
>  
> -    if (qemu_savevm_state_blocked(&local_err)) {
> -        error_report_err(local_err);
> +    if (qemu_savevm_state_blocked(errp)) {
>          return -EINVAL;
>      }
>  
>      qemu_loadvm_thread_pool_create(mis);
>  
> -    ret = qemu_loadvm_state_header(f, &local_err);
> +    ret = qemu_loadvm_state_header(f, errp);
>      if (ret) {
> -        error_report_err(local_err);
>          return ret;
>      }
>  
> -    if (qemu_loadvm_state_setup(f, &local_err) != 0) {
> -        error_report_err(local_err);
> +    if (qemu_loadvm_state_setup(f, errp) != 0) {
>          return -EINVAL;
>      }
>  
> @@ -3191,6 +3187,9 @@ int qemu_loadvm_state(QEMUFile *f)
>      cpu_synchronize_all_pre_loadvm();
>  
>      ret = qemu_loadvm_state_main(f, mis);
> +    if (ret < 0) {
> +        error_setg(errp, "Load VM state failed: %d", ret);
> +    }
>      qemu_event_set(&mis->main_thread_load_event);
>  
>      trace_qemu_loadvm_state_post_main(ret);
> @@ -3208,8 +3207,14 @@ int qemu_loadvm_state(QEMUFile *f)
>          if (migrate_has_error(migrate_get_current()) ||
>              !qemu_loadvm_thread_pool_wait(s, mis)) {
>              ret = -EINVAL;
> +            error_setg(errp,
> +                       "Error while loading VM state: "
> +                       "Migration stream has error");

The stream error is the one below. Just keep a generic message here
because we'll propagate the error from qemu_loadvm_state_main() later in
the series.

>          } else {
>              ret = qemu_file_get_error(f);
> +            if (ret < 0) {
> +                error_setg(errp, "Error while loading vmstate : %d", ret);

+ stream error:

> +            }
>          }
>      }
>      /*
> @@ -3474,6 +3479,7 @@ void qmp_xen_save_devices_state(const char *filename, bool has_live, bool live,
>  
>  void qmp_xen_load_devices_state(const char *filename, Error **errp)
>  {
> +    ERRP_GUARD();
>      QEMUFile *f;
>      QIOChannelFile *ioc;
>      int ret;
> @@ -3495,10 +3501,10 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
>      f = qemu_file_new_input(QIO_CHANNEL(ioc));
>      object_unref(OBJECT(ioc));
>  
> -    ret = qemu_loadvm_state(f);
> +    ret = qemu_loadvm_state(f, errp);
>      qemu_fclose(f);
>      if (ret < 0) {
> -        error_setg(errp, "loading Xen device state failed");
> +        error_prepend(errp, "loading Xen device state failed: ");
>      }
>      migration_incoming_state_destroy();
>  }
> @@ -3506,6 +3512,7 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
>  bool load_snapshot(const char *name, const char *vmstate,
>                     bool has_devices, strList *devices, Error **errp)
>  {
> +    ERRP_GUARD();
>      BlockDriverState *bs_vm_state;
>      QEMUSnapshotInfo sn;
>      QEMUFile *f;
> @@ -3569,13 +3576,13 @@ bool load_snapshot(const char *name, const char *vmstate,
>          ret = -EINVAL;
>          goto err_drain;
>      }
> -    ret = qemu_loadvm_state(f);
> +    ret = qemu_loadvm_state(f, errp);
>      migration_incoming_state_destroy();
>  
>      bdrv_drain_all_end();
>  
>      if (ret < 0) {
> -        error_setg(errp, "Error %d while loading VM state", ret);
> +        error_prepend(errp, "Error %d while loading VM state: ", ret);

This message will get redundant, leave it out.

>          return false;
>      }
>  
> diff --git a/migration/savevm.h b/migration/savevm.h
> index 2d5e9c716686f06720325e82fe90c75335ced1de..b80770b7461a60e2ad6ba5e24a7baeae73d90955 100644
> --- a/migration/savevm.h
> +++ b/migration/savevm.h
> @@ -64,7 +64,7 @@ void qemu_savevm_send_colo_enable(QEMUFile *f);
>  void qemu_savevm_live_state(QEMUFile *f);
>  int qemu_save_device_state(QEMUFile *f);
>  
> -int qemu_loadvm_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_load_device_state(QEMUFile *f);