On Tue, Dec 02, 2025 at 12:59:40PM +0100, Markus Armbruster wrote:
> 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?
Yes it looks more readable indeed, thanks for the suggestion.
Since the whole series got almost reviewed, instead of reposting everything
I'll send a small follow up patch soon as a separate cleanup.
>
> > }
> >
> > @@ -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.
Would you mind if I queue this series together with patch 2+3? If so, I
can touch it up when queuing.
>
> > 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]>
Thanks!
--
Peter Xu
