Re: [PATCH v2 24/25] migration: Remove qmp_migrate_finish

[Peter Xu <peterx@redhat.com>]

On Mon, Jan 05, 2026 at 04:06:41PM -0300, Fabiano Rosas wrote:
> After cleanups, the qmp_migrate_finish function is now just a call to
> migration_connect_outgoing(). Remove qmp_migrate_finish() and rename
> the qmp_migrate_finish_cb callback.
> 
> This also allows the cleanup at qmp_migrate_finish() to move to the
> top level along with everyting else.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Reviewed-by: Peter Xu <peterx@redhat.com>

One nitpick:

> ---
>  migration/migration.c | 35 +++++++++++++----------------------
>  1 file changed, 13 insertions(+), 22 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index a9d5f5880d..2991e01d65 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1997,15 +1997,18 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
>      return true;
>  }
>  
> -static void qmp_migrate_finish(MigrationAddress *addr, Error **errp);
> -
> -static gboolean qmp_migrate_finish_cb(QIOChannel *channel,
> -                                      GIOCondition cond,
> -                                      void *opaque)
> +static gboolean migration_connect_outgoing_cb(QIOChannel *channel,
> +                                              GIOCondition cond, void *opaque)
>  {
> -    MigrationAddress *addr = opaque;
> +    MigrationState *s = migrate_get_current();
> +    Error *local_err = NULL;
> +
> +    migration_connect_outgoing(s, opaque, &local_err);
> +
> +    if (local_err) {
> +        migration_connect_error_propagate(s, local_err);
> +    }
>  
> -    qmp_migrate_finish(addr, NULL);
>      return G_SOURCE_REMOVE;
>  }
>  
> @@ -2059,10 +2062,11 @@ void qmp_migrate(const char *uri, bool has_channels,
>       * connection, so qmp_migrate_finish will fail to connect, and then recover.
>       */
>      if (migrate_mode() == MIG_MODE_CPR_TRANSFER) {
> -        cpr_transfer_add_hup_watch(s, qmp_migrate_finish_cb, main_ch->addr);
> +        cpr_transfer_add_hup_watch(s, migration_connect_outgoing_cb,
> +                                   main_ch->addr);
>  
>      } else {
> -        qmp_migrate_finish(main_ch->addr, errp);
> +        migration_connect_outgoing(s, main_ch->addr, &local_err);

I definitely overlooked that here it used &local_err to replace errp when
looking at this patch the 1st time.  You did mention it in the commit
message on "move to the top level", good enough!  But maybe even better if
spelling it out on the error object being passed in the commit message too.

>      }
>  
>  out:
> @@ -2072,19 +2076,6 @@ out:
>      }
>  }
>  
> -static void qmp_migrate_finish(MigrationAddress *addr, Error **errp)
> -{
> -    MigrationState *s = migrate_get_current();
> -    Error *local_err = NULL;
> -
> -    migration_connect_outgoing(s, addr, &local_err);
> -
> -    if (local_err) {
> -        migration_connect_error_propagate(s, error_copy(local_err));
> -        error_propagate(errp, local_err);
> -    }
> -}
> -
>  void qmp_migrate_cancel(Error **errp)
>  {
>      /*
> -- 
> 2.51.0
> 

-- 
Peter Xu