[PATCH v3 10/25] migration: yank: Move register instance earlier

Fabiano Rosas posted 25 patches 1 month ago
Maintainers: Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>, Mark Kanda <mark.kanda@oracle.com>, Ben Chaney <bchaney@akamai.com>, Li Zhijian <lizhijian@fujitsu.com>
There is a newer version of this series
[PATCH v3 10/25] migration: yank: Move register instance earlier
Posted by Fabiano Rosas 1 month ago
Move the register_instance call to migrate_prepare() so it can be
paired with the unregister_instance at migration_cleanup(). Otherwise,
the cleanup cannot be run when cpr_state_save() fails because the
instance is registered only after it.

When resuming from a paused postcopy migration, migrate_prepare()
returns early, but migration_cleanup() doesn't run, so the yank will
remain paired.

Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 259b60af04..164cb26c48 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2180,11 +2180,12 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
         return false;
     }
 
+    yank_register_instance(MIGRATION_YANK_INSTANCE, &error_abort);
+
     return true;
 }
 
-static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
-                               Error **errp);
+static void qmp_migrate_finish(MigrationAddress *addr, Error **errp);
 
 static void migrate_hup_add(MigrationState *s, QIOChannel *ioc, GSourceFunc cb,
                             void *opaque)
@@ -2209,7 +2210,7 @@ static gboolean qmp_migrate_finish_cb(QIOChannel *channel,
 {
     MigrationAddress *addr = opaque;
 
-    qmp_migrate_finish(addr, false, NULL);
+    qmp_migrate_finish(addr, NULL);
 
     cpr_state_close();
     migrate_hup_delete(migrate_get_current());
@@ -2221,7 +2222,6 @@ void qmp_migrate(const char *uri, bool has_channels,
                  MigrationChannelList *channels, bool has_detach, bool detach,
                  bool has_resume, bool resume, Error **errp)
 {
-    bool resume_requested;
     Error *local_err = NULL;
     MigrationState *s = migrate_get_current();
     g_autoptr(MigrationChannel) channel = NULL;
@@ -2274,8 +2274,7 @@ void qmp_migrate(const char *uri, bool has_channels,
         return;
     }
 
-    resume_requested = has_resume && resume;
-    if (!migrate_prepare(s, resume_requested, errp)) {
+    if (!migrate_prepare(s, has_resume && resume, errp)) {
         /* Error detected, put into errp */
         return;
     }
@@ -2299,28 +2298,22 @@ void qmp_migrate(const char *uri, bool has_channels,
                         QAPI_CLONE(MigrationAddress, addr));
 
     } else {
-        qmp_migrate_finish(addr, resume_requested, errp);
+        qmp_migrate_finish(addr, errp);
     }
 
 out:
     if (local_err) {
+        yank_unregister_instance(MIGRATION_YANK_INSTANCE);
         migration_connect_error_propagate(s, error_copy(local_err));
         error_propagate(errp, local_err);
     }
 }
 
-static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
-                               Error **errp)
+static void qmp_migrate_finish(MigrationAddress *addr, Error **errp)
 {
     MigrationState *s = migrate_get_current();
     Error *local_err = NULL;
 
-    if (!resume_requested) {
-        if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) {
-            return;
-        }
-    }
-
     if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
         SocketAddress *saddr = &addr->u.socket;
         if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
@@ -2343,9 +2336,6 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
     }
 
     if (local_err) {
-        if (!resume_requested) {
-            yank_unregister_instance(MIGRATION_YANK_INSTANCE);
-        }
         migration_connect_error_propagate(s, error_copy(local_err));
         error_propagate(errp, local_err);
         return;
-- 
2.51.0
Re: [PATCH v3 10/25] migration: yank: Move register instance earlier
Posted by Prasad Pandit 2 weeks, 6 days ago
On Fri, 9 Jan 2026 at 18:13, Fabiano Rosas <farosas@suse.de> wrote:
> Move the register_instance call to migrate_prepare() so it can be
> paired with the unregister_instance at migration_cleanup(). Otherwise,
> the cleanup cannot be run when cpr_state_save() fails because the
> instance is registered only after it.
>
> When resuming from a paused postcopy migration, migrate_prepare()
> returns early, but migration_cleanup() doesn't run, so the yank will
> remain paired.
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/migration.c | 26 ++++++++------------------
>  1 file changed, 8 insertions(+), 18 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 259b60af04..164cb26c48 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2180,11 +2180,12 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
>          return false;
>      }
>
> +    yank_register_instance(MIGRATION_YANK_INSTANCE, &error_abort);
> +
>      return true;
>  }
>
> -static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
> -                               Error **errp);
> +static void qmp_migrate_finish(MigrationAddress *addr, Error **errp);
>
>  static void migrate_hup_add(MigrationState *s, QIOChannel *ioc, GSourceFunc cb,
>                              void *opaque)
> @@ -2209,7 +2210,7 @@ static gboolean qmp_migrate_finish_cb(QIOChannel *channel,
>  {
>      MigrationAddress *addr = opaque;
>
> -    qmp_migrate_finish(addr, false, NULL);
> +    qmp_migrate_finish(addr, NULL);
>
>      cpr_state_close();
>      migrate_hup_delete(migrate_get_current());
> @@ -2221,7 +2222,6 @@ void qmp_migrate(const char *uri, bool has_channels,
>                   MigrationChannelList *channels, bool has_detach, bool detach,
>                   bool has_resume, bool resume, Error **errp)
>  {
> -    bool resume_requested;
>      Error *local_err = NULL;
>      MigrationState *s = migrate_get_current();
>      g_autoptr(MigrationChannel) channel = NULL;
> @@ -2274,8 +2274,7 @@ void qmp_migrate(const char *uri, bool has_channels,
>          return;
>      }
>
> -    resume_requested = has_resume && resume;
> -    if (!migrate_prepare(s, resume_requested, errp)) {
> +    if (!migrate_prepare(s, has_resume && resume, errp)) {
>          /* Error detected, put into errp */
>          return;
>      }
> @@ -2299,28 +2298,22 @@ void qmp_migrate(const char *uri, bool has_channels,
>                          QAPI_CLONE(MigrationAddress, addr));
>
>      } else {
> -        qmp_migrate_finish(addr, resume_requested, errp);
> +        qmp_migrate_finish(addr, errp);
>      }
>
>  out:
>      if (local_err) {
> +        yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>          migration_connect_error_propagate(s, error_copy(local_err));
>          error_propagate(errp, local_err);
>      }
>  }
>
> -static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
> -                               Error **errp)
> +static void qmp_migrate_finish(MigrationAddress *addr, Error **errp)
>  {
>      MigrationState *s = migrate_get_current();
>      Error *local_err = NULL;
>
> -    if (!resume_requested) {
> -        if (!yank_register_instance(MIGRATION_YANK_INSTANCE, errp)) {
> -            return;
> -        }
> -    }
> -
>      if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
>          SocketAddress *saddr = &addr->u.socket;
>          if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
> @@ -2343,9 +2336,6 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
>      }
>
>      if (local_err) {
> -        if (!resume_requested) {
> -            yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> -        }
>          migration_connect_error_propagate(s, error_copy(local_err));
>          error_propagate(errp, local_err);
>          return;
> --

* Looks good.
Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>

Thank you.
---
  - Prasad