Re: [PATCH v1 11/15] tests/qtest/migration: Use defer for cpr-tests

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Tue, May 05, 2026 at 01:09:10PM -0300, Fabiano Rosas wrote:
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  tests/qtest/migration/cpr-tests.c | 21 ++++++++++++---------
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
> Only one thing to double check:
>
>>  1 file changed, 12 insertions(+), 9 deletions(-)
>> 
>> diff --git a/tests/qtest/migration/cpr-tests.c b/tests/qtest/migration/cpr-tests.c
>> index 18217f0460..9091a84213 100644
>> --- a/tests/qtest/migration/cpr-tests.c
>> +++ b/tests/qtest/migration/cpr-tests.c
>> @@ -60,7 +60,7 @@ static int test_transfer(MigrateCommon *args, const char *cpr_channel,
>>      obj = migrate_str_to_channel(cpr_channel);
>>      qlist_append(channels_list, obj);
>>  
>> -    if (migrate_start(&from, &to, args->listen_uri, &args->start)) {
>> +    if (migrate_start(&from, &to, "defer", &args->start)) {
>>          return -1;
>>      }
>>  
>> @@ -128,11 +128,17 @@ static void test_mode_transfer_common(MigrateCommon *args, bool incoming_defer)
>>      int cpr_sockfd = qtest_socket_server(cpr_path);
>>      g_assert(cpr_sockfd >= 0);
>>  
>> -    opts_target = g_strdup_printf("-incoming cpr,addr.transport=socket,"
>> -                                  "addr.type=fd,addr.str=%d %s",
>> -                                  cpr_sockfd, opts);
>> +    if (incoming_defer) {
>> +        opts_target = g_strdup_printf("-incoming cpr,addr.transport=socket,"
>> +                                      "addr.type=fd,addr.str=%d %s",
>> +                                      cpr_sockfd, opts);
>> +    } else {
>> +        opts_target = g_strdup_printf("-incoming %s "
>> +                                      "-incoming cpr,addr.transport=socket,"
>> +                                      "addr.type=fd,addr.str=%d %s",
>> +                                      uri, cpr_sockfd, opts);
>
> IIUC now in this path qtest will ultimately generate cmdline like this for
> dest:
>
>   -incoming defer \
>   -incoming unix:... \
>   -incoming cpr,addr...
>
> I am not sure if this is the intention, but it looks working indeed where
> the 1st defer will be an no-op, and "-incoming" allows overwrites.
>

Yes, it's on purpose, I think I remember some discussion at the time
about this and we decided it was ok.

In any case, it's probably good to have some variability because that
parsing code at vl.c is quite fragile in my opinion. Better to find a
breakage during testing than in production.

> We'll just need to remember this and be careful in the future on the order
> of these parameters. Say, if we accidentally move the first "-incoming
> defer" parameter to be after opts_target, then it might break, because then
> it's not no-op anymore but freeing the main channel we just set.
>
> But I think it's ok for now.
>
>> +    }
>>  
>> -    args->listen_uri = incoming_defer ? "defer" : uri;
>>      args->connect_channels = connect_channels;
>>  
>>      args->start.opts_source = opts;
>> @@ -224,7 +230,7 @@ static void test_cpr_exec(MigrateCommon *args)
>>      g_autofree char *filename = g_strdup_printf("%s/%s", tmpfs,
>>                                                  FILE_TEST_FILENAME);
>>  
>> -    if (migrate_start(&from, NULL, args->listen_uri, &args->start)) {
>> +    if (migrate_start(&from, NULL, "defer", &args->start)) {
>>          return;
>>      }
>>  
>> @@ -274,10 +280,7 @@ static void test_mode_exec(char *name, MigrateCommon *args)
>>  {
>>      g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs,
>>                                             FILE_TEST_FILENAME);
>> -    g_autofree char *listen_uri = g_strdup_printf("defer");
>> -
>>      args->connect_uri = uri;
>> -    args->listen_uri = listen_uri;
>>      args->start_hook = test_mode_exec_start;
>>  
>>      args->start.only_source = true;
>> -- 
>> 2.51.0
>>