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.g...@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
