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.
> + 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.
> @@ -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
