Peter Xu <[email protected]> writes:
> migrate_set_error() currently doesn't take ownership of the error being
> passed in. It's not aligned with the error API and meanwhile it also
> makes most of the caller free the error explicitly.
>
> Change the API to take the ownership of the Error object instead. This
> should save a lot of error_copy() invocations.
>
> Signed-off-by: Peter Xu <[email protected]>
> ---
> migration/migration.h | 2 +-
> migration/cpr-exec.c | 5 ++--
> migration/migration.c | 44 +++++++++++++++-----------------
> migration/multifd-device-state.c | 5 +---
> migration/multifd.c | 19 +++++++-------
> migration/postcopy-ram.c | 5 ++--
> migration/ram.c | 4 +--
> migration/savevm.c | 16 +++++-------
> 8 files changed, 43 insertions(+), 57 deletions(-)
>
> diff --git a/migration/migration.h b/migration/migration.h
> index 213b33fe6e..e4b4f25deb 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -525,7 +525,7 @@ void migration_incoming_process(void);
>
> bool migration_has_all_channels(void);
>
> -void migrate_set_error(MigrationState *s, const Error *error);
> +void migrate_error_propagate(MigrationState *s, Error *error);
> bool migrate_has_error(MigrationState *s);
>
> void migration_connect(MigrationState *s, Error *error_in);
> diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
> index 0b8344a86f..da287d8031 100644
> --- a/migration/cpr-exec.c
> +++ b/migration/cpr-exec.c
> @@ -158,8 +158,9 @@ static void cpr_exec_cb(void *opaque)
>
> error_report_err(error_copy(err));
> migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
> - migrate_set_error(s, err);
> - error_free(err);
> +
> + migrate_error_propagate(s, err);
> + /* We must reset the error because it'll be reused later */
> err = NULL;
>
> /* Note, we can go from state COMPLETED to FAILED */
> diff --git a/migration/migration.c b/migration/migration.c
> index 0ff8b31a88..70813e5006 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -914,9 +914,7 @@ process_incoming_migration_co(void *opaque)
> fail:
> migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
> MIGRATION_STATUS_FAILED);
> - migrate_set_error(s, local_err);
> - error_free(local_err);
> -
> + migrate_error_propagate(s, local_err);
> migration_incoming_state_destroy();
>
> if (mis->exit_on_error) {
> @@ -1548,14 +1546,20 @@ static void migration_cleanup_bh(void *opaque)
> migration_cleanup(opaque);
> }
>
> -void migrate_set_error(MigrationState *s, const Error *error)
> +/*
> + * Propagate the Error* object to migration core. The caller mustn't
> + * reference the error pointer after the function returned, because the
> + * Error* object might be freed.
> + */
> +void migrate_error_propagate(MigrationState *s, Error *error)
> {
> QEMU_LOCK_GUARD(&s->error_mutex);
> -
> trace_migrate_error(error_get_pretty(error));
>
> if (!s->error) {
> - s->error = error_copy(error);
> + s->error = error;
> + } else {
> + error_free(error);
> }
This is correct. Also correct, and possibly easier to understand at a
glance:
error_propagate(&s->error, error);
error_propagate() has code for a number of additional conditions, but
these are all impossible:
* @error cannot be null here (because error_get_pretty(error) above)
* &s->error cannot be &error_abort, &error_fatal, or null
Thoughts?
> }
>
> @@ -1601,8 +1605,7 @@ static void
> migration_connect_error_propagate(MigrationState *s, Error *error)
> }
>
> migrate_set_state(&s->state, current, next);
> - migrate_set_error(s, error);
> - error_free(error);
> + migrate_error_propagate(s, error);
> }
>
> void migration_cancel(void)
> @@ -2014,8 +2017,7 @@ void qmp_migrate_pause(Error **errp)
>
> /* Tell the core migration that we're pausing */
> error_setg(&error, "Postcopy migration is paused by the user");
> - migrate_set_error(ms, error);
> - error_free(error);
> + migrate_error_propagate(ms, error);
>
> qemu_mutex_lock(&ms->qemu_file_lock);
> if (ms->to_dst_file) {
> @@ -2647,8 +2649,7 @@ static void *source_return_path_thread(void *opaque)
>
> out:
> if (err) {
> - migrate_set_error(ms, err);
> - error_free(err);
> + migrate_error_propagate(ms, err);
> trace_source_return_path_thread_bad_end();
> }
>
> @@ -3094,12 +3095,10 @@ static void migration_completion(MigrationState *s)
>
> fail:
> if (qemu_file_get_error_obj(s->to_dst_file, &local_err)) {
> - migrate_set_error(s, local_err);
> - error_free(local_err);
> + migrate_error_propagate(s, local_err);
> } else if (ret) {
> error_setg_errno(&local_err, -ret, "Error in migration completion");
> - migrate_set_error(s, local_err);
> - error_free(local_err);
> + migrate_error_propagate(s, local_err);
> }
>
> if (s->state != MIGRATION_STATUS_CANCELLING) {
> @@ -3326,8 +3325,7 @@ static MigThrError
> migration_detect_error(MigrationState *s)
> }
>
> if (local_error) {
> - migrate_set_error(s, local_error);
> - error_free(local_error);
> + migrate_error_propagate(s, local_error);
> }
>
> if (state == MIGRATION_STATUS_POSTCOPY_ACTIVE && ret) {
> @@ -3522,7 +3520,7 @@ static MigIterateState
> migration_iteration_run(MigrationState *s)
> if (must_precopy <= s->threshold_size &&
> can_switchover && qatomic_read(&s->start_postcopy)) {
> if (postcopy_start(s, &local_err)) {
> - migrate_set_error(s, local_err);
> + migrate_error_propagate(s, error_copy(local_err));
> error_report_err(local_err);
> }
> return MIG_ITERATE_SKIP;
> @@ -3819,8 +3817,7 @@ static void *migration_thread(void *opaque)
> * devices to unplug. This to preserve migration state transitions.
> */
> if (ret) {
> - migrate_set_error(s, local_err);
> - error_free(local_err);
> + migrate_error_propagate(s, local_err);
> migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
> MIGRATION_STATUS_FAILED);
> goto out;
> @@ -3944,8 +3941,7 @@ static void *bg_migration_thread(void *opaque)
> * devices to unplug. This to preserve migration state transitions.
> */
> if (ret) {
> - migrate_set_error(s, local_err);
> - error_free(local_err);
> + migrate_error_propagate(s, local_err);
> migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
> MIGRATION_STATUS_FAILED);
> goto fail_setup;
> @@ -4127,7 +4123,7 @@ void migration_connect(MigrationState *s, Error
> *error_in)
> return;
>
> fail:
> - migrate_set_error(s, local_err);
> + migrate_error_propagate(s, error_copy(local_err));
> if (s->state != MIGRATION_STATUS_CANCELLING) {
> migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
> }
> diff --git a/migration/multifd-device-state.c
> b/migration/multifd-device-state.c
> index db3239fef5..91d5d81556 100644
> --- a/migration/multifd-device-state.c
> +++ b/migration/multifd-device-state.c
> @@ -143,8 +143,6 @@ static int multifd_device_state_save_thread(void *opaque)
> Error *local_err = NULL;
>
> if (!data->hdlr(data, &local_err)) {
> - MigrationState *s = migrate_get_current();
> -
> /*
> * Can't call abort_device_state_save_threads() here since new
> * save threads could still be in process of being launched
> @@ -158,8 +156,7 @@ static int multifd_device_state_save_thread(void *opaque)
> * In case of multiple save threads failing which thread error
> * return we end setting is purely arbitrary.
> */
> - migrate_set_error(s, local_err);
> - error_free(local_err);
> + migrate_error_propagate(migrate_get_current(), local_err);
> }
>
> return 0;
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 52e4d25857..bf6da85af8 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -428,8 +428,9 @@ static void multifd_send_error_propagate(Error *err)
>
> if (err) {
> MigrationState *s = migrate_get_current();
> - migrate_set_error(s, err);
> - error_free(err);
> +
> + migrate_error_propagate(s, err);
> +
> if (s->state == MIGRATION_STATUS_SETUP ||
> s->state == MIGRATION_STATUS_PRE_SWITCHOVER ||
> s->state == MIGRATION_STATUS_DEVICE ||
> @@ -588,8 +589,7 @@ void multifd_send_shutdown(void)
> Error *local_err = NULL;
>
> if (!multifd_send_cleanup_channel(p, &local_err)) {
> - migrate_set_error(migrate_get_current(), local_err);
> - error_free(local_err);
> + migrate_error_propagate(migrate_get_current(), local_err);
> }
> }
>
> @@ -962,8 +962,7 @@ bool multifd_send_setup(void)
> p->write_flags = 0;
>
> if (!multifd_new_send_channel_create(p, &local_err)) {
> - migrate_set_error(s, local_err);
> - error_free(local_err);
> + migrate_error_propagate(s, local_err);
> ret = -1;
> }
> }
> @@ -987,8 +986,7 @@ bool multifd_send_setup(void)
>
> ret = multifd_send_state->ops->send_setup(p, &local_err);
> if (ret) {
> - migrate_set_error(s, local_err);
> - error_free(local_err);
> + migrate_error_propagate(s, local_err);
> goto err;
> }
> assert(p->iov);
> @@ -1067,8 +1065,9 @@ static void multifd_recv_terminate_threads(Error *err)
>
> if (err) {
> MigrationState *s = migrate_get_current();
> - migrate_set_error(s, err);
> - error_free(err);
> +
> + migrate_error_propagate(s, err);
> +
> if (s->state == MIGRATION_STATUS_SETUP ||
> s->state == MIGRATION_STATUS_ACTIVE) {
> migrate_set_state(&s->state, s->state,
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 7c9fe61041..dd8e31f5ae 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -1927,8 +1927,7 @@ postcopy_preempt_send_channel_done(MigrationState *s,
> QIOChannel *ioc, Error *local_err)
> {
> if (local_err) {
> - migrate_set_error(s, local_err);
> - error_free(local_err);
> + migrate_error_propagate(s, local_err);
> } else {
> migration_ioc_register_yank(ioc);
> s->postcopy_qemufile_src = qemu_file_new_output(ioc);
> @@ -2162,7 +2161,7 @@ static void *postcopy_listen_thread(void *opaque)
> * exit depending on if postcopy-exit-on-error is true, but the
> * migration cannot be recovered.
> */
> - migrate_set_error(migr, local_err);
> + migrate_error_propagate(migr, error_copy(local_err));
> error_report_err(local_err);
> migrate_set_state(&mis->state, mis->state,
> MIGRATION_STATUS_FAILED);
> goto out;
> diff --git a/migration/ram.c b/migration/ram.c
> index 29f016cb25..a2f456d858 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -4730,9 +4730,7 @@ static void ram_mig_ram_block_resized(RAMBlockNotifier
> *n, void *host,
> * Abort and indicate a proper reason.
> */
> error_setg(&err, "RAM block '%s' resized during precopy.",
> rb->idstr);
> - migrate_set_error(migrate_get_current(), err);
> - error_free(err);
> -
> + migrate_error_propagate(migrate_get_current(), err);
> migration_cancel();
> }
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 638e9b364f..67f10a5dbe 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1125,13 +1125,12 @@ void qemu_savevm_send_open_return_path(QEMUFile *f)
> int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t len)
> {
> uint32_t tmp;
> - MigrationState *ms = migrate_get_current();
> Error *local_err = NULL;
>
> if (len > MAX_VM_CMD_PACKAGED_SIZE) {
> error_setg(&local_err, "%s: Unreasonably large packaged state: %zu",
> __func__, len);
> - migrate_set_error(ms, local_err);
> + migrate_error_propagate(migrate_get_current(),
> error_copy(local_err));
I'd break this line between the arguments. Matter of taste, up to you.
> error_report_err(local_err);
> return -1;
> }
> @@ -1373,7 +1372,7 @@ int qemu_savevm_state_setup(QEMUFile *f, Error **errp)
> if (se->vmsd && se->vmsd->early_setup) {
> ret = vmstate_save(f, se, vmdesc, errp);
> if (ret) {
> - migrate_set_error(ms, *errp);
> + migrate_error_propagate(ms, error_copy(*errp));
> qemu_file_set_error(f, ret);
> break;
> }
> @@ -1681,7 +1680,7 @@ int
> qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
>
> ret = vmstate_save(f, se, vmdesc, &local_err);
> if (ret) {
> - migrate_set_error(ms, local_err);
> + migrate_error_propagate(ms, error_copy(local_err));
> error_report_err(local_err);
> qemu_file_set_error(f, ret);
> return ret;
> @@ -1858,7 +1857,6 @@ void qemu_savevm_live_state(QEMUFile *f)
>
> int qemu_save_device_state(QEMUFile *f)
> {
> - MigrationState *ms = migrate_get_current();
> Error *local_err = NULL;
> SaveStateEntry *se;
>
> @@ -1876,7 +1874,8 @@ int qemu_save_device_state(QEMUFile *f)
> }
> ret = vmstate_save(f, se, NULL, &local_err);
> if (ret) {
> - migrate_set_error(ms, local_err);
> + migrate_error_propagate(migrate_get_current(),
> + error_copy(local_err));
> error_report_err(local_err);
> return ret;
> }
> @@ -2826,8 +2825,6 @@ static int qemu_loadvm_load_thread(void *thread_opaque)
> Error *local_err = NULL;
>
> if (!data->function(data->opaque, &mis->load_threads_abort, &local_err))
> {
> - MigrationState *s = migrate_get_current();
> -
> /*
> * Can't set load_threads_abort here since processing of main
> migration
> * channel data could still be happening, resulting in launching of
> new
> @@ -2840,8 +2837,7 @@ static int qemu_loadvm_load_thread(void *thread_opaque)
> * In case of multiple load threads failing which thread error
> * return we end setting is purely arbitrary.
> */
> - migrate_set_error(s, local_err);
> - error_free(local_err);
> + migrate_error_propagate(migrate_get_current(), local_err);
> }
>
> return 0;
Reviewed-by: Markus Armbruster <[email protected]>
