Re: [PATCH for-11.0 6/6] migration: Replace migrate_set_error() with migrate_error_propagate()
On Wed, Nov 26, 2025 at 08:57:43AM +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. When at
> > it, remove the first parameter so it's friendly to g_clear_pointer(). It
> > can be used whenever the caller wants to provide extra safety measure (or
> > reuse the pointer) to reset the Error* pointer after stolen.
> >
> > Signed-off-by: Peter Xu
>
> Worth mentioning that this avoids Error object copies?
Sure.
>
> > ---
> > migration/migration.h| 2 +-
> > migration/cpr-exec.c | 4 +--
> > migration/migration.c| 46 +++-
> > migration/multifd-device-state.c | 5 +---
> > migration/multifd.c | 19 +++--
> > migration/postcopy-ram.c | 5 ++--
> > migration/ram.c | 4 +--
> > migration/savevm.c | 15 ---
> > 8 files changed, 42 insertions(+), 58 deletions(-)
> >
> > diff --git a/migration/migration.h b/migration/migration.h
> > index 213b33fe6e..df74f9b14f 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(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..13e6138f56 100644
> > --- a/migration/cpr-exec.c
> > +++ b/migration/cpr-exec.c
> > @@ -158,9 +158,7 @@ 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);
> > -err = NULL;
> > +g_clear_pointer(&err, migrate_error_propagate);
> >
> > /* Note, we can go from state COMPLETED to FAILED */
> > migration_call_notifiers(s, MIG_EVENT_PRECOPY_FAILED, NULL);
>
> I dislike g_clear_pointer(), and I dislike the change from taking the
> migration state as argument to getting the global migration state with
> migrate_get_current(). The loss of similarity to error_propagate() is
> unfortunate, but tolerable. This is not a demand.
>
> For each hunk, we need to prove that the migrate_set_error()'s first
> argument before the patch equals migrate_get_current() afterwards.
It's guaranteed; the only point of set_error() is to persist the error to
the global MigrationState's error field that which will be queried later.
>
> For this hunk, it's locally obvious: @s is initialized to
> migrate_get_current() at the beginning of the funtion.
>
> Where it's not locally obvious, I guess we could argue that just one
> MigrationState object exists, so a MigrationState * can only point to
> that one.
>
> If locally non-obvious hunks exist, such an argument needs to be made in
> the commit message.
>
> I did *not* check this aspect of the patch.
Personally I liked the safety measure that g_clear_pointer() enforces, on
pointer reset together with object release. migrate_error_propagate() is
almost a free function to me, except that it optionally remembers the error
if it's the first one for migration purpose.
But since you don't like it, and Cedric also similarly shared his opinion,
I can keep the MigrationState* arg, and remove this g_clear_pointer() for
now.
Maybe I'll try to "fight" more if we have more use cases of explicit
resetting Error* pointer for reuse like what cpr_exec_cb() does. But that
seems the only use case anyway.. I think we can keep it open-coded.
>
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 4fe69cc2ef..219d3129cb 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(local_err);
> > migration_incoming_state_destroy();
> >
> > if (mis->exit_on_error) {
> > @@ -1548,14 +1546,22 @@ 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(Error *er
Re: [PATCH for-11.0 6/6] 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. When at
> it, remove the first parameter so it's friendly to g_clear_pointer(). It
> can be used whenever the caller wants to provide extra safety measure (or
> reuse the pointer) to reset the Error* pointer after stolen.
>
> Signed-off-by: Peter Xu
Worth mentioning that this avoids Error object copies?
> ---
> migration/migration.h| 2 +-
> migration/cpr-exec.c | 4 +--
> migration/migration.c| 46 +++-
> migration/multifd-device-state.c | 5 +---
> migration/multifd.c | 19 +++--
> migration/postcopy-ram.c | 5 ++--
> migration/ram.c | 4 +--
> migration/savevm.c | 15 ---
> 8 files changed, 42 insertions(+), 58 deletions(-)
>
> diff --git a/migration/migration.h b/migration/migration.h
> index 213b33fe6e..df74f9b14f 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(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..13e6138f56 100644
> --- a/migration/cpr-exec.c
> +++ b/migration/cpr-exec.c
> @@ -158,9 +158,7 @@ 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);
> -err = NULL;
> +g_clear_pointer(&err, migrate_error_propagate);
>
> /* Note, we can go from state COMPLETED to FAILED */
> migration_call_notifiers(s, MIG_EVENT_PRECOPY_FAILED, NULL);
I dislike g_clear_pointer(), and I dislike the change from taking the
migration state as argument to getting the global migration state with
migrate_get_current(). The loss of similarity to error_propagate() is
unfortunate, but tolerable. This is not a demand.
For each hunk, we need to prove that the migrate_set_error()'s first
argument before the patch equals migrate_get_current() afterwards.
For this hunk, it's locally obvious: @s is initialized to
migrate_get_current() at the beginning of the funtion.
Where it's not locally obvious, I guess we could argue that just one
MigrationState object exists, so a MigrationState * can only point to
that one.
If locally non-obvious hunks exist, such an argument needs to be made in
the commit message.
I did *not* check this aspect of the patch.
> diff --git a/migration/migration.c b/migration/migration.c
> index 4fe69cc2ef..219d3129cb 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(local_err);
> migration_incoming_state_destroy();
>
> if (mis->exit_on_error) {
> @@ -1548,14 +1546,22 @@ 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(Error *error)
> {
> -QEMU_LOCK_GUARD(&s->error_mutex);
> +MigrationState *s = migrate_get_current();
>
> +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 +1607,7 @@ static void migration_connect_set_error(MigrationState
> *s, Error *error)
> }
>
> migrate_set_state(&s->state, current, next);
> -migrate_set_error(s, error);
> -error_free(error);
> +migrate_error_propagate(error);
> }
>
> void migration_cancel(void)
> @@ -2014,8 +2019,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(error);
>
> qemu_mutex_lock(&ms->qemu_file_lock);
> if (ms->to_dst_file) {
[PATCH for-11.0 6/6] 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. When at
it, remove the first parameter so it's friendly to g_clear_pointer(). It
can be used whenever the caller wants to provide extra safety measure (or
reuse the pointer) to reset the Error* pointer after stolen.
Signed-off-by: Peter Xu
---
migration/migration.h| 2 +-
migration/cpr-exec.c | 4 +--
migration/migration.c| 46 +++-
migration/multifd-device-state.c | 5 +---
migration/multifd.c | 19 +++--
migration/postcopy-ram.c | 5 ++--
migration/ram.c | 4 +--
migration/savevm.c | 15 ---
8 files changed, 42 insertions(+), 58 deletions(-)
diff --git a/migration/migration.h b/migration/migration.h
index 213b33fe6e..df74f9b14f 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(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..13e6138f56 100644
--- a/migration/cpr-exec.c
+++ b/migration/cpr-exec.c
@@ -158,9 +158,7 @@ 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);
-err = NULL;
+g_clear_pointer(&err, migrate_error_propagate);
/* Note, we can go from state COMPLETED to FAILED */
migration_call_notifiers(s, MIG_EVENT_PRECOPY_FAILED, NULL);
diff --git a/migration/migration.c b/migration/migration.c
index 4fe69cc2ef..219d3129cb 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(local_err);
migration_incoming_state_destroy();
if (mis->exit_on_error) {
@@ -1548,14 +1546,22 @@ 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(Error *error)
{
-QEMU_LOCK_GUARD(&s->error_mutex);
+MigrationState *s = migrate_get_current();
+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 +1607,7 @@ static void migration_connect_set_error(MigrationState
*s, Error *error)
}
migrate_set_state(&s->state, current, next);
-migrate_set_error(s, error);
-error_free(error);
+migrate_error_propagate(error);
}
void migration_cancel(void)
@@ -2014,8 +2019,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(error);
qemu_mutex_lock(&ms->qemu_file_lock);
if (ms->to_dst_file) {
@@ -2647,8 +2651,7 @@ static void *source_return_path_thread(void *opaque)
out:
if (err) {
-migrate_set_error(ms, err);
-error_free(err);
+migrate_error_propagate(err);
trace_source_return_path_thread_bad_end();
}
@@ -3094,12 +3097,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(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(local_err);
}
if (s->state != MIGRATION_STATUS_CANCELLING) {
@@ -3326,8 +3327,7 @@ static MigThrError migration_detect_error(MigrationState
*s)
}
if (local_error) {
-migrate_set_error(s, local_error);
-error_free(local_error);
+migrate_error_propagate(local_error);
}
if (state == MIGRATION_STATUS_PO
