Re: [PATCH for-11.0 v2 7/7] migration: Replace migrate_set_error() with migrate_error_propagate()

[Peter Xu <peterx@redhat.com>]

On Tue, Dec 02, 2025 at 12:59:40PM +0100, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> 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 <peterx@redhat.com>
> > ---
> >  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 <armbru@redhat.com>

Thanks!

-- 
Peter Xu