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? 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