Re: [PATCH v3 20/25] migration: Move channel parsing to channel.c

[Fabiano Rosas <farosas@suse.de>]

Prasad Pandit <ppandit@redhat.com> writes:

> On Fri, 9 Jan 2026 at 18:14, Fabiano Rosas <farosas@suse.de> wrote:
>> Encapsulate the MigrationChannelList parsing in a new
>> migrate_channels_parse() located at channel.c.
>>
>> This also makes the memory management of the MigrationAddress more
>> uniform. Previously, half the parsing code (uri parsing) would
>> allocate memory for the address while the other half (channel parsing)
>> would instead pass the original QAPI object along. After this patch,
>> the MigrationAddress is always QAPI_CLONEd, so the callers can use
>> g_autoptr(MigrationAddress) in all cases.
>>
>> Reviewed-by: Peter Xu <peterx@redhat.com>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/channel.c   | 45 ++++++++++++++++++++++++++++++++++++++
>>  migration/channel.h   |  5 +++++
>>  migration/migration.c | 50 ++++++++++++-------------------------------
>>  3 files changed, 64 insertions(+), 36 deletions(-)
>>
>> diff --git a/migration/channel.c b/migration/channel.c
>> index 56c80b5cdf..8b71b3f430 100644
>> --- a/migration/channel.c
>> +++ b/migration/channel.c
>> @@ -11,6 +11,7 @@
>>   */
>>
>>  #include "qemu/osdep.h"
>> +#include "qemu/cutils.h"
>>  #include "channel.h"
>>  #include "exec.h"
>>  #include "fd.h"
>> @@ -20,7 +21,9 @@
>>  #include "migration.h"
>>  #include "multifd.h"
>>  #include "options.h"
>> +#include "qapi/clone-visitor.h"
>>  #include "qapi/qapi-types-migration.h"
>> +#include "qapi/qapi-visit-migration.h"
>>  #include "qapi/error.h"
>>  #include "qemu-file.h"
>>  #include "qemu/yank.h"
>> @@ -280,3 +283,45 @@ int migration_channel_read_peek(QIOChannel *ioc,
>>
>>      return 0;
>>  }
>> +
>> +bool migrate_channels_parse(MigrationChannelList *channels,
>> +                            MigrationChannel **main_channelp,
>> +                            MigrationChannel **cpr_channelp,
>> +                            Error **errp)
>> +{
>> +    MigrationChannel *channelv[MIGRATION_CHANNEL_TYPE__MAX] = { NULL };
>> +    bool single_channel = cpr_channelp ? false : true;
>> +
>> +    if (single_channel && channels->next) {
>> +        error_setg(errp, "Channel list must have only one entry, "
>> +                   "for type 'main'");
>> +        return false;
>> +    }
>
> * Instead of the single_channel variable above, we could say
> (!cpr_channelp && channels->next)? and avoid the single_channel
> variable.
>

ok

>> +    for ( ; channels; channels = channels->next) {
>> +        MigrationChannelType type;
>> +
>> +        type = channels->value->channel_type;
>> +        if (channelv[type]) {
>> +            error_setg(errp, "Channel list has more than one %s entry",
>> +                       MigrationChannelType_str(type));
>> +            return false;
>> +        }
>> +        channelv[type] = channels->value;
>> +    }
>> +
>> +    if (cpr_channelp) {
>> +        *cpr_channelp = QAPI_CLONE(MigrationChannel,
>> +                                   channelv[MIGRATION_CHANNEL_TYPE_CPR]);
>> +    }
>> +
>> +    *main_channelp = QAPI_CLONE(MigrationChannel,
>> +                                channelv[MIGRATION_CHANNEL_TYPE_MAIN]);
>> +
>> +    if (!(*main_channelp)->addr) {
>> +        error_setg(errp, "Channel list has no main entry");
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
>> diff --git a/migration/channel.h b/migration/channel.h
>> index 8264fe327d..5110fb45a4 100644
>> --- a/migration/channel.h
>> +++ b/migration/channel.h
>> @@ -42,4 +42,9 @@ bool migration_has_all_channels(void);
>>  void migration_connect_outgoing(MigrationState *s, MigrationAddress *addr,
>>                                  Error **errp);
>>  void migration_connect_incoming(MigrationAddress *addr, Error **errp);
>> +
>> +bool migrate_channels_parse(MigrationChannelList *channels,
>> +                            MigrationChannel **main_channelp,
>> +                            MigrationChannel **cpr_channelp,
>> +                            Error **errp);
>>  #endif
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 3c93fb23cc..98c1f38e8e 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -741,8 +741,7 @@ static void qemu_setup_incoming_migration(const char *uri, bool has_channels,
>>                                            MigrationChannelList *channels,
>>                                            Error **errp)
>>  {
>> -    g_autoptr(MigrationChannel) channel = NULL;
>> -    MigrationAddress *addr = NULL;
>> +    g_autoptr(MigrationChannel) main_ch = NULL;
>>      MigrationIncomingState *mis = migration_incoming_get_current();
>>
>>      /*
>> @@ -754,25 +753,20 @@ static void qemu_setup_incoming_migration(const char *uri, bool has_channels,
>>      }
>>
>>      if (channels) {
>> -        /* To verify that Migrate channel list has only item */
>> -        if (channels->next) {
>> -            error_setg(errp, "Channel list must have only one entry, "
>> -                             "for type 'main'");
>> +        if (!migrate_channels_parse(channels, &main_ch, NULL, errp)) {
>>              return;
>>          }
>> -        addr = channels->value->addr;
>>      }
>>
>>      if (uri) {
>>          /* caller uses the old URI syntax */
>> -        if (!migrate_uri_parse(uri, &channel, errp)) {
>> +        if (!migrate_uri_parse(uri, &main_ch, errp)) {
>>              return;
>>          }
>> -        addr = channel->addr;
>>      }
>>
>>      /* transport mechanism not suitable for migration? */
>> -    if (!migration_transport_compatible(addr, errp)) {
>> +    if (!migration_transport_compatible(main_ch->addr, errp)) {
>>          return;
>>      }
>>
>> @@ -780,7 +774,7 @@ static void qemu_setup_incoming_migration(const char *uri, bool has_channels,
>>          return;
>>      }
>>
>> -    migration_connect_incoming(addr, errp);
>> +    migration_connect_incoming(main_ch->addr, errp);
>>
>>      /* Close cpr socket to tell source that we are listening */
>>      cpr_state_close();
>> @@ -2116,10 +2110,8 @@ void qmp_migrate(const char *uri, bool has_channels,
>>                   bool has_resume, bool resume, Error **errp)
>>  {
>>      MigrationState *s = migrate_get_current();
>> -    g_autoptr(MigrationChannel) channel = NULL;
>> -    MigrationAddress *addr = NULL;
>> -    MigrationChannel *channelv[MIGRATION_CHANNEL_TYPE__MAX] = { NULL };
>> -    MigrationChannel *cpr_channel = NULL;
>> +    g_autoptr(MigrationChannel) main_ch = NULL;
>> +    g_autoptr(MigrationChannel) cpr_ch = NULL;
>>
>>      /*
>>       * Having preliminary checks for uri and channel
>> @@ -2130,38 +2122,24 @@ void qmp_migrate(const char *uri, bool has_channels,
>>      }
>>
>>      if (channels) {
>> -        for ( ; channels; channels = channels->next) {
>> -            MigrationChannelType type = channels->value->channel_type;
>> -
>> -            if (channelv[type]) {
>> -                error_setg(errp, "Channel list has more than one %s entry",
>> -                           MigrationChannelType_str(type));
>> -                return;
>> -            }
>> -            channelv[type] = channels->value;
>> -        }
>> -        cpr_channel = channelv[MIGRATION_CHANNEL_TYPE_CPR];
>> -        addr = channelv[MIGRATION_CHANNEL_TYPE_MAIN]->addr;
>> -        if (!addr) {
>> -            error_setg(errp, "Channel list has no main entry");
>> +        if (!migrate_channels_parse(channels, &main_ch, &cpr_ch, errp)) {
>>              return;
>>          }
>>      }
>>
>>      if (uri) {
>>          /* caller uses the old URI syntax */
>> -        if (!migrate_uri_parse(uri, &channel, errp)) {
>> +        if (!migrate_uri_parse(uri, &main_ch, errp)) {
>>              return;
>>          }
>> -        addr = channel->addr;
>>      }
>>
>>      /* transport mechanism not suitable for migration? */
>> -    if (!migration_transport_compatible(addr, errp)) {
>> +    if (!migration_transport_compatible(main_ch->addr, errp)) {
>>          return;
>>      }
>>
>> -    if (migrate_mode() == MIG_MODE_CPR_TRANSFER && !cpr_channel) {
>> +    if (migrate_mode() == MIG_MODE_CPR_TRANSFER && !cpr_ch) {
>>          error_setg(errp, "missing 'cpr' migration channel");
>>          return;
>>      }
>
> * This check for (_CPR_TRANSFER && !cpr_ch) and error could be moved
> to migrate_channels_parse() as is done for the main_ch.
>

Hm, it would then apply to the incoming side as well which can't set the
mode due to how cpr-transfer works. But I guess it's fine as the
invocation of migration_channel_parse_input is already different between
src and dst. I'll change it. Thanks

>> @@ -2178,7 +2156,7 @@ void qmp_migrate(const char *uri, bool has_channels,
>>       */
>>      Error *local_err = NULL;
>>
>> -    if (!cpr_state_save(cpr_channel, &local_err)) {
>> +    if (!cpr_state_save(cpr_ch, &local_err)) {
>>          goto out;
>>      }
>>
>> @@ -2194,10 +2172,10 @@ void qmp_migrate(const char *uri, bool has_channels,
>>       */
>>      if (migrate_mode() == MIG_MODE_CPR_TRANSFER) {
>>          migrate_hup_add(s, cpr_state_ioc(), (GSourceFunc)qmp_migrate_finish_cb,
>> -                        QAPI_CLONE(MigrationAddress, addr));
>> +                        QAPI_CLONE(MigrationAddress, main_ch->addr));
>>
>>      } else {
>> -        qmp_migrate_finish(addr, errp);
>> +        qmp_migrate_finish(main_ch->addr, errp);
>>      }
>>
>>  out:
>> --
>
> * Otherwise it looks okay.
> Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
>
> Thank you.
> ---
>   - Prasad