[PATCH 0/2] Fix: qtest/migration: Improve multifd_tcp_channels_none test

[PATCH 0/2] Fix: qtest/migration: Improve multifd_tcp_channels_none test

[Het Gala]

With the introduction of new patchset to have 'channels' as the start
argument of migrate QAPIs instead of 'uri' (tests/qtest/migration: Add
tests for introducing 'channels' argument in migrate QAPIs), a few minor
typos got went unnoticed, which were caught while trying to introduce
similar qtests in migration.
Fix multifd_tcp_channels_none qtest to actually utilise 'channels' arg
in migrate QAPIs

This patchset is built on top of (tests/qtest/migration: Add tests for
introducing 'channels' argument in migrate QAPIs)

Het Gala (2):
  Fix typo to allow migrate_qmp_fail command with 'channels' argument
  Call args->connect_channels to actually test multifd_tcp_channels_none
    qtest

 tests/qtest/migration-helpers.c | 2 --
 tests/qtest/migration-test.c    | 4 ++--
 2 files changed, 2 insertions(+), 4 deletions(-)

-- 
2.22.3

[PATCH 1/2] Fix typo to allow migrate_qmp_fail command with 'channels' argument

[Het Gala]

Fixes: (tests/qtest/migration: Add negative tests to validate migration QAPIs)
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
 tests/qtest/migration-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index d03a655f83..584d7c496f 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1724,7 +1724,7 @@ static void test_precopy_common(MigrateCommon *args)
     }
 
     if (args->result == MIG_TEST_QMP_ERROR) {
-        migrate_qmp_fail(from, args->connect_uri, args->connect_uri, "{}");
+        migrate_qmp_fail(from, args->connect_uri, args->connect_channels, "{}");
         goto finish;
     }
 
-- 
2.22.3

[PATCH 2/2] Call args->connect_channels to actually test multifd_tcp_channels_none qtest

[Het Gala]

Earlier, without args->connect_channels, multifd_tcp_channels_none would
call uri internally even though connect_channels was introduced in
function definition. To actually call 'migrate' QAPI with modified syntax,
args->connect_channels need to be passed.
Double free happens while setting correct migration ports. Fix that.

Fixes: (tests/qtest/migration: Add multifd_tcp_plain test using list of
        channels instead of uri)
Signed-off-by: Het Gala <het.gala@nutanix.com>
---
 tests/qtest/migration-helpers.c | 2 --
 tests/qtest/migration-test.c    | 2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index b2a90469fb..b1d06187ab 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -146,8 +146,6 @@ static void migrate_set_ports(QTestState *to, QList *channel_list)
                 qdict_put_str(addrdict, "port", addr_port);
         }
     }
-
-    qobject_unref(addr);
 }
 
 bool migrate_watch_for_events(QTestState *who, const char *name,
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 584d7c496f..5d6d8cd634 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1728,7 +1728,7 @@ static void test_precopy_common(MigrateCommon *args)
         goto finish;
     }
 
-    migrate_qmp(from, to, args->connect_uri, NULL, "{}");
+    migrate_qmp(from, to, args->connect_uri, args->connect_channels, "{}");
 
     if (args->result != MIG_TEST_SUCCEED) {
         bool allow_active = args->result == MIG_TEST_FAIL;
-- 
2.22.3

Re: [PATCH 1/2] Fix typo to allow migrate_qmp_fail command with 'channels' argument

[Peter Xu]

Hey, Het,

On Sun, Apr 07, 2024 at 01:21:24PM +0000, Het Gala wrote:
> Fixes: (tests/qtest/migration: Add negative tests to validate migration QAPIs)

I think I get your intention to provide two fixup patches on top of
migration-next, which indeed would be preferred so that I can squash them
into the patches before the pull.

However please next time use "git commit --fixup" so that a better subject
will be generated, and that'll make my life (and Fabiano's I suppose in the
future) easier because git rebase understand those subjects.  Then you
don't need Fixes with an empty commit ID.  They'll start with "fixup: XXX"
pointing to a commit with subject rather than commit IDs.

Thanks,

> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  tests/qtest/migration-test.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index d03a655f83..584d7c496f 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -1724,7 +1724,7 @@ static void test_precopy_common(MigrateCommon *args)
>      }
>  
>      if (args->result == MIG_TEST_QMP_ERROR) {
> -        migrate_qmp_fail(from, args->connect_uri, args->connect_uri, "{}");
> +        migrate_qmp_fail(from, args->connect_uri, args->connect_channels, "{}");
>          goto finish;
>      }

-- 
Peter Xu

Re: [PATCH 2/2] Call args->connect_channels to actually test multifd_tcp_channels_none qtest

[Peter Xu]

On Sun, Apr 07, 2024 at 01:21:25PM +0000, Het Gala wrote:
> Earlier, without args->connect_channels, multifd_tcp_channels_none would
> call uri internally even though connect_channels was introduced in
> function definition. To actually call 'migrate' QAPI with modified syntax,
> args->connect_channels need to be passed.
> Double free happens while setting correct migration ports. Fix that.
> 
> Fixes: (tests/qtest/migration: Add multifd_tcp_plain test using list of
>         channels instead of uri)

[1]

> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  tests/qtest/migration-helpers.c | 2 --
>  tests/qtest/migration-test.c    | 2 +-
>  2 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
> index b2a90469fb..b1d06187ab 100644
> --- a/tests/qtest/migration-helpers.c
> +++ b/tests/qtest/migration-helpers.c
> @@ -146,8 +146,6 @@ static void migrate_set_ports(QTestState *to, QList *channel_list)
>                  qdict_put_str(addrdict, "port", addr_port);
>          }
>      }
> -
> -    qobject_unref(addr);

Firstly, this doesn't belong to the commit you were pointing at above [1].
Instead this line is part of:

  tests/qtest/migration: Add migrate_set_ports into migrate_qmp to update migration port value

You may want to split them?

Side note: I didn't review carefully on the whole patchset, but I think
it's preferred to not include any dead code like what you did with
"tests/qtest/migration: Add migrate_set_ports into migrate_qmp to update
migration port value".  It'll be better to me if we introduce code that
will be used already otherwise reviewing such patch is a pain, same to when
we follow up stuff later like this.

More importantly.. why free?  I'll paste whole thing over, and raise my
questions.

static void migrate_set_ports(QTestState *to, QList *channel_list)
{
    QDict *addr;
    QListEntry *entry;
    g_autofree const char *addr_port = NULL;   <--------- this points to sub-field of "addr", if we free "addr", why autofree here?

    addr = migrate_get_connect_qdict(to);

    QLIST_FOREACH_ENTRY(channel_list, entry) {
        QDict *channel = qobject_to(QDict, qlist_entry_obj(entry));
        QDict *addrdict = qdict_get_qdict(channel, "addr");

        if (qdict_haskey(addrdict, "port") &&
            qdict_haskey(addr, "port") &&
            (strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
                addr_port = qdict_get_str(addr, "port");
                qdict_put_str(addrdict, "port", addr_port);  <--------- shouldn't we g_strdup() instead of dropping the below unref()?
        }
    }

    qobject_unref(addr);
}

>  }
>  
>  bool migrate_watch_for_events(QTestState *who, const char *name,
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 584d7c496f..5d6d8cd634 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -1728,7 +1728,7 @@ static void test_precopy_common(MigrateCommon *args)
>          goto finish;
>      }
>  
> -    migrate_qmp(from, to, args->connect_uri, NULL, "{}");
> +    migrate_qmp(from, to, args->connect_uri, args->connect_channels, "{}");
>  
>      if (args->result != MIG_TEST_SUCCEED) {
>          bool allow_active = args->result == MIG_TEST_FAIL;
> -- 
> 2.22.3
> 

-- 
Peter Xu

Re: [PATCH 1/2] Fix typo to allow migrate_qmp_fail command with 'channels' argument

[Het Gala]

[-- Attachment #1: Type: text/plain, Size: 1185 bytes --]


On 08/04/24 9:05 pm, Peter Xu wrote:
> !-------------------------------------------------------------------|
>    CAUTION: External Email
>
> |-------------------------------------------------------------------!
>
> Hey, Het,
>
> On Sun, Apr 07, 2024 at 01:21:24PM +0000, Het Gala wrote:
>> Fixes: (tests/qtest/migration: Add negative tests to validate migration QAPIs)
> I think I get your intention to provide two fixup patches on top of
> migration-next, which indeed would be preferred so that I can squash them
> into the patches before the pull.
>
> However please next time use "git commit --fixup" so that a better subject
> will be generated, and that'll make my life (and Fabiano's I suppose in the
> future) easier because git rebase understand those subjects.  Then you
> don't need Fixes with an empty commit ID.  They'll start with "fixup: XXX"
> pointing to a commit with subject rather than commit IDs.
I apologize for any inconvenience caused by not using "git commit 
--fixup" in my previous submission. Let me resend the patchset with 
correct message convention. Will take care of this in future patches 
too, thanks for bringing it to my notice. Regards, Het Gala

[-- Attachment #2: Type: text/html, Size: 2214 bytes --]

Re: [PATCH 1/2] Fix typo to allow migrate_qmp_fail command with 'channels' argument

[Peter Xu]

[-- Attachment #1: Type: text/plain, Size: 1444 bytes --]

Het,

It's all fine, no worries! This is good enough.  Let's finish the
discussion in the next patch before a repost.

Thanks,

On Mon, Apr 8, 2024, 2:35 p.m. Het Gala <het.gala@nutanix.com> wrote:

>
> On 08/04/24 9:05 pm, Peter Xu wrote:
>
> !-------------------------------------------------------------------|
>   CAUTION: External Email
>
> |-------------------------------------------------------------------!
>
> Hey, Het,
>
> On Sun, Apr 07, 2024 at 01:21:24PM +0000, Het Gala wrote:
>
> Fixes: (tests/qtest/migration: Add negative tests to validate migration QAPIs)
>
>
> I think I get your intention to provide two fixup patches on top of
> migration-next, which indeed would be preferred so that I can squash them
> into the patches before the pull.
>
> However please next time use "git commit --fixup" so that a better subject
> will be generated, and that'll make my life (and Fabiano's I suppose in the
> future) easier because git rebase understand those subjects.  Then you
> don't need Fixes with an empty commit ID.  They'll start with "fixup: XXX"
> pointing to a commit with subject rather than commit IDs.
>
> I apologize for any inconvenience caused by not using "git commit --fixup"
> in my previous submission. Let me resend the patchset with correct message
> convention. Will take care of this in future patches too, thanks for
> bringing it to my notice. Regards, Het Gala
>

[-- Attachment #2: Type: text/html, Size: 2486 bytes --]

Re: [PATCH 2/2] Call args->connect_channels to actually test multifd_tcp_channels_none qtest

[Het Gala]

[-- Attachment #1: Type: text/plain, Size: 4254 bytes --]


On 08/04/24 9:10 pm, Peter Xu wrote:
> !-------------------------------------------------------------------|
>    CAUTION: External Email
>
> |-------------------------------------------------------------------!
>
> On Sun, Apr 07, 2024 at 01:21:25PM +0000, Het Gala wrote:
>> Earlier, without args->connect_channels, multifd_tcp_channels_none would
>> call uri internally even though connect_channels was introduced in
>> function definition. To actually call 'migrate' QAPI with modified syntax,
>> args->connect_channels need to be passed.
>> Double free happens while setting correct migration ports. Fix that.
>>
>> Fixes: (tests/qtest/migration: Add multifd_tcp_plain test using list of
>>          channels instead of uri)
> [1]
>
>> Signed-off-by: Het Gala<het.gala@nutanix.com>
>> ---
>>   tests/qtest/migration-helpers.c | 2 --
>>   tests/qtest/migration-test.c    | 2 +-
>>   2 files changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
>> index b2a90469fb..b1d06187ab 100644
>> --- a/tests/qtest/migration-helpers.c
>> +++ b/tests/qtest/migration-helpers.c
>> @@ -146,8 +146,6 @@ static void migrate_set_ports(QTestState *to, QList *channel_list)
>>                   qdict_put_str(addrdict, "port", addr_port);
>>           }
>>       }
>> -
>> -    qobject_unref(addr);
> Firstly, this doesn't belong to the commit you were pointing at above [1].
> Instead this line is part of:
>
>    tests/qtest/migration: Add migrate_set_ports into migrate_qmp to update migration port value
>
> You may want to split them?
Ack
> Side note: I didn't review carefully on the whole patchset, but I think
> it's preferred to not include any dead code like what you did with
> "tests/qtest/migration: Add migrate_set_ports into migrate_qmp to update
> migration port value".  It'll be better to me if we introduce code that
> will be used already otherwise reviewing such patch is a pain, same to when
> we follow up stuff later like this.
Yes Peter. My intention was to have the code which could actually take the
benefit of using 'channels' for the new QAPI syntax. But somehow I missed
adding connect_channels in the code, despite that the test passed because
it generated connect_uri with the help of listen_uri inside migrate_qmp.
And it generated migrate QMP command using old syntax. Also because it never
entered migrate_set_ports, couldn't catch double free issue while manual
testing as well as while the CI/CD pipeline was run.
> More importantly.. why free?  I'll paste whole thing over, and raise my
> questions.
>
> static void migrate_set_ports(QTestState *to, QList *channel_list)
> {
>      QDict *addr;
>      QListEntry *entry;
>      g_autofree const char *addr_port = NULL;   <--------- this points to sub-field of "addr", if we free "addr", why autofree here?
>
>      addr = migrate_get_connect_qdict(to);
>
>      QLIST_FOREACH_ENTRY(channel_list, entry) {
>          QDict *channel = qobject_to(QDict, qlist_entry_obj(entry));
>          QDict *addrdict = qdict_get_qdict(channel, "addr");
>
>          if (qdict_haskey(addrdict, "port") &&
>              qdict_haskey(addr, "port") &&
>              (strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
>                  addr_port = qdict_get_str(addr, "port");
>                  qdict_put_str(addrdict, "port", addr_port);  <--------- shouldn't we g_strdup() instead of dropping the below unref()?
>          }
>      }
>
>      qobject_unref(addr);
> }
Yes, I got your point Peter. Will update in the new patch.
>>   }
>>   
>>   bool migrate_watch_for_events(QTestState *who, const char *name,
>> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
>> index 584d7c496f..5d6d8cd634 100644
>> --- a/tests/qtest/migration-test.c
>> +++ b/tests/qtest/migration-test.c
>> @@ -1728,7 +1728,7 @@ static void test_precopy_common(MigrateCommon *args)
>>           goto finish;
>>       }
>>   
>> -    migrate_qmp(from, to, args->connect_uri, NULL, "{}");
>> +    migrate_qmp(from, to, args->connect_uri, args->connect_channels, "{}");
>>   
>>       if (args->result != MIG_TEST_SUCCEED) {
>>           bool allow_active = args->result == MIG_TEST_FAIL;
>> -- 
>> 2.22.3
Regards,
Het Gala

[-- Attachment #2: Type: text/html, Size: 5850 bytes --]