Re: [PATCH v9 08/10] migration: Implement MigrateChannelList to qmp migration flow.

[Het Gala <het.gala@nutanix.com>]

On 26/07/23 12:08 am, Daniel P. Berrangé wrote:
> On Fri, Jul 21, 2023 at 02:49:34PM +0000, Het Gala wrote:
>> Integrate MigrateChannelList with all transport backends
>> (socket, exec and rdma) for both src and dest migration
>> endpoints for qmp migration.
>>
>> For current series, limit the size of MigrateChannelList
>> to single element (single interface) as runtime check.
>>
>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>> ---
>>   migration/migration.c | 77 ++++++++++++++++++++++++++++---------------
>>   1 file changed, 50 insertions(+), 27 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 6e0a8beaf2..acf80b3590 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -426,9 +426,10 @@ void migrate_add_address(SocketAddress *address)
>>   }
>>   
>>   static bool migrate_uri_parse(const char *uri,
>> -                              MigrationAddress **channel,
>> +                              MigrationChannel **channel,
>>                                 Error **errp)
>>   {
>> +    g_autoptr(MigrationChannel) val = g_new0(MigrationChannel, 1);
>>       g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
>>       SocketAddress *saddr = &addr->u.socket;
>>       InetSocketAddress *isock = &addr->u.rdma;
>> @@ -465,7 +466,9 @@ static bool migrate_uri_parse(const char *uri,
>>           return false;
>>       }
>>   
>> -    *channel = addr;
>> +    val->channel_type = MIGRATION_CHANNEL_TYPE_MAIN;
>> +    val->addr = addr;
>> +    *channel = val;
>>       return true;
>>   }
>>   
>> @@ -473,7 +476,8 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
>>                                             MigrationChannelList *channels,
>>                                             Error **errp)
>>   {
>> -    g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
>> +    g_autoptr(MigrationChannel) channel = g_new0(MigrationChannel, 1);
>> +    g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
>>   
>>       /*
>>        * Having preliminary checks for uri and channel
>> @@ -483,20 +487,29 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
>>                      "exclusive; exactly one of the two should be present in "
>>                      "'migrate-incoming' qmp command ");
>>           return;
>> +    } else if (channels) {
>> +        /* To verify that Migrate channel list has only item */
>> +        if (channels->next) {
>> +            error_setg(errp, "Channel list has more than one entries");
>> +            return;
>> +        }
>> +        channel = channels->value;
>> +    } else {
>> +        /* caller uses the old URI syntax */
>> +        if (uri && !migrate_uri_parse(uri, &channel, errp)) {
>> +            return;
>> +        }
>>       }
>> -
>> -    if (uri && !migrate_uri_parse(uri, &channel, errp)) {
>> -        return;
>> -    }
>> +    addr = channel->addr;
>>   
>>       /* transport mechanism not suitable for migration? */
>> -    if (!migration_channels_and_transport_compatible(channel, errp)) {
>> +    if (!migration_channels_and_transport_compatible(addr, errp)) {
>>           return;
>>       }
>>   
>>       qapi_event_send_migration(MIGRATION_STATUS_SETUP);
>> -    if (channel->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
>> -        SocketAddress *saddr = &channel->u.socket;
>> +    if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
>> +        SocketAddress *saddr = &addr->u.socket;
>>           if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
>>               saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
>>               saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
>> @@ -505,11 +518,11 @@ static void qemu_start_incoming_migration(const char *uri, bool has_channels,
>>               fd_start_incoming_migration(saddr->u.fd.str, errp);
>>           }
>>   #ifdef CONFIG_RDMA
>> -    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
>> -        rdma_start_incoming_migration(&channel->u.rdma, errp);
>> -#endif
>> -    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
>> -        exec_start_incoming_migration(channel->u.exec.args, errp);
>> +    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
>> +        rdma_start_incoming_migration(&addr->u.rdma, errp);
>> + #endif
>> +    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
>> +        exec_start_incoming_migration(addr->u.exec.args, errp);
>>       } else {
>>           error_setg(errp, "unknown migration protocol: %s", uri);
>>       }
>> @@ -1709,7 +1722,8 @@ void qmp_migrate(const char *uri, bool has_channels,
>>       bool resume_requested;
>>       Error *local_err = NULL;
>>       MigrationState *s = migrate_get_current();
>> -    g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
>> +    g_autoptr(MigrationChannel) channel = g_new0(MigrationChannel, 1);
>> +    g_autoptr(MigrationAddress) addr = g_new0(MigrationAddress, 1);
>>   
>>       /*
>>        * Having preliminary checks for uri and channel
>> @@ -1719,14 +1733,23 @@ void qmp_migrate(const char *uri, bool has_channels,
>>                      "exclusive; exactly one of the two should be present in "
>>                      "'migrate' qmp command ");
>>           return;
>> +    } else if (channels) {
>> +        /* To verify that Migrate channel list has only item */
>> +        if (channels->next) {
>> +            error_setg(errp, "Channel list has more than one entries");
>> +            return;
>> +        }
>> +        channel = channels->value;
>> +    } else {
> } else if (uri) {
>       if (!migrate_uri_parse(....))
> } else {
>      ...error - neither uri or has_channels ...
>
> }
Ack. Will add this here.
>> +        /* caller uses the old URI syntax */
>> +        if (uri && !migrate_uri_parse(uri, &channel, errp)) {
>> +            return;
>> +        }
>>       }
>> -
>> -    if (!migrate_uri_parse(uri, &channel, errp)) {
>> -        return;
>> -    }
>> +    addr = channel->addr;
>>   
>>       /* transport mechanism not suitable for migration? */
>> -    if (!migration_channels_and_transport_compatible(channel, errp)) {
>> +    if (!migration_channels_and_transport_compatible(addr, errp)) {
>>           return;
>>       }
>>   
>> @@ -1743,8 +1766,8 @@ void qmp_migrate(const char *uri, bool has_channels,
>>           }
>>       }
>>   
>> -    if (channel->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
>> -        SocketAddress *saddr = &channel->u.socket;
>> +    if (addr->transport == MIGRATION_ADDRESS_TYPE_SOCKET) {
>> +        SocketAddress *saddr = &addr->u.socket;
>>           if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
>>               saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
>>               saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
>> @@ -1753,11 +1776,11 @@ void qmp_migrate(const char *uri, bool has_channels,
>>               fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
>>           }
>>   #ifdef CONFIG_RDMA
>> -    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
>> -        rdma_start_outgoing_migration(s, &channel->u.rdma, &local_err);
>> +    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
>> +        rdma_start_outgoing_migration(s, &addr->u.rdma, &local_err);
>>   #endif
>> -    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
>> -        exec_start_outgoing_migration(s, channel->u.exec.args, &local_err);
>> +    } else if (addr->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
>> +        exec_start_outgoing_migration(s, addr->u.exec.args, &local_err);
>>       } else {
>>           if (!resume_requested) {
>>               yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>> -- 
>> 2.22.3
>>
> With regards,
> Daniel
Regards,
Het Gala