| Message ID | 20240407132125.159528-3-het.gala@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix: qtest/migration: Improve multifd_tcp_channels_none test | expand |
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 >
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
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); } 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;
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) 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(-)