[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/2] Call args->connect_channels to actually test multifd_tcp
From: |
Peter Xu |
Subject: |
Re: [PATCH 2/2] Call args->connect_channels to actually test multifd_tcp_channels_none qtest |
Date: |
Mon, 8 Apr 2024 11:40:19 -0400 |
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