Re: [PATCH v2 25/25] migration/channel: Centralize calling migration_channel_connect_outgoing

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Mon, Jan 05, 2026 at 04:06:42PM -0300, Fabiano Rosas wrote:
>> Make the synchronous calls evident by not hiding the call to
>> migration_channel_connect_outgoing() in the transport code. Have those
>> functions return and call the function at the upper level.
>> 
>> This helps with navigation: the transport code returns the ioc,
>> there's no need to look into them when browsing the code.
>> 
>> It also allows RDMA in the source side to use the same path as the
>> rest of the transports.
>> 
>> While here, document the async calls which are the exception.
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
> One question inline.
>
>> ---
>>  migration/channel.c | 28 ++++++++++++++++++++++++----
>>  migration/exec.c    |  8 ++++----
>>  migration/exec.h    |  5 ++++-
>>  migration/fd.c      | 13 +++++++------
>>  migration/fd.h      |  7 +++++--
>>  migration/file.c    | 18 ++++++++++--------
>>  migration/file.h    |  5 +++--
>>  migration/rdma.c    | 11 +++++------
>>  migration/rdma.h    |  4 ++--
>>  9 files changed, 64 insertions(+), 35 deletions(-)
>> 
>> diff --git a/migration/channel.c b/migration/channel.c
>> index 6bb2077274..a8516837cf 100644
>> --- a/migration/channel.c
>> +++ b/migration/channel.c
>> @@ -37,26 +37,40 @@
>>  void migration_connect_outgoing(MigrationState *s, MigrationAddress *addr,
>>                                  Error **errp)
>>  {
>> +    g_autoptr(QIOChannel) ioc = NULL;
>> +
>>      if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
>>          SocketAddress *saddr = &addr->u.socket;
>>          if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
>>              saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
>>              saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
>>              socket_connect_outgoing(s, saddr, errp);
>> +            /*
>> +             * async: after the socket is connected, calls
>> +             * migration_channel_connect_outgoing() directly.
>> +             */
>> +            return;
>> +
>>          } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
>> -            fd_connect_outgoing(s, saddr->u.fd.str, errp);
>> +            ioc = fd_connect_outgoing(s, saddr->u.fd.str, errp);
>>          }
>>  #ifdef CONFIG_RDMA
>>      } else if (addr->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
>> -        rdma_connect_outgoing(s, &addr->u.rdma, errp);
>> +        ioc = rdma_connect_outgoing(s, &addr->u.rdma, errp);
>>  #endif
>>      } else if (addr->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
>> -        exec_connect_outgoing(s, addr->u.exec.args, errp);
>> +        ioc = exec_connect_outgoing(s, addr->u.exec.args, errp);
>>      } else if (addr->transport == MIGRATION_ADDRESS_TYPE_FILE) {
>> -        file_connect_outgoing(s, &addr->u.file, errp);
>> +        ioc = file_connect_outgoing(s, &addr->u.file, errp);
>>      } else {
>>          error_setg(errp, "uri is not a valid migration protocol");
>>      }
>> +
>> +    if (ioc) {
>> +        migration_channel_connect_outgoing(s, ioc);
>> +    }
>> +
>> +    return;
>>  }
>>  
>>  void migration_connect_incoming(MigrationAddress *addr, Error **errp)
>> @@ -81,6 +95,12 @@ void migration_connect_incoming(MigrationAddress *addr, Error **errp)
>>      } else {
>>          error_setg(errp, "unknown migration protocol");
>>      }
>> +
>> +    /*
>> +     * async: the above routines all wait for the incoming connection
>> +     * and call back to migration_channel_process_incoming() to start
>> +     * the migration.
>> +     */
>>  }
>>  
>>  bool migration_has_main_and_multifd_channels(void)
>> diff --git a/migration/exec.c b/migration/exec.c
>> index c3085e803e..a1a7ede3b4 100644
>> --- a/migration/exec.c
>> +++ b/migration/exec.c
>> @@ -40,7 +40,8 @@ const char *exec_get_cmd_path(void)
>>  }
>>  #endif
>>  
>> -void exec_connect_outgoing(MigrationState *s, strList *command, Error **errp)
>> +QIOChannel *exec_connect_outgoing(MigrationState *s, strList *command,
>> +                                  Error **errp)
>>  {
>>      QIOChannel *ioc = NULL;
>>      g_auto(GStrv) argv = strv_from_str_list(command);
>> @@ -50,12 +51,11 @@ void exec_connect_outgoing(MigrationState *s, strList *command, Error **errp)
>>      trace_migration_exec_outgoing(new_command);
>>      ioc = QIO_CHANNEL(qio_channel_command_new_spawn(args, O_RDWR, errp));
>>      if (!ioc) {
>> -        return;
>> +        return NULL;
>>      }
>>  
>>      qio_channel_set_name(ioc, "migration-exec-outgoing");
>> -    migration_channel_connect_outgoing(s, ioc);
>> -    object_unref(OBJECT(ioc));
>> +    return ioc;
>>  }
>>  
>>  static gboolean exec_accept_incoming_migration(QIOChannel *ioc,
>> diff --git a/migration/exec.h b/migration/exec.h
>> index e7e8e475ac..3e39270dce 100644
>> --- a/migration/exec.h
>> +++ b/migration/exec.h
>> @@ -20,10 +20,13 @@
>>  #ifndef QEMU_MIGRATION_EXEC_H
>>  #define QEMU_MIGRATION_EXEC_H
>>  
>> +#include "io/channel.h"
>> +
>>  #ifdef WIN32
>>  const char *exec_get_cmd_path(void);
>>  #endif
>>  void exec_connect_incoming(strList *host_port, Error **errp);
>>  
>> -void exec_connect_outgoing(MigrationState *s, strList *host_port, Error **errp);
>> +QIOChannel *exec_connect_outgoing(MigrationState *s, strList *host_port,
>> +                                  Error **errp);
>>  #endif
>> diff --git a/migration/fd.c b/migration/fd.c
>> index b689426ad4..bbf380d1a0 100644
>> --- a/migration/fd.c
>> +++ b/migration/fd.c
>> @@ -49,12 +49,13 @@ static bool migration_fd_valid(int fd)
>>      return false;
>>  }
>>  
>> -void fd_connect_outgoing(MigrationState *s, const char *fdname, Error **errp)
>> +QIOChannel *fd_connect_outgoing(MigrationState *s, const char *fdname,
>> +                                Error **errp)
>>  {
>> -    QIOChannel *ioc;
>> +    QIOChannel *ioc = NULL;
>>      int fd = monitor_get_fd(monitor_cur(), fdname, errp);
>>      if (fd == -1) {
>> -        return;
>> +        goto out;
>>      }
>>  
>>      if (!migration_fd_valid(fd)) {
>> @@ -66,12 +67,12 @@ void fd_connect_outgoing(MigrationState *s, const char *fdname, Error **errp)
>>      ioc = qio_channel_new_fd(fd, errp);
>>      if (!ioc) {
>>          close(fd);
>> -        return;
>> +        goto out;
>>      }
>>  
>>      qio_channel_set_name(ioc, "migration-fd-outgoing");
>> -    migration_channel_connect_outgoing(s, ioc);
>> -    object_unref(OBJECT(ioc));
>> +out:
>> +    return ioc;
>>  }
>>  
>>  static gboolean fd_accept_incoming_migration(QIOChannel *ioc,
>> diff --git a/migration/fd.h b/migration/fd.h
>> index 7211629270..ce0b751273 100644
>> --- a/migration/fd.h
>> +++ b/migration/fd.h
>> @@ -16,8 +16,11 @@
>>  
>>  #ifndef QEMU_MIGRATION_FD_H
>>  #define QEMU_MIGRATION_FD_H
>> +
>> +#include "io/channel.h"
>> +
>>  void fd_connect_incoming(const char *fdname, Error **errp);
>>  
>> -void fd_connect_outgoing(MigrationState *s, const char *fdname,
>> -                         Error **errp);
>> +QIOChannel *fd_connect_outgoing(MigrationState *s, const char *fdname,
>> +                                Error **errp);
>>  #endif
>> diff --git a/migration/file.c b/migration/file.c
>> index b7b0fb5194..5618aced49 100644
>> --- a/migration/file.c
>> +++ b/migration/file.c
>> @@ -93,36 +93,38 @@ out:
>>      return ret;
>>  }
>>  
>> -void file_connect_outgoing(MigrationState *s,
>> -                           FileMigrationArgs *file_args, Error **errp)
>> +QIOChannel *file_connect_outgoing(MigrationState *s,
>> +                                  FileMigrationArgs *file_args, Error **errp)
>>  {
>> -    g_autoptr(QIOChannelFile) fioc = NULL;
>> +    QIOChannelFile *fioc = NULL;
>>      g_autofree char *filename = g_strdup(file_args->filename);
>>      uint64_t offset = file_args->offset;
>> -    QIOChannel *ioc;
>> +    QIOChannel *ioc = NULL;
>>  
>>      trace_migration_file_outgoing(filename);
>>  
>>      fioc = qio_channel_file_new_path(filename, O_CREAT | O_WRONLY, 0600, errp);
>>      if (!fioc) {
>> -        return;
>> +        goto out;
>>      }
>>  
>>      if (ftruncate(fioc->fd, offset)) {
>>          error_setg_errno(errp, errno,
>>                           "failed to truncate migration file to offset %" PRIx64,
>>                           offset);
>> -        return;
>> +        goto out;
>>      }
>>  
>>      outgoing_args.fname = g_strdup(filename);
>>  
>>      ioc = QIO_CHANNEL(fioc);
>>      if (offset && qio_channel_io_seek(ioc, offset, SEEK_SET, errp) < 0) {
>> -        return;
>> +        ioc = NULL;
>> +        goto out;
>>      }
>>      qio_channel_set_name(ioc, "migration-file-outgoing");
>> -    migration_channel_connect_outgoing(s, ioc);
>> +out:
>> +    return ioc;
>>  }
>>  
>>  static gboolean file_accept_incoming_migration(QIOChannel *ioc,
>> diff --git a/migration/file.h b/migration/file.h
>> index 9b1e874bb7..5936c64fea 100644
>> --- a/migration/file.h
>> +++ b/migration/file.h
>> @@ -9,14 +9,15 @@
>>  #define QEMU_MIGRATION_FILE_H
>>  
>>  #include "qapi/qapi-types-migration.h"
>> +#include "io/channel.h"
>>  #include "io/task.h"
>>  #include "channel.h"
>>  #include "multifd.h"
>>  
>>  void file_connect_incoming(FileMigrationArgs *file_args, Error **errp);
>>  
>> -void file_connect_outgoing(MigrationState *s,
>> -                           FileMigrationArgs *file_args, Error **errp);
>> +QIOChannel *file_connect_outgoing(MigrationState *s,
>> +                                  FileMigrationArgs *file_args, Error **errp);
>>  int file_parse_offset(char *filespec, uint64_t *offsetp, Error **errp);
>>  void file_cleanup_outgoing_migration(void);
>>  bool file_send_channel_create(gpointer opaque, Error **errp);
>> diff --git a/migration/rdma.c b/migration/rdma.c
>> index 582e0651d4..66bc337b6b 100644
>> --- a/migration/rdma.c
>> +++ b/migration/rdma.c
>> @@ -3934,8 +3934,8 @@ err:
>>      g_free(rdma);
>>  }
>>  
>> -void rdma_connect_outgoing(void *opaque,
>> -                           InetSocketAddress *host_port, Error **errp)
>> +QIOChannel *rdma_connect_outgoing(void *opaque,
>> +                                  InetSocketAddress *host_port, Error **errp)
>>  {
>>      MigrationState *s = opaque;
>>      RDMAContext *rdma_return_path = NULL;
>> @@ -3945,7 +3945,7 @@ void rdma_connect_outgoing(void *opaque,
>>      /* Avoid ram_block_discard_disable(), cannot change during migration. */
>>      if (ram_block_discard_is_required()) {
>>          error_setg(errp, "RDMA: cannot disable RAM discard");
>> -        return;
>> +        return NULL;
>>      }
>>  
>>      rdma = qemu_rdma_data_init(host_port, errp);
>> @@ -3995,12 +3995,11 @@ void rdma_connect_outgoing(void *opaque,
>>      trace_rdma_connect_outgoing_after_rdma_connect();
>>  
>>      s->rdma_migration = true;
>> -    migration_outgoing_setup(rdma_new_output(rdma));
>> -    migration_start_outgoing(s);
>
> migration_channel_connect_outgoing() does two more things:
>
> - check for migrate_channel_requires_tls_upgrade()
> - migration_ioc_register_yank()
>
> The latter is probably fine because rdma iochannel doesn't support
> .shutdown() API.

Hm, let me make sure this doesn't blow up when calling yank.

>
> The former... may not be relevant to this patch, but anyway I wonder if
> it'll always be better to fail a QMP migrate command when RDMA is used with
> TLS, in migration_capabilities_and_transport_compatible().  It can be a
> separate small patch rather than reposting this wholeset.
>

Right, we even discussed this on IRC. I don't remember now, but I hit
some issue because TLS is not really a capability. I'll take another
look. Thanks!

>> -    return;
>> +    return rdma_new_output(rdma);
>>  return_path_err:
>>      qemu_rdma_cleanup(rdma);
>>  err:
>>      g_free(rdma);
>>      g_free(rdma_return_path);
>> +    return NULL;
>>  }
>> diff --git a/migration/rdma.h b/migration/rdma.h
>> index 170c25cf44..8a6515f130 100644
>> --- a/migration/rdma.h
>> +++ b/migration/rdma.h
>> @@ -21,8 +21,8 @@
>>  
>>  #include "system/memory.h"
>>  
>> -void rdma_connect_outgoing(void *opaque, InetSocketAddress *host_port,
>> -                           Error **errp);
>> +QIOChannel *rdma_connect_outgoing(void *opaque, InetSocketAddress *host_port,
>> +                                  Error **errp);
>>  
>>  void rdma_connect_incoming(InetSocketAddress *host_port, Error **errp);
>>  
>> -- 
>> 2.51.0
>>