Re: [PATCH v4 8/8] migration: Introduced MigrateChannelList struct to migration code flow.

[Juan Quintela]

Het Gala <het.gala@nutanix.com> wrote:
> On 15/05/23 4:12 pm, Daniel P. Berrangé wrote:
>> On Fri, May 12, 2023 at 02:32:40PM +0000, Het Gala wrote:
>>> Integrated MigrateChannelList with all transport backends (socket, exec
>>> and rdma) for both source and destination migration code flow.
>>>
>>> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
>>> Signed-off-by: Het Gala <het.gala@nutanix.com>
>>> ---
>>>   migration/migration.c | 95 +++++++++++++++++++++++++++----------------
>>>   migration/socket.c    |  5 ++-
>>>   2 files changed, 64 insertions(+), 36 deletions(-)
>>>
>>>       Error *local_err = NULL;
>>> +    MigrateChannel *val = g_new0(MigrateChannel, 1);
>>>       MigrateAddress *addrs = g_new0(MigrateAddress, 1);
>>>       SocketAddress *saddr;
>>>       InetSocketAddress *isock = &addrs->u.rdma;
>>> @@ -441,6 +442,7 @@ static bool migrate_uri_parse(const char *uri,
>>>       }
>>>         if (local_err) {
>>> +        qapi_free_MigrateChannel(val);
>>>           qapi_free_MigrateAddress(addrs);
>>>           qapi_free_SocketAddress(saddr);
>>>           qapi_free_InetSocketAddress(isock);
>>> @@ -448,7 +450,9 @@ static bool migrate_uri_parse(const char *uri,
>>>           return false;
>>>       }
>>>   -    *channel = addrs;
>>> +    val->channeltype = MIGRATE_CHANNEL_TYPE_MAIN;
>>> +    val->addr = addrs;
>>> +    *channel = val;
>>>       return true;
>>>   }
>>>   @@ -457,8 +461,9 @@ static void
>>> qemu_start_incoming_migration(const char *uri, bool has_channels,
>>>                                             Error **errp)
>>>   {
>>>       Error *local_err = NULL;
>>> -    MigrateAddress *channel = g_new0(MigrateAddress, 1);
>>> +    MigrateAddress *addrs;
>>>       SocketAddress *saddr;
>>> +    MigrateChannel *channel = NULL;
>>>         /*
>>>        * Having preliminary checks for uri and channel
>>> @@ -467,22 +472,30 @@ static void qemu_start_incoming_migration(const char 
>>> *uri, bool has_channels,
>>>           error_setg(errp, "'uri' and 'channels' arguments are mutually "
>>>                      "exclusive; exactly one of the two should be present 
>>> in "
>>>                      "'migrate-incoming' qmp command ");
>>> -        return;
>>> -    }
>>> -
>>> -    /* URI is not suitable for migration? */
>>> -    if (!migration_channels_and_uri_compatible(uri, errp)) {
>>>           goto out;
>>> -    }
>>> +    } 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");
>>> +            goto out;
>>> +        }
>>> +        channel = channels->value;
>>> +    } else {
>>> +        /* URI is not suitable for migration? */
>>> +        if (!migration_channels_and_uri_compatible(uri, errp)) {
>>> +            goto out;
>>> +        }
>> THis check only gets executed when the caller uses the old
>> URI syntax. We need to it be run when using the modern
>> MigrateChannel QAPI syntax too.
>>
>> IOW, migration_channels_and_uri_compatible() needs converting
>> to take a 'MigrateChannel' object instead of URI, and then
>> the check can be run after the URI -> MigrateCHannel conversion
>
> Yes, Daniel. Got your point. Will change it in the next version.
>
> For Juan's comments, I have not explored the test side code still, so
> is the idea to have some similar test functions like
> test_precopy_tcp_plain, test_precopy_unix_plain but with the new
> syntax ? Please confirm this, and any advice on how appraoch this?

There are several places that use this syntax:

    rsp = wait_command(to, "{ 'execute': 'migrate-incoming',"
                           "  'arguments': { 'uri': 'tcp:127.0.0.1:0' }}");

Just change a couple of them to the new syntax.

I guess you will want to do the multifd tests with the new syntax, not
sure if it makes sense to also test the old one.

I guess you will also want to check that your are catching errors (like
different number of channels on source/destination).

Later, Juan.