Peter Xu <[email protected]> writes: > On Fri, Dec 26, 2025 at 06:19:26PM -0300, Fabiano Rosas wrote: >> The migrate_uri_parse function is responsible for converting the URI >> string into a MigrationChannel for consumption by the rest of the >> code. Move it to channel.c and add a wrapper that calls both URI and >> channels parsing. >> >> While here, add some words about the memory management of the >> MigrationChannel object, which is slightly different from >> migrate_uri_parse() and migrate_channels_parse(). The former takes a >> string and has to allocate a MigrationChannel, the latter takes a QAPI >> object that is managed by the QAPI code. >> >> Signed-off-by: Fabiano Rosas <[email protected]> > > There're some comments added into migration_connect(), that I still don't > think I fully agree upon.. but the rest looks all good to me. >
You're talking about the comments not being at the right place? I can duplicate them in migration_connect_outgoing|incoming. > That's probably to be discussed separately anyway, I think you can take > mine here: > > Reviewed-by: Peter Xu <[email protected]> > >> --- >> migration/channel.c | 97 +++++++++++++++++++++++++++++++++++++++++-- >> migration/channel.h | 9 ++-- >> migration/migration.c | 95 ++---------------------------------------- >> 3 files changed, 101 insertions(+), 100 deletions(-) >> >> diff --git a/migration/channel.c b/migration/channel.c >> index 8b43c3d983..d30e29c9b3 100644 >> --- a/migration/channel.c >> +++ b/migration/channel.c >> @@ -40,6 +40,12 @@ bool migration_connect(MigrationAddress *addr, bool out, >> Error **errp) >> SocketAddress *saddr; >> ERRP_GUARD(); >> >> + /* >> + * This is reached from a QMP command, the transport code below >> + * must copy the relevant parts of 'addr' before this function >> + * returns because the QAPI code will free it. >> + */ >> + >> switch (addr->transport) { >> case MIGRATION_ADDRESS_TYPE_SOCKET: >> saddr = &addr->u.socket; >> @@ -318,10 +324,10 @@ int migration_channel_read_peek(QIOChannel *ioc, >> return 0; >> } >> >> -bool migrate_channels_parse(MigrationChannelList *channels, >> - MigrationChannel **main_channelp, >> - MigrationChannel **cpr_channelp, >> - Error **errp) >> +static bool migrate_channels_parse(MigrationChannelList *channels, >> + MigrationChannel **main_channelp, >> + MigrationChannel **cpr_channelp, >> + Error **errp) >> { >> MigrationChannel *channelv[MIGRATION_CHANNEL_TYPE__MAX] = { NULL }; >> bool single_channel; >> @@ -353,6 +359,15 @@ bool migrate_channels_parse(MigrationChannelList >> *channels, >> } >> } >> >> + /* >> + * These don't technically need to be cloned because they come >> + * from a QAPI object ('channels'). The top-level caller >> + * (qmp_migrate) needs to copy the necessary information before >> + * returning from the QMP command. Cloning here is just to keep >> + * the interface consistent with migrate_uri_parse() that _does >> + * not_ take a QAPI object and instead allocates and transfers >> + * ownership of a MigrationChannel to qmp_migrate. >> + */ >> if (cpr_channelp) { >> *cpr_channelp = QAPI_CLONE(MigrationChannel, >> channelv[MIGRATION_CHANNEL_TYPE_CPR]); >> @@ -368,3 +383,77 @@ bool migrate_channels_parse(MigrationChannelList >> *channels, >> >> return true; >> } >> + >> +bool migrate_uri_parse(const char *uri, MigrationChannel **channel, >> + Error **errp) >> +{ >> + g_autoptr(MigrationChannel) val = g_new0(MigrationChannel, 1); >> + g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1); >> + InetSocketAddress *isock = &addr->u.rdma; >> + strList **tail = &addr->u.exec.args; >> + >> + if (strstart(uri, "exec:", NULL)) { >> + addr->transport = MIGRATION_ADDRESS_TYPE_EXEC; >> +#ifdef WIN32 >> + QAPI_LIST_APPEND(tail, g_strdup(exec_get_cmd_path())); >> + QAPI_LIST_APPEND(tail, g_strdup("/c")); >> +#else >> + QAPI_LIST_APPEND(tail, g_strdup("/bin/sh")); >> + QAPI_LIST_APPEND(tail, g_strdup("-c")); >> +#endif >> + QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:"))); >> + } else if (strstart(uri, "rdma:", NULL)) { >> + if (inet_parse(isock, uri + strlen("rdma:"), errp)) { >> + qapi_free_InetSocketAddress(isock); >> + return false; >> + } >> + addr->transport = MIGRATION_ADDRESS_TYPE_RDMA; >> + } else if (strstart(uri, "tcp:", NULL) || >> + strstart(uri, "unix:", NULL) || >> + strstart(uri, "vsock:", NULL) || >> + strstart(uri, "fd:", NULL)) { >> + addr->transport = MIGRATION_ADDRESS_TYPE_SOCKET; >> + SocketAddress *saddr = socket_parse(uri, errp); >> + if (!saddr) { >> + return false; >> + } >> + addr->u.socket.type = saddr->type; >> + addr->u.socket.u = saddr->u; >> + /* Don't free the objects inside; their ownership moved to "addr" */ >> + g_free(saddr); >> + } else if (strstart(uri, "file:", NULL)) { >> + addr->transport = MIGRATION_ADDRESS_TYPE_FILE; >> + addr->u.file.filename = g_strdup(uri + strlen("file:")); >> + if (file_parse_offset(addr->u.file.filename, &addr->u.file.offset, >> + errp)) { >> + return false; >> + } >> + } else { >> + error_setg(errp, "unknown migration protocol: %s", uri); >> + return false; >> + } >> + >> + val->channel_type = MIGRATION_CHANNEL_TYPE_MAIN; >> + val->addr = g_steal_pointer(&addr); >> + *channel = g_steal_pointer(&val); >> + return true; >> +} >> + >> +bool migration_channel_parse_input(const char *uri, >> + MigrationChannelList *channels, >> + MigrationChannel **main_channelp, >> + MigrationChannel **cpr_channelp, >> + Error **errp) >> +{ >> + if (!uri == !channels) { >> + error_setg(errp, "need either 'uri' or 'channels' argument"); >> + return false; >> + } >> + >> + if (channels) { >> + return migrate_channels_parse(channels, main_channelp, cpr_channelp, >> + errp); >> + } else { >> + return migrate_uri_parse(uri, main_channelp, errp); >> + } >> +} >> diff --git a/migration/channel.h b/migration/channel.h >> index b3276550b7..3724b0493a 100644 >> --- a/migration/channel.h >> +++ b/migration/channel.h >> @@ -52,8 +52,9 @@ static inline bool >> migration_connect_incoming(MigrationAddress *addr, >> return migration_connect(addr, false, errp); >> } >> >> -bool migrate_channels_parse(MigrationChannelList *channels, >> - MigrationChannel **main_channelp, >> - MigrationChannel **cpr_channelp, >> - Error **errp); >> +bool migration_channel_parse_input(const char *uri, >> + MigrationChannelList *channels, >> + MigrationChannel **main_channelp, >> + MigrationChannel **cpr_channelp, >> + Error **errp); >> #endif >> diff --git a/migration/migration.c b/migration/migration.c >> index 6064f1e5ea..15d8459a81 100644 >> --- a/migration/migration.c >> +++ b/migration/migration.c >> @@ -15,7 +15,6 @@ >> >> #include "qemu/osdep.h" >> #include "qemu/ctype.h" >> -#include "qemu/cutils.h" >> #include "qemu/error-report.h" >> #include "qemu/main-loop.h" >> #include "migration/blocker.h" >> @@ -659,61 +658,6 @@ bool migrate_is_uri(const char *uri) >> return *uri == ':'; >> } >> >> -bool migrate_uri_parse(const char *uri, MigrationChannel **channel, >> - Error **errp) >> -{ >> - g_autoptr(MigrationChannel) val = g_new0(MigrationChannel, 1); >> - g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1); >> - InetSocketAddress *isock = &addr->u.rdma; >> - strList **tail = &addr->u.exec.args; >> - >> - if (strstart(uri, "exec:", NULL)) { >> - addr->transport = MIGRATION_ADDRESS_TYPE_EXEC; >> -#ifdef WIN32 >> - QAPI_LIST_APPEND(tail, g_strdup(exec_get_cmd_path())); >> - QAPI_LIST_APPEND(tail, g_strdup("/c")); >> -#else >> - QAPI_LIST_APPEND(tail, g_strdup("/bin/sh")); >> - QAPI_LIST_APPEND(tail, g_strdup("-c")); >> -#endif >> - QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:"))); >> - } else if (strstart(uri, "rdma:", NULL)) { >> - if (inet_parse(isock, uri + strlen("rdma:"), errp)) { >> - qapi_free_InetSocketAddress(isock); >> - return false; >> - } >> - addr->transport = MIGRATION_ADDRESS_TYPE_RDMA; >> - } else if (strstart(uri, "tcp:", NULL) || >> - strstart(uri, "unix:", NULL) || >> - strstart(uri, "vsock:", NULL) || >> - strstart(uri, "fd:", NULL)) { >> - addr->transport = MIGRATION_ADDRESS_TYPE_SOCKET; >> - SocketAddress *saddr = socket_parse(uri, errp); >> - if (!saddr) { >> - return false; >> - } >> - addr->u.socket.type = saddr->type; >> - addr->u.socket.u = saddr->u; >> - /* Don't free the objects inside; their ownership moved to "addr" */ >> - g_free(saddr); >> - } else if (strstart(uri, "file:", NULL)) { >> - addr->transport = MIGRATION_ADDRESS_TYPE_FILE; >> - addr->u.file.filename = g_strdup(uri + strlen("file:")); >> - if (file_parse_offset(addr->u.file.filename, &addr->u.file.offset, >> - errp)) { >> - return false; >> - } >> - } else { >> - error_setg(errp, "unknown migration protocol: %s", uri); >> - return false; >> - } >> - >> - val->channel_type = MIGRATION_CHANNEL_TYPE_MAIN; >> - val->addr = g_steal_pointer(&addr); >> - *channel = g_steal_pointer(&val); >> - return true; >> -} >> - >> static bool >> migration_incoming_state_setup(MigrationIncomingState *mis, Error **errp) >> { >> @@ -744,27 +688,10 @@ static void qemu_setup_incoming_migration(const char >> *uri, bool has_channels, >> g_autoptr(MigrationChannel) main_ch = NULL; >> MigrationIncomingState *mis = migration_incoming_get_current(); >> >> - /* >> - * Having preliminary checks for uri and channel >> - */ >> - if (!uri == !channels) { >> - error_setg(errp, "need either 'uri' or 'channels' argument"); >> + if (!migration_channel_parse_input(uri, channels, &main_ch, NULL, >> errp)) { >> return; >> } >> >> - if (channels) { >> - if (!migrate_channels_parse(channels, &main_ch, NULL, errp)) { >> - return; >> - } >> - } >> - >> - if (uri) { >> - /* caller uses the old URI syntax */ >> - if (!migrate_uri_parse(uri, &main_ch, errp)) { >> - return; >> - } >> - } >> - >> /* transport mechanism not suitable for migration? */ >> if (!migration_transport_compatible(main_ch->addr, errp)) { >> return; >> @@ -2124,27 +2051,11 @@ void qmp_migrate(const char *uri, bool has_channels, >> g_autoptr(MigrationChannel) main_ch = NULL; >> g_autoptr(MigrationChannel) cpr_ch = NULL; >> >> - /* >> - * Having preliminary checks for uri and channel >> - */ >> - if (!uri == !channels) { >> - error_setg(errp, "need either 'uri' or 'channels' argument"); >> + if (!migration_channel_parse_input(uri, channels, &main_ch, &cpr_ch, >> + errp)) { >> return; >> } >> >> - if (channels) { >> - if (!migrate_channels_parse(channels, &main_ch, &cpr_ch, errp)) { >> - return; >> - } >> - } >> - >> - if (uri) { >> - /* caller uses the old URI syntax */ >> - if (!migrate_uri_parse(uri, &main_ch, errp)) { >> - return; >> - } >> - } >> - >> /* transport mechanism not suitable for migration? */ >> if (!migration_transport_compatible(main_ch->addr, errp)) { >> return; >> -- >> 2.51.0 >>
