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

[Het Gala]

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