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
[PATCH for-11.0 v2 7/7] migration: Replace migrate_set_error() with migrate_error_propagate()
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);
}
}
@@ -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 && qa
