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

[Peter Xu <peterx@redhat.com>]

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