Prasad Pandit <[email protected]> writes:
> On Fri, 9 Jan 2026 at 18:14, Fabiano Rosas <[email protected]> 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 <[email protected]>
>> Signed-off-by: Fabiano Rosas <[email protected]>
>> ---
>> 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 <[email protected]>
>
> Thank you.
> ---
> - Prasad
