Re: [PATCH v2 24/24] tests/qtest/migration: Pass the migration config to file tests

[Peter Xu <peterx@redhat.com>]

On Thu, Aug 14, 2025 at 12:30:00PM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@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 <farosas@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