Peter Xu <[email protected]> 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 <[email protected]> > > Reviewed-by: Peter Xu <[email protected]> > > 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 >>
