Re: [PATCH] Improve error propagation via return path

[Vladimir Sementsov-Ogievskiy]

On 21.10.25 17:34, Peter Xu wrote:

On Tue, Oct 21, 2025 at 07:52:53AM +0000, Dhruv Choudhary wrote:

Use the return-path thread to send error details from the
destination to the source on a migration failure. Management
applications can then query the source QEMU for errors, as
the single source of truth, making failures easy to trace.

Signed-off-by: Dhruv Choudhary <dhruv.choudh...@nutanix.com>

+Vladimir, Dan

IIUC we may still need to know whether the src QEMU supports this message
or not.

OTOH, we have introduced exit-on-error since 9.1:

# @exit-on-error: Exit on incoming migration failure.  Default true.
#     When set to false, the failure triggers a :qapi:event:`MIGRATION`
#     event, and error details could be retrieved with `query-migrate`.
#     (since 9.1)

This patch is going the other way.  That feature suggests the mgmt query
the error from dest directly.

We should stick with one plan rather than doing both.

Why?

exit-on-error=false is good anyway: when QMP connection is established, the
management of target QEMU process is the same: we do call qmp commands to
add devices, etc. We get QMP events. Actually, exiting is unexpected, better
to fit into QMP protocol, continuing to send events and wait for qmp quit
to exit.

Passing error back to the source simply improves error message on source,
which otherwise is often confusing.

Using both, we of course see same error in two places.. But we do have two
QEMU processes, which both handled by on-host managing services. We should
correctly report error on both parts anyway.

Improving error messages on source is just and improvement, which makes
current behavior better (with or without exit-on-error=false).

Removing exit-on-error=false semantics (with or without passing errors back)
would be a step backward, to violating of QMP protocol by unexpected exits.

---
  migration/migration.c | 25 +++++++++++++++++++++++--
  1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index a63b46bbef..123cffb286 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -87,6 +87,7 @@ enum mig_rp_message_type {
      MIG_RP_MSG_RECV_BITMAP,  /* send recved_bitmap back to source */
      MIG_RP_MSG_RESUME_ACK,   /* tell source that we are ready to resume */
      MIG_RP_MSG_SWITCHOVER_ACK, /* Tell source it's OK to do switchover */
+    MIG_RP_MSG_ERROR,        /* propogate error to source */

MIG_RP_MSG_MAX

  };
@@ -608,6 +609,17 @@ int migrate_send_rp_req_pages(MigrationIncomingState *mis,
      return migrate_send_rp_message_req_pages(mis, rb, start);
  }

+static void migrate_send_rp_error(MigrationIncomingState *mis, Error *errp)

+{
+    const char *rpmsg = error_get_pretty(errp);
+    if (!mis->to_src_file) {
+        mis->to_src_file = qemu_file_get_return_path(mis->from_src_file);
+    }
+    migrate_send_rp_message(mis, MIG_RP_MSG_ERROR,
+                            (uint16_t)(strlen(rpmsg) + 1),
+                            (char *)rpmsg);
+}
+
  static bool migration_colo_enabled;
  bool migration_incoming_colo_enabled(void)
  {
@@ -905,8 +917,12 @@ process_incoming_migration_co(void *opaque)
      }

if (ret < 0) {

-        error_prepend(&local_err, "load of migration failed: %s: ",
-                      strerror(-ret));
+        error_prepend(&local_err, "destination error : load of migration 
failed:
+                       %s: ", strerror(-ret));
+        /* Check if return path is enabled and then send error to source */
+        if (migrate_postcopy_ram() || migrate_return_path()) {
+            migrate_send_rp_error(mis, local_err);
+        }
          goto fail;
      }

@@ -2437,6 +2453,7 @@ static struct rp_cmd_args {

      [MIG_RP_MSG_RECV_BITMAP]    = { .len = -1, .name = "RECV_BITMAP" },
      [MIG_RP_MSG_RESUME_ACK]     = { .len =  4, .name = "RESUME_ACK" },
      [MIG_RP_MSG_SWITCHOVER_ACK] = { .len =  0, .name = "SWITCHOVER_ACK" },
+    [MIG_RP_MSG_ERROR]          = { .len = -1, .name = "ERROR"},
      [MIG_RP_MSG_MAX]            = { .len = -1, .name = "MAX" },
  };

@@ -2667,6 +2684,10 @@ static void *source_return_path_thread(void *opaque)

              trace_source_return_path_thread_switchover_acked();
              break;

+ case MIG_RP_MSG_ERROR:

+            error_setg(&err, "%s", (char *)buf);
+            goto out;
+
          default:
              break;
          }
--
2.39.3

--
Best regards,
Vladimir