On Thu, Aug 14, 2025 at 12:30:00PM -0300, Fabiano Rosas wrote:
> Peter Xu <pet...@redhat.com> writes:
>
> > On Mon, Jun 30, 2025 at 04:59:13PM -0300, Fabiano Rosas wrote:
> >> Use the existing file tests to test the new way of passing parameters
> >> to the migration via the config argument to qmp_migrate*.
> >>
> >> Signed-off-by: Fabiano Rosas <faro...@suse.de>
> >> ---
> >> tests/qtest/migration/file-tests.c | 68 +++++++++++----------------
> >> tests/qtest/migration/framework.c | 9 ++--
> >> tests/qtest/migration/precopy-tests.c | 1 +
> >> 3 files changed, 34 insertions(+), 44 deletions(-)
> >>
> >> diff --git a/tests/qtest/migration/file-tests.c
> >> b/tests/qtest/migration/file-tests.c
> >> index 4d78ce0855..656d6527e8 100644
> >> --- a/tests/qtest/migration/file-tests.c
> >> +++ b/tests/qtest/migration/file-tests.c
> >> @@ -27,6 +27,7 @@ static void test_precopy_file(void)
> >> MigrateCommon args = {
> >> .connect_uri = uri,
> >> .listen_uri = "defer",
> >> + .start.config = qdict_new(),
> >> };
> >>
> >> test_file_common(&args, true);
> >> @@ -74,6 +75,7 @@ static void test_precopy_file_offset_fdset(void)
> >> .connect_uri = uri,
> >> .listen_uri = "defer",
> >> .start_hook = migrate_hook_start_file_offset_fdset,
> >> + .start.config = qdict_new(),
> >> };
> >>
> >> test_file_common(&args, false);
> >> @@ -88,6 +90,7 @@ static void test_precopy_file_offset(void)
> >> MigrateCommon args = {
> >> .connect_uri = uri,
> >> .listen_uri = "defer",
> >> + .start.config = qdict_new(),
> >> };
> >>
> >> test_file_common(&args, false);
> >> @@ -102,6 +105,7 @@ static void test_precopy_file_offset_bad(void)
> >> .connect_uri = uri,
> >> .listen_uri = "defer",
> >> .result = MIG_TEST_QMP_ERROR,
> >> + .start.config = qdict_new(),
> >> };
> >>
> >> test_file_common(&args, false);
> >> @@ -114,11 +118,10 @@ static void test_precopy_file_mapped_ram_live(void)
> >> MigrateCommon args = {
> >> .connect_uri = uri,
> >> .listen_uri = "defer",
> >> - .start = {
> >> - .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
> >> - },
> >> + .start.config = qdict_new(),
> >> };
> >>
> >> + qdict_put_bool(args.start.config, "mapped-ram", true);
> >> test_file_common(&args, false);
> >> }
> >>
> >> @@ -129,11 +132,9 @@ static void test_precopy_file_mapped_ram(void)
> >> MigrateCommon args = {
> >> .connect_uri = uri,
> >> .listen_uri = "defer",
> >> - .start = {
> >> - .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
> >> - },
> >> + .start.config = qdict_new(),
> >> };
> >> -
> >> + qdict_put_bool(args.start.config, "mapped-ram", true);
> >> test_file_common(&args, true);
> >> }
> >>
> >> @@ -144,12 +145,11 @@ static void test_multifd_file_mapped_ram_live(void)
> >> MigrateCommon args = {
> >> .connect_uri = uri,
> >> .listen_uri = "defer",
> >> - .start = {
> >> - .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
> >> - .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
> >> - },
> >> + .start.config = qdict_new(),
> >> };
> >>
> >> + qdict_put_bool(args.start.config, "mapped-ram", true);
> >> + qdict_put_bool(args.start.config, "multifd", true);
> >> test_file_common(&args, false);
> >> }
> >>
> >> @@ -160,24 +160,13 @@ static void test_multifd_file_mapped_ram(void)
> >> MigrateCommon args = {
> >> .connect_uri = uri,
> >> .listen_uri = "defer",
> >> - .start = {
> >> - .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
> >> - .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
> >> - },
> >> + .start.config = qdict_new(),
> >> };
> >> -
> >> + qdict_put_bool(args.start.config, "mapped-ram", true);
> >> + qdict_put_bool(args.start.config, "multifd", true);
> >> test_file_common(&args, true);
> >> }
> >>
> >> -static void *migrate_hook_start_multifd_mapped_ram_dio(QTestState *from,
> >> - QTestState *to)
> >> -{
> >> - migrate_set_parameter_bool(from, "direct-io", true);
> >> - migrate_set_parameter_bool(to, "direct-io", true);
> >> -
> >> - return NULL;
> >> -}
> >> -
> >> static void test_multifd_file_mapped_ram_dio(void)
> >> {
> >> g_autofree char *uri = g_strdup_printf("file:%s/%s", tmpfs,
> >> @@ -185,13 +174,13 @@ static void test_multifd_file_mapped_ram_dio(void)
> >> MigrateCommon args = {
> >> .connect_uri = uri,
> >> .listen_uri = "defer",
> >> - .start_hook = migrate_hook_start_multifd_mapped_ram_dio,
> >> - .start = {
> >> - .caps[MIGRATION_CAPABILITY_MAPPED_RAM] = true,
> >> - .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
> >> - },
> >> + .start.config = qdict_new(),
> >> };
> >>
> >> + qdict_put_bool(args.start.config, "direct-io", true);
> >
> > So the start_hook doesn't take args so we need to duplicate all these
> > direct-io setups in each test.. I assume not a big deal so it's fine, but
> > this is slightly going backward for sure..
> >
>
> I'm not sure it is. Having to go follow the hooks is confusing,
> specially when hook names start to get similar. Having the test provide
> everything it needs right here is clearer. Also, maintenance of the
> hooks is a pain when it comes to code conflicts. I'd like to see less
> hooks overall.
IMHO it depends. If a hook can greatly dedup code, then I'll go for it.
This one only contains a dio setup, definitely not a huge deal.
>
> > What's your plan in mind on the tests? Looks like you want to keep both
> > ways in tests/, only use it in some tests to cover both paths (and you
> > chose file-tests to start testing config)? Or is this only an example and
> > you plan to convert more?
> >
>
> Yes the idea is to cover both paths and I chose file-tests for config
> arbitrarily.
But then file-tests lose the old coverage on using migrate-set-*.
IMHO, we could choose one that will be the officially suggested way, then
let all tests to use it. With that, we can duplicate one or a few tests to
cover the not-suggested way.
So if "config" is the suggested way, we could make all tests moving over to
"config", then add only one or a few tests to cover migrate-set-parameters?
--
Peter Xu
