Re: [PATCH for-11.0 v2 7/7] migration: Replace migrate_set_error() with migrate_error_propagate()
On Tue, Dec 02, 2025 at 12:59:40PM +0100, Markus Armbruster wrote:
> Peter Xu 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
> > ---
> > 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);
> > +
Re: [PATCH for-11.0 v2 7/7] migration: Replace migrate_set_error() with migrate_error_propagate()
Peter Xu 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
> ---
> 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_e
