[PATCH for-11.0 0/6] migration: Error reporting cleanups

[PATCH for-11.0 0/6] migration: Error reporting cleanups

[Peter Xu]

[not -rc material; target QEMU 11.0 only]

Based-on: <20251125070554.2256181-1-armbru@redhat.com>

This series is based on Markus's recent fix:

[PATCH] migration: Fix double-free on error path
https://lore.kernel.org/r/20251125070554.2256181-1-armbru@redhat.com

This series should address the issues discussed in this thread here:

https://lore.kernel.org/r/871plmk1bc.fsf@pond.sub.org

The problem is Error is not a good candidate of g_autoptr, however the
cleanup function was accidentally merged.  Luckily, we only have two users
so far (after Markus's patch above).  This series removes the last two in
migration code and reverts the auto cleanup function for Error.

When at it, it'll also change migrate_set_error() to start taking ownership
of errors, just like what most error APIs do.  When at it, it is renamed to
migrate_error_propagate() to imply migration version of error_propagate().

Comments welcomed, thanks.

Peter Xu (6):
  migration: Use explicit error_free() instead of g_autoptr
  Revert "error: define g_autoptr() cleanup function for the Error type"
  migration: Make migration_connect_set_error() own the error
  migration: Make multifd_send_set_error() own the error
  migration: Make multifd_recv_terminate_threads() own the error
  migration: Replace migrate_set_error() with migrate_error_propagate()

 include/qapi/error.h             |  2 --
 migration/migration.h            |  2 +-
 migration/channel.c              |  1 -
 migration/cpr-exec.c             |  4 +--
 migration/migration.c            | 51 ++++++++++++++++----------------
 migration/multifd-device-state.c |  6 ++--
 migration/multifd.c              | 24 +++++++--------
 migration/postcopy-ram.c         |  5 ++--
 migration/ram.c                  |  4 +--
 migration/savevm.c               | 16 ++++------
 10 files changed, 49 insertions(+), 66 deletions(-)

-- 
2.50.1

[PATCH for-11.0 1/6] migration: Use explicit error_free() instead of g_autoptr

[Peter Xu]

There're only two use cases of g_autoptr to free Error objects in migration
code paths.

Due to the nature of how Error should be used (normally ownership will be
passed over to Error APIs, like error_report_err), auto-free functions may
be error prone on its own.  The auto cleanup function was accidentally
merged as pointed out by Dan and Markus:

https://lore.kernel.org/r/aSWSLMi6ZhTCS_p2@redhat.com

Remove the two use cases so that we can remove the auto cleanup function,
hence suggest to not use auto frees for Errors.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/multifd-device-state.c | 3 ++-
 migration/savevm.c               | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/migration/multifd-device-state.c b/migration/multifd-device-state.c
index fce64f00b0..db3239fef5 100644
--- a/migration/multifd-device-state.c
+++ b/migration/multifd-device-state.c
@@ -140,7 +140,7 @@ static void multifd_device_state_save_thread_data_free(void *opaque)
 static int multifd_device_state_save_thread(void *opaque)
 {
     SaveCompletePrecopyThreadData *data = opaque;
-    g_autoptr(Error) local_err = NULL;
+    Error *local_err = NULL;
 
     if (!data->hdlr(data, &local_err)) {
         MigrationState *s = migrate_get_current();
@@ -159,6 +159,7 @@ static int multifd_device_state_save_thread(void *opaque)
          * return we end setting is purely arbitrary.
          */
         migrate_set_error(s, local_err);
+        error_free(local_err);
     }
 
     return 0;
diff --git a/migration/savevm.c b/migration/savevm.c
index 62cc2ce25c..638e9b364f 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2823,7 +2823,7 @@ static int qemu_loadvm_load_thread(void *thread_opaque)
 {
     struct LoadThreadData *data = thread_opaque;
     MigrationIncomingState *mis = migration_incoming_get_current();
-    g_autoptr(Error) local_err = NULL;
+    Error *local_err = NULL;
 
     if (!data->function(data->opaque, &mis->load_threads_abort, &local_err)) {
         MigrationState *s = migrate_get_current();
@@ -2841,6 +2841,7 @@ static int qemu_loadvm_load_thread(void *thread_opaque)
          * return we end setting is purely arbitrary.
          */
         migrate_set_error(s, local_err);
+        error_free(local_err);
     }
 
     return 0;
-- 
2.50.1

[PATCH for-11.0 2/6] Revert "error: define g_autoptr() cleanup function for the Error type"

[Peter Xu]

This reverts commit 18eb55546a54e443d94a4c49286348176ad4b00a.  Discussion
can be seen at:

https://lore.kernel.org/r/aSWSLMi6ZhTCS_p2@redhat.com

Cc: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
Cc: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/qapi/error.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index b16c6303f8..f3ce4a4a2d 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -437,8 +437,6 @@ Error *error_copy(const Error *err);
  */
 void error_free(Error *err);
 
-G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free)
-
 /*
  * Convenience function to assert that *@errp is set, then silently free it.
  */
-- 
2.50.1

[PATCH for-11.0 3/6] migration: Make migration_connect_set_error() own the error

[Peter Xu]

Make migration_connect_set_error() take ownership of the error always.
Paving way for making migrate_set_error() to take ownership.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/channel.c   | 1 -
 migration/migration.c | 7 ++++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/migration/channel.c b/migration/channel.c
index 462cc183e1..92435fa7f7 100644
--- a/migration/channel.c
+++ b/migration/channel.c
@@ -95,7 +95,6 @@ void migration_channel_connect(MigrationState *s,
         }
     }
     migration_connect(s, error);
-    error_free(error);
 }
 
 
diff --git a/migration/migration.c b/migration/migration.c
index b316ee01ab..4fe69cc2ef 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1575,7 +1575,7 @@ static void migrate_error_free(MigrationState *s)
     }
 }
 
-static void migration_connect_set_error(MigrationState *s, const Error *error)
+static void migration_connect_set_error(MigrationState *s, Error *error)
 {
     MigrationStatus current = s->state;
     MigrationStatus next;
@@ -1602,6 +1602,7 @@ static void migration_connect_set_error(MigrationState *s, const Error *error)
 
     migrate_set_state(&s->state, current, next);
     migrate_set_error(s, error);
+    error_free(error);
 }
 
 void migration_cancel(void)
@@ -2292,7 +2293,7 @@ void qmp_migrate(const char *uri, bool has_channels,
 
 out:
     if (local_err) {
-        migration_connect_set_error(s, local_err);
+        migration_connect_set_error(s, error_copy(local_err));
         error_propagate(errp, local_err);
     }
 }
@@ -2337,7 +2338,7 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
         if (!resume_requested) {
             yank_unregister_instance(MIGRATION_YANK_INSTANCE);
         }
-        migration_connect_set_error(s, local_err);
+        migration_connect_set_error(s, error_copy(local_err));
         error_propagate(errp, local_err);
         return;
     }
-- 
2.50.1

[PATCH for-11.0 4/6] migration: Make multifd_send_set_error() own the error

[Peter Xu]

Make multifd_send_set_error() take ownership of the error always.  Paving
way for making migrate_set_error() to take ownership.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/multifd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index 3203dc98e1..930eee9949 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -429,6 +429,7 @@ static void multifd_send_set_error(Error *err)
     if (err) {
         MigrationState *s = migrate_get_current();
         migrate_set_error(s, err);
+        error_free(err);
         if (s->state == MIGRATION_STATUS_SETUP ||
             s->state == MIGRATION_STATUS_PRE_SWITCHOVER ||
             s->state == MIGRATION_STATUS_DEVICE ||
@@ -779,7 +780,6 @@ out:
         trace_multifd_send_error(p->id);
         multifd_send_set_error(local_err);
         multifd_send_kick_main(p);
-        error_free(local_err);
     }
 
     rcu_unregister_thread();
@@ -908,7 +908,6 @@ out:
      * cleanup code doesn't even know its existence.
      */
     object_unref(OBJECT(ioc));
-    error_free(local_err);
 }
 
 static bool multifd_new_send_channel_create(gpointer opaque, Error **errp)
-- 
2.50.1

[PATCH for-11.0 5/6] migration: Make multifd_recv_terminate_threads() own the error

[Peter Xu]

Make multifd_recv_terminate_threads() take ownership of the error always.
Paving way for making migrate_set_error() to take ownership.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/multifd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index 930eee9949..c861b4b557 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -1068,6 +1068,7 @@ static void multifd_recv_terminate_threads(Error *err)
     if (err) {
         MigrationState *s = migrate_get_current();
         migrate_set_error(s, err);
+        error_free(err);
         if (s->state == MIGRATION_STATUS_SETUP ||
             s->state == MIGRATION_STATUS_ACTIVE) {
             migrate_set_state(&s->state, s->state,
@@ -1434,7 +1435,6 @@ static void *multifd_recv_thread(void *opaque)
 
     if (local_err) {
         multifd_recv_terminate_threads(local_err);
-        error_free(local_err);
     }
 
     rcu_unregister_thread();
@@ -1535,7 +1535,7 @@ void multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
     if (use_packets) {
         id = multifd_recv_initial_packet(ioc, &local_err);
         if (id < 0) {
-            multifd_recv_terminate_threads(local_err);
+            multifd_recv_terminate_threads(error_copy(local_err));
             error_propagate_prepend(errp, local_err,
                                     "failed to receive packet"
                                     " via multifd channel %d: ",
@@ -1551,7 +1551,7 @@ void multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
     if (p->c != NULL) {
         error_setg(&local_err, "multifd: received id '%d' already setup'",
                    id);
-        multifd_recv_terminate_threads(local_err);
+        multifd_recv_terminate_threads(error_copy(local_err));
         error_propagate(errp, local_err);
         return;
     }
-- 
2.50.1

[PATCH for-11.0 6/6] migration: Replace migrate_set_error() with migrate_error_propagate()

[Peter Xu]

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 <peterx@redhat.com>
---
 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_POSTCOPY_ACTIVE && ret) {
@@ -3522,7 +3522,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(error_copy(local_err));
                 error_report_err(local_err);
             }
             return MIG_ITERATE_SKIP;
@@ -3819,8 +3819,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(local_err);
         migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
                           MIGRATION_STATUS_FAILED);
         goto out;
@@ -3944,8 +3943,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(local_err);
         migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
                           MIGRATION_STATUS_FAILED);
         goto fail_setup;
@@ -4127,7 +4125,7 @@ void migration_connect(MigrationState *s, Error *error_in)
     return;
 
 fail:
-    migrate_set_error(s, local_err);
+    migrate_error_propagate(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..3040d70872 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(local_err);
     }
 
     return 0;
diff --git a/migration/multifd.c b/migration/multifd.c
index c861b4b557..99717b64e9 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -428,8 +428,9 @@ static void multifd_send_set_error(Error *err)
 
     if (err) {
         MigrationState *s = migrate_get_current();
-        migrate_set_error(s, err);
-        error_free(err);
+
+        migrate_error_propagate(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(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(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(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(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..856366072a 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(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(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..1d2a47526e 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(err);
         migration_cancel();
     }
 
diff --git a/migration/savevm.c b/migration/savevm.c
index 638e9b364f..ab9d1e579e 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(error_copy(local_err));
         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(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(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,7 @@ 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(error_copy(local_err));
             error_report_err(local_err);
             return ret;
         }
@@ -2826,8 +2824,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 +2836,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(local_err);
     }
 
     return 0;
-- 
2.50.1

Re: [PATCH for-11.0 1/6] migration: Use explicit error_free() instead of g_autoptr

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> There're only two use cases of g_autoptr to free Error objects in migration
> code paths.
>
> Due to the nature of how Error should be used (normally ownership will be
> passed over to Error APIs, like error_report_err), auto-free functions may
> be error prone on its own.  The auto cleanup function was accidentally
> merged as pointed out by Dan and Markus:
>
> https://lore.kernel.org/r/aSWSLMi6ZhTCS_p2@redhat.com

Perhaps "merged without proper review" would be more accurate.

> Remove the two use cases so that we can remove the auto cleanup function,
> hence suggest to not use auto frees for Errors.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH for-11.0 2/6] Revert "error: define g_autoptr() cleanup function for the Error type"

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> This reverts commit 18eb55546a54e443d94a4c49286348176ad4b00a.  Discussion
> can be seen at:
>
> https://lore.kernel.org/r/aSWSLMi6ZhTCS_p2@redhat.com

Suggest to steal from the previous commit like this:

  Due to the nature of how Error should be used (normally ownership will be
  passed over to Error APIs, like error_report_err), auto-free functions may
  be error prone on its own.  The auto cleanup function was merged without 
  proper review as pointed out by Dan and Markus:

  https://lore.kernel.org/r/aSWSLMi6ZhTCS_p2@redhat.com

> Cc: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> Cc: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/qapi/error.h | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index b16c6303f8..f3ce4a4a2d 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -437,8 +437,6 @@ Error *error_copy(const Error *err);
>   */
>  void error_free(Error *err);
>  
> -G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free)
> -
>  /*
>   * Convenience function to assert that *@errp is set, then silently free it.
>   */

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH for-11.0 3/6] migration: Make migration_connect_set_error() own the error

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> Make migration_connect_set_error() take ownership of the error always.
> Paving way for making migrate_set_error() to take ownership.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/channel.c   | 1 -
>  migration/migration.c | 7 ++++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/migration/channel.c b/migration/channel.c
> index 462cc183e1..92435fa7f7 100644
> --- a/migration/channel.c
> +++ b/migration/channel.c
> @@ -95,7 +95,6 @@ void migration_channel_connect(MigrationState *s,
>          }
>      }
>      migration_connect(s, error);
> -    error_free(error);
>  }
>  
>  
> diff --git a/migration/migration.c b/migration/migration.c
> index b316ee01ab..4fe69cc2ef 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1575,7 +1575,7 @@ static void migrate_error_free(MigrationState *s)
>      }
>  }
>  
> -static void migration_connect_set_error(MigrationState *s, const Error *error)
> +static void migration_connect_set_error(MigrationState *s, Error *error)

Recommend to rename for the same reason you rename migrate_set_error()
in the last patch.

Bonus: all calls become visible in the patch, easing review.

>  {
>      MigrationStatus current = s->state;
>      MigrationStatus next;
> @@ -1602,6 +1602,7 @@ static void migration_connect_set_error(MigrationState *s, const Error *error)
>  
>      migrate_set_state(&s->state, current, next);
>      migrate_set_error(s, error);
> +    error_free(error);
>  }
>  
>  void migration_cancel(void)
> @@ -2292,7 +2293,7 @@ void qmp_migrate(const char *uri, bool has_channels,
>  
>  out:
>      if (local_err) {
> -        migration_connect_set_error(s, local_err);
> +        migration_connect_set_error(s, error_copy(local_err));
>          error_propagate(errp, local_err);
>      }
>  }
> @@ -2337,7 +2338,7 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
>          if (!resume_requested) {
>              yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>          }
> -        migration_connect_set_error(s, local_err);
> +        migration_connect_set_error(s, error_copy(local_err));
>          error_propagate(errp, local_err);
>          return;
>      }

There's one more call in migration_connect().  Doesn't it need an
error_copy() now?  Oh, I see: it doesn't, because its only caller
migration_channel_connect() loses its error_free() in the first hunk.

So, migration_connect() *also* takes ownership now.  The commit message
should cover this.

Aside: I'd be tempted to lift the if (error_in) code from
migration_connect() to migration_channel_connect() and drop the
@error_in parameter.

Re: [PATCH for-11.0 4/6] migration: Make multifd_send_set_error() own the error

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> Make multifd_send_set_error() take ownership of the error always.  Paving
> way for making migrate_set_error() to take ownership.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH for-11.0 4/6] migration: Make multifd_send_set_error() own the error

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Peter Xu <peterx@redhat.com> writes:
>
>> Make multifd_send_set_error() take ownership of the error always.  Paving
>> way for making migrate_set_error() to take ownership.
>>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Forgot to mention: recommend to rename for the same reason you rename
migrate_set_error() in the last patch.

Re: [PATCH for-11.0 2/6] Revert "error: define g_autoptr() cleanup function for the Error type"

[Cédric Le Goater]

On 11/25/25 21:46, Peter Xu wrote:
> This reverts commit 18eb55546a54e443d94a4c49286348176ad4b00a.  Discussion
> can be seen at:
> 
> https://lore.kernel.org/r/aSWSLMi6ZhTCS_p2@redhat.com
> 
> Cc: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> Cc: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   include/qapi/error.h | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index b16c6303f8..f3ce4a4a2d 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -437,8 +437,6 @@ Error *error_copy(const Error *err);
>    */
>   void error_free(Error *err);
>   
> -G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free)
> -
>   /*
>    * Convenience function to assert that *@errp is set, then silently free it.
>    */

Is that related to CID 1643463 issue ?

anyhow,


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

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

[Markus Armbruster]

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.  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 <peterx@redhat.com>

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) {
> @@ -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_POSTCOPY_ACTIVE && ret) {
> @@ -3522,7 +3522,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(error_copy(local_err));

First hunk where we don't save a copy.  Reason: we report in addition to
propagate.  There are a several more below.

"Forking" the error like this gives me an uneasy feeling, as explained
in
    Subject: Re: [PATCH 0/3] migration: Error fixes and improvements
    Date: Tue, 18 Nov 2025 08:44:32 +0100
    Message-ID: <87a50jlr8f.fsf@pond.sub.org>

Clearly out of scope for this series.

>                  error_report_err(local_err);
>              }
>              return MIG_ITERATE_SKIP;
> @@ -3819,8 +3819,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(local_err);
>          migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
>                            MIGRATION_STATUS_FAILED);
>          goto out;
> @@ -3944,8 +3943,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(local_err);
>          migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
>                            MIGRATION_STATUS_FAILED);
>          goto fail_setup;
> @@ -4127,7 +4125,7 @@ void migration_connect(MigrationState *s, Error *error_in)
>      return;
>  
>  fail:
> -    migrate_set_error(s, local_err);
> +    migrate_error_propagate(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..3040d70872 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(local_err);
>      }
>  
>      return 0;
> diff --git a/migration/multifd.c b/migration/multifd.c
> index c861b4b557..99717b64e9 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -428,8 +428,9 @@ static void multifd_send_set_error(Error *err)
>  
>      if (err) {
>          MigrationState *s = migrate_get_current();
> -        migrate_set_error(s, err);
> -        error_free(err);
> +
> +        migrate_error_propagate(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(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(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(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(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..856366072a 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(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(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..1d2a47526e 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(err);
>          migration_cancel();
>      }
>  
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 638e9b364f..ab9d1e579e 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(error_copy(local_err));
>          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(error_copy(*errp));

Here, we need to keep the copy because we additionally propagate to the
caller.

>                  qemu_file_set_error(f, ret);
>                  break;
>              }

[...]

Re: [PATCH for-11.0 2/6] Revert "error: define g_autoptr() cleanup function for the Error type"

[Markus Armbruster]

Cédric Le Goater <clg@redhat.com> writes:

> On 11/25/25 21:46, Peter Xu wrote:
>> This reverts commit 18eb55546a54e443d94a4c49286348176ad4b00a.  Discussion
>> can be seen at:
>> https://lore.kernel.org/r/aSWSLMi6ZhTCS_p2@redhat.com
>> Cc: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
>> Cc: Cédric Le Goater <clg@redhat.com>
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> ---
>>  include/qapi/error.h | 2 --
>>  1 file changed, 2 deletions(-)
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index b16c6303f8..f3ce4a4a2d 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -437,8 +437,6 @@ Error *error_copy(const Error *err);
>>   */
>>  void error_free(Error *err);
>>
>> -G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free)
>> -
>>  /*
>>   * Convenience function to assert that *@errp is set, then silently free it.
>>   */
>
> Is that related to CID 1643463 issue ?

g_autoptr(Error) is a bad idea, as discussed in

    Subject: g_autoptr(Error) (was: [PATCH] migration: Fix double-free on error path) 
    Date: Tue, 25 Nov 2025 08:40:07 +0100
    Message-ID: <871plmk1bc.fsf@pond.sub.org>

We have three instances of g_autoptr(Error) in master.  CID 1643463 made
me see them.

One is removed by my fix to CID 1643463:

    Subject: [PATCH] migration: Fix double-free on error path
    Date: Tue, 25 Nov 2025 08:05:54 +0100
    Message-ID: <20251125070554.2256181-1-armbru@redhat.com>

The remaining two get removed in PATCH 1.  This patch deletes the code
that makes g_autoptr(Error) work.

> anyhow,
>
>
> Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thank you!

[PATCH 2.5/6] error: Explain why we don't g_autoptr(Error)

[Markus Armbruster]

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/error.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index f3ce4a4a2d..fc018b4c59 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -437,6 +437,23 @@ Error *error_copy(const Error *err);
  */
 void error_free(Error *err);
 
+/*
+ * Note: we intentionally do not enable g_autoptr(Error) with
+ * G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(Error, error_free).
+ *
+ * Functions that report or propagate an error take ownership of the
+ * Error object.  Explicit error_free() is needed when you handle an
+ * error in some other way.  This is rare.
+ *
+ * g_autoptr(Error) would call error_free() automatically on return.
+ * To avoid a double-free, we'd have to manually clear the pointer
+ * every time we propagate or report.
+ *
+ * Thus, g_autoptr(Error) would make the rare case easier to get right
+ * (less prone to leaks), and the common case easier to get wrong
+ * (more prone to double-free).
+ */
+
 /*
  * Convenience function to assert that *@errp is set, then silently free it.
  */
-- 
2.49.0

Re: [PATCH 2.5/6] error: Explain why we don't g_autoptr(Error)

[Cédric Le Goater]

On 11/26/25 15:34, Markus Armbruster wrote:
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   include/qapi/error.h | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index f3ce4a4a2d..fc018b4c59 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -437,6 +437,23 @@ Error *error_copy(const Error *err);
>    */
>   void error_free(Error *err);
>   
> +/*
> + * Note: we intentionally do not enable g_autoptr(Error) with
> + * G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(Error, error_free).
> + *
> + * Functions that report or propagate an error take ownership of the
> + * Error object.  Explicit error_free() is needed when you handle an
> + * error in some other way.  This is rare.
> + *
> + * g_autoptr(Error) would call error_free() automatically on return.
> + * To avoid a double-free, we'd have to manually clear the pointer
> + * every time we propagate or report.
> + *
> + * Thus, g_autoptr(Error) would make the rare case easier to get right
> + * (less prone to leaks), and the common case easier to get wrong
> + * (more prone to double-free).
> + */
> +
>   /*
>    * Convenience function to assert that *@errp is set, then silently free it.
>    */


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

Re: [PATCH 2.5/6] error: Explain why we don't g_autoptr(Error)

[Peter Xu]

On Wed, Nov 26, 2025 at 04:14:55PM +0100, Cédric Le Goater wrote:
> On 11/26/25 15:34, Markus Armbruster wrote:
> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> > ---
> >   include/qapi/error.h | 17 +++++++++++++++++
> >   1 file changed, 17 insertions(+)
> > 
> > diff --git a/include/qapi/error.h b/include/qapi/error.h
> > index f3ce4a4a2d..fc018b4c59 100644
> > --- a/include/qapi/error.h
> > +++ b/include/qapi/error.h
> > @@ -437,6 +437,23 @@ Error *error_copy(const Error *err);
> >    */
> >   void error_free(Error *err);
> > +/*
> > + * Note: we intentionally do not enable g_autoptr(Error) with
> > + * G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(Error, error_free).
> > + *
> > + * Functions that report or propagate an error take ownership of the
> > + * Error object.  Explicit error_free() is needed when you handle an
> > + * error in some other way.  This is rare.
> > + *
> > + * g_autoptr(Error) would call error_free() automatically on return.
> > + * To avoid a double-free, we'd have to manually clear the pointer
> > + * every time we propagate or report.
> > + *
> > + * Thus, g_autoptr(Error) would make the rare case easier to get right
> > + * (less prone to leaks), and the common case easier to get wrong
> > + * (more prone to double-free).

How about we further poison the auto free altogether?

IIUC this should work:

+extern void
+__attribute__((error("Error should not be used with g_autoptr")))
+error_free_poisoned(Error *err);
+
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free_poisoned)

> > + */
> > +
> >   /*
> >    * Convenience function to assert that *@errp is set, then silently free it.
> >    */
> 
> 
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> 
> Thanks,
> 
> C.
> 
> 

-- 
Peter Xu

Re: [PATCH for-11.0 2/6] Revert "error: define g_autoptr() cleanup function for the Error type"

[Maciej S. Szmigiero]

On 25.11.2025 21:46, Peter Xu wrote:
> This reverts commit 18eb55546a54e443d94a4c49286348176ad4b00a.  Discussion
> can be seen at:
> 
> https://lore.kernel.org/r/aSWSLMi6ZhTCS_p2@redhat.com
> 
> Cc: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> Cc: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   include/qapi/error.h | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index b16c6303f8..f3ce4a4a2d 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -437,8 +437,6 @@ Error *error_copy(const Error *err);
>    */
>   void error_free(Error *err);
>   
> -G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free)
> -
>   /*
>    * Convenience function to assert that *@errp is set, then silently free it.
>    */

Acked-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

Thanks,
Maciej

Re: [PATCH 2.5/6] error: Explain why we don't g_autoptr(Error)

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Wed, Nov 26, 2025 at 04:14:55PM +0100, Cédric Le Goater wrote:
>> On 11/26/25 15:34, Markus Armbruster wrote:
>> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> > ---
>> >   include/qapi/error.h | 17 +++++++++++++++++
>> >   1 file changed, 17 insertions(+)
>> > 
>> > diff --git a/include/qapi/error.h b/include/qapi/error.h
>> > index f3ce4a4a2d..fc018b4c59 100644
>> > --- a/include/qapi/error.h
>> > +++ b/include/qapi/error.h
>> > @@ -437,6 +437,23 @@ Error *error_copy(const Error *err);
>> >    */
>> >   void error_free(Error *err);
>> > +/*
>> > + * Note: we intentionally do not enable g_autoptr(Error) with
>> > + * G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(Error, error_free).
>> > + *
>> > + * Functions that report or propagate an error take ownership of the
>> > + * Error object.  Explicit error_free() is needed when you handle an
>> > + * error in some other way.  This is rare.
>> > + *
>> > + * g_autoptr(Error) would call error_free() automatically on return.
>> > + * To avoid a double-free, we'd have to manually clear the pointer
>> > + * every time we propagate or report.
>> > + *
>> > + * Thus, g_autoptr(Error) would make the rare case easier to get right
>> > + * (less prone to leaks), and the common case easier to get wrong
>> > + * (more prone to double-free).
>
> How about we further poison the auto free altogether?
>
> IIUC this should work:
>
> +extern void
> +__attribute__((error("Error should not be used with g_autoptr")))
> +error_free_poisoned(Error *err);
> +
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free_poisoned)

Cute.  Why not.  I'll post a new patch.

[PATCH 2.5/6] error: Poison g_autoptr(Error) to prevent its use

[Markus Armbruster]

The previous commit reverted support for g_autoptr(Error).  This one
should stop it from coming back.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qapi/error.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index f3ce4a4a2d..2356b84bb3 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -437,6 +437,26 @@ Error *error_copy(const Error *err);
  */
 void error_free(Error *err);
 
+/*
+ * Poison g_autoptr(Error) to prevent its use.
+ *
+ * Functions that report or propagate an error take ownership of the
+ * Error object.  Explicit error_free() is needed when you handle an
+ * error in some other way.  This is rare.
+ *
+ * g_autoptr(Error) would call error_free() automatically on return.
+ * To avoid a double-free, we'd have to manually clear the pointer
+ * every time we propagate or report.
+ *
+ * Thus, g_autoptr(Error) would make the rare case easier to get right
+ * (less prone to leaks), and the common case easier to get wrong
+ * (more prone to double-free).
+ */
+extern void
+__attribute__((error("Do not use g_autoptr() to declare Error * variables")))
+error_free_poisoned(Error *err);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free_poisoned)
+
 /*
  * Convenience function to assert that *@errp is set, then silently free it.
  */
-- 
2.49.0

Re: [PATCH 2.5/6] error: Poison g_autoptr(Error) to prevent its use

[Cédric Le Goater]

On 11/27/25 08:10, Markus Armbruster wrote:
> The previous commit reverted support for g_autoptr(Error).  This one
> should stop it from coming back.
> 
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   include/qapi/error.h | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index f3ce4a4a2d..2356b84bb3 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -437,6 +437,26 @@ Error *error_copy(const Error *err);
>    */
>   void error_free(Error *err);
>   
> +/*
> + * Poison g_autoptr(Error) to prevent its use.
> + *
> + * Functions that report or propagate an error take ownership of the
> + * Error object.  Explicit error_free() is needed when you handle an
> + * error in some other way.  This is rare.
> + *
> + * g_autoptr(Error) would call error_free() automatically on return.
> + * To avoid a double-free, we'd have to manually clear the pointer
> + * every time we propagate or report.
> + *
> + * Thus, g_autoptr(Error) would make the rare case easier to get right
> + * (less prone to leaks), and the common case easier to get wrong
> + * (more prone to double-free).
> + */
> +extern void
> +__attribute__((error("Do not use g_autoptr() to declare Error * variables")))
> +error_free_poisoned(Error *err);
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(Error, error_free_poisoned)
> +
>   /*
>    * Convenience function to assert that *@errp is set, then silently free it.
>    */

broken build, even better.

Tested-by: Cédric Le Goater <clg@redhat.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

Re: [PATCH for-11.0 3/6] migration: Make migration_connect_set_error() own the error

[Peter Xu]

On Wed, Nov 26, 2025 at 08:27:48AM +0100, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > Make migration_connect_set_error() take ownership of the error always.
> > Paving way for making migrate_set_error() to take ownership.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/channel.c   | 1 -
> >  migration/migration.c | 7 ++++---
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/migration/channel.c b/migration/channel.c
> > index 462cc183e1..92435fa7f7 100644
> > --- a/migration/channel.c
> > +++ b/migration/channel.c
> > @@ -95,7 +95,6 @@ void migration_channel_connect(MigrationState *s,
> >          }
> >      }
> >      migration_connect(s, error);
> > -    error_free(error);
> >  }
> >  
> >  
> > diff --git a/migration/migration.c b/migration/migration.c
> > index b316ee01ab..4fe69cc2ef 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -1575,7 +1575,7 @@ static void migrate_error_free(MigrationState *s)
> >      }
> >  }
> >  
> > -static void migration_connect_set_error(MigrationState *s, const Error *error)
> > +static void migration_connect_set_error(MigrationState *s, Error *error)
> 
> Recommend to rename for the same reason you rename migrate_set_error()
> in the last patch.
> 
> Bonus: all calls become visible in the patch, easing review.

Makes sense, will do.

> 
> >  {
> >      MigrationStatus current = s->state;
> >      MigrationStatus next;
> > @@ -1602,6 +1602,7 @@ static void migration_connect_set_error(MigrationState *s, const Error *error)
> >  
> >      migrate_set_state(&s->state, current, next);
> >      migrate_set_error(s, error);
> > +    error_free(error);
> >  }
> >  
> >  void migration_cancel(void)
> > @@ -2292,7 +2293,7 @@ void qmp_migrate(const char *uri, bool has_channels,
> >  
> >  out:
> >      if (local_err) {
> > -        migration_connect_set_error(s, local_err);
> > +        migration_connect_set_error(s, error_copy(local_err));
> >          error_propagate(errp, local_err);
> >      }
> >  }
> > @@ -2337,7 +2338,7 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
> >          if (!resume_requested) {
> >              yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> >          }
> > -        migration_connect_set_error(s, local_err);
> > +        migration_connect_set_error(s, error_copy(local_err));
> >          error_propagate(errp, local_err);
> >          return;
> >      }
> 
> There's one more call in migration_connect().  Doesn't it need an
> error_copy() now?  Oh, I see: it doesn't, because its only caller
> migration_channel_connect() loses its error_free() in the first hunk.
> 
> So, migration_connect() *also* takes ownership now.  The commit message
> should cover this.

Will add a note.

> 
> Aside: I'd be tempted to lift the if (error_in) code from
> migration_connect() to migration_channel_connect() and drop the
> @error_in parameter.

It was deliberately introduced in:

commit 688a3dcba980bf01344a1ae2bc37fea44c6014ac
Author: Dr. David Alan Gilbert <dgilbert@redhat.com>
Date:   Fri Dec 15 17:16:55 2017 +0000

    migration: Route errors down through migration_channel_connect

That change will need some justification and care taken on its own when
changing back to before.  I plan to stick with the current goal of this
series so far (which is to remove autoptr for Error and also make the main
migration error API to follow Error's) to land this first.

Thanks!

-- 
Peter Xu

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

[Peter Xu]

On Wed, Nov 26, 2025 at 08:57:43AM +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.  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 <peterx@redhat.com>
> 
> 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 *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_POSTCOPY_ACTIVE && ret) {
> > @@ -3522,7 +3522,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(error_copy(local_err));
> 
> First hunk where we don't save a copy.  Reason: we report in addition to
> propagate.  There are a several more below.
> 
> "Forking" the error like this gives me an uneasy feeling, as explained
> in
>     Subject: Re: [PATCH 0/3] migration: Error fixes and improvements
>     Date: Tue, 18 Nov 2025 08:44:32 +0100
>     Message-ID: <87a50jlr8f.fsf@pond.sub.org>
> 
> Clearly out of scope for this series.

Yes, and we discussed the need of doing that in the other thread as well.
Hope that justifies we should keep it until we want to change the behavior
of error reports.

Thanks,

> 
> >                  error_report_err(local_err);
> >              }
> >              return MIG_ITERATE_SKIP;
> > @@ -3819,8 +3819,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(local_err);
> >          migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
> >                            MIGRATION_STATUS_FAILED);
> >          goto out;
> > @@ -3944,8 +3943,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(local_err);
> >          migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,
> >                            MIGRATION_STATUS_FAILED);
> >          goto fail_setup;
> > @@ -4127,7 +4125,7 @@ void migration_connect(MigrationState *s, Error *error_in)
> >      return;
> >  
> >  fail:
> > -    migrate_set_error(s, local_err);
> > +    migrate_error_propagate(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..3040d70872 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(local_err);
> >      }
> >  
> >      return 0;
> > diff --git a/migration/multifd.c b/migration/multifd.c
> > index c861b4b557..99717b64e9 100644
> > --- a/migration/multifd.c
> > +++ b/migration/multifd.c
> > @@ -428,8 +428,9 @@ static void multifd_send_set_error(Error *err)
> >  
> >      if (err) {
> >          MigrationState *s = migrate_get_current();
> > -        migrate_set_error(s, err);
> > -        error_free(err);
> > +
> > +        migrate_error_propagate(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(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(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(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(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..856366072a 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(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(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..1d2a47526e 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(err);
> >          migration_cancel();
> >      }
> >  
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 638e9b364f..ab9d1e579e 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(error_copy(local_err));
> >          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(error_copy(*errp));
> 
> Here, we need to keep the copy because we additionally propagate to the
> caller.
> 
> >                  qemu_file_set_error(f, ret);
> >                  break;
> >              }
> 
> [...]
> 

-- 
Peter Xu