Re: [RFC PATCH 24/25] migration: Move URI parsing to channel.c

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> 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 <farosas@suse.de>
>
> 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 <peterx@redhat.com>
>
>> ---
>>  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
>>