Re: [PATCH v2 01/13] migration: Propagate errors in migration_completion_precopy()

[Avihai Horon]

On 6/4/2026 6:21 PM, Peter Xu wrote:

External email: Use caution opening links or attachments


On Thu, Jun 04, 2026 at 06:12:11PM +0300, Avihai Horon wrote:

On 6/3/2026 10:47 PM, Peter Xu wrote:

External email: Use caution opening links or attachments


On Tue, Jun 02, 2026 at 12:26:09PM +0300, Avihai Horon wrote:

migration_completion_precopy() doesn't propagate errors to migration
core which leads to error information loss. Fix that.

This prepares for a follow-up where migration_switchover_start() can
fail on switchover-ack and still report a useful error. Errors from
qemu_savevm_state_complete_precopy() are not propagated yet as it
requires more plumbing.

Signed-off-by: Avihai Horon <avih...@nvidia.com>
---
   migration/migration.c | 11 +++++++----
   1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 074d3f2c69..7488a94206 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2814,7 +2814,7 @@ static bool migration_switchover_start(MigrationState *s, 
Error **errp)
       return true;
   }

-static int migration_completion_precopy(MigrationState *s)
+static int migration_completion_precopy(MigrationState *s, Error **errp)
   {
       int ret;

@@ -2823,11 +2823,12 @@ static int migration_completion_precopy(MigrationState 
*s)
       if (!migrate_mode_is_cpr()) {
           ret = migration_stop_vm(s, RUN_STATE_FINISH_MIGRATE);
           if (ret < 0) {
+            error_setg_errno(errp, -ret, "Failed to stop the VM");
               goto out_unlock;
           }
       }

-    if (!migration_switchover_start(s, NULL)) {
+    if (!migration_switchover_start(s, errp)) {
           ret = -EFAULT;
           goto out_unlock;
       }

IIUC this patch overlooked the follow up call:

      ret = qemu_savevm_state_complete_precopy(s);

We should make sure ret!=0 will always set Error*.  Better pass Error**
into qemu_savevm_state_complete_precopy() too.

Yes, that was intentional, because this requires more plumbing in
qemu_savevm_state_complete_precopy() (as mentioned in the commit message).
Although not clean, I thought it was OK to do this one-time exception since
we explicitly check for local_err in migration_completion().

I can give it a shot and try adding Error **errp to
qemu_savevm_state_complete_precopy(), but I think it'll be opening a can of
worms.
Do you have any thoughts?

If we want to keep the min change, we can consider setting errp at least
when qemu_savevm_state_complete_precopy() returned non-zero in the path of
migration_completion_precopy(), to make sure errp is always set when error
happened.  Otherwise it's error prone.

OK, I'll see how that works.

Thanks.

Thanks,

Thanks.

Thanks,

@@ -2869,7 +2870,7 @@ static void migration_completion(MigrationState *s)
       Error *local_err = NULL;

       if (s->state == MIGRATION_STATUS_ACTIVE) {
-        ret = migration_completion_precopy(s);
+        ret = migration_completion_precopy(s, &local_err);
       } else if (s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
           migration_completion_postcopy(s);
       } else {
@@ -2900,7 +2901,9 @@ static void migration_completion(MigrationState *s)
       return;

   fail:
-    if (qemu_file_get_error_obj(s->to_dst_file, &local_err)) {
+    if (local_err) {
+        migrate_error_propagate(s, local_err);
+    } else if (qemu_file_get_error_obj(s->to_dst_file, &local_err)) {
           migrate_error_propagate(s, local_err);
       } else if (ret) {
           error_setg_errno(&local_err, -ret, "Error in migration completion");
--
2.40.1

--
Peter Xu

--
Peter Xu