Re: [PATCH v3 25/51] tests/qtest/migration: Pass MigrateCommon into test functions

[Peter Xu <peterx@redhat.com>]

On Mon, Dec 15, 2025 at 07:00:11PM -0300, Fabiano Rosas wrote:
> With the upcoming addition of the config QDict, the tests will need a
> better way of managing the memory of the test data than putting the
> test arguments on the stack of the test functions. The config QDict
> will need to be merged into the arguments of migrate_qmp* functions,
> which causes a refcount increment, so the test functions would need to
> allocate and deref the config QDict themselves.
> 
> A better approach is to already pass the arguments into the test
> functions and do the memory management in the existing wrapper. There
> is already migration_test_destroy(), which is called for every test.
> 
> Do the following:
> 
> - merge the two existing wrappers, migration_test_wrapper() and
>   migration_test_wrapper_full(). The latter was pioneer in passing
>   data into the tests, but now all tests will receive data, so we
>   don't need it anymore.
> 
>   The usage of migration_test_wrapper_full() was in passing a slightly
>   different test name string into the cancel tests, so still keep the
>   migration_test_add_suffix() function.
> 
> - add (char *name, MigrateCommon *args) to the signature of all test
>   functions.
> 
> - alter any code to stop allocating args on the stack and instead use
>   the object that came as parameter.
> 
> - pass args around as needed.
> 
> - while here, order args (MigrateCommon) before args->start
>   (MigrateStart) and put a blank like in between.
> 
> No functional change.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

This looks fine,

Reviewed-by: Peter Xu <peterx@redhat.com>

I'm just curious, is it required to touch all these lines?  E.g.,

> ---
>  tests/qtest/migration/compression-tests.c | 127 +++---
>  tests/qtest/migration/cpr-tests.c         |  71 ++--
>  tests/qtest/migration/file-tests.c        | 184 ++++----
>  tests/qtest/migration/migration-util.c    |  26 +-
>  tests/qtest/migration/migration-util.h    |   8 +-
>  tests/qtest/migration/misc-tests.c        | 108 ++---
>  tests/qtest/migration/postcopy-tests.c    |  80 ++--
>  tests/qtest/migration/precopy-tests.c     | 354 +++++++---------
>  tests/qtest/migration/tls-tests.c         | 485 ++++++++++------------
>  9 files changed, 642 insertions(+), 801 deletions(-)
> 
> diff --git a/tests/qtest/migration/compression-tests.c b/tests/qtest/migration/compression-tests.c
> index b827665b8e..845e622cd5 100644
> --- a/tests/qtest/migration/compression-tests.c
> +++ b/tests/qtest/migration/compression-tests.c
> @@ -31,30 +31,25 @@ migrate_hook_start_precopy_tcp_multifd_zstd(QTestState *from,
>      return migrate_hook_start_precopy_tcp_multifd_common(from, to, "zstd");
>  }
>  
> -static void test_multifd_tcp_zstd(void)
> +static void test_multifd_tcp_zstd(char *name, MigrateCommon *args)
>  {
> -    MigrateCommon args = {

Can this be changed to:

       *args = (struct MigrateCommon) {

So as to avoid touching most of below across whole tree?

> -        .listen_uri = "defer",
> -        .start = {
> -            .caps[MIGRATION_CAPABILITY_MULTIFD] = true,
> -        },
> -        .start_hook = migrate_hook_start_precopy_tcp_multifd_zstd,
> -    };
> -    test_precopy_common(&args);
> +    args->listen_uri = "defer";
> +    args->start_hook = migrate_hook_start_precopy_tcp_multifd_zstd;
> +
> +    args->start.caps[MIGRATION_CAPABILITY_MULTIFD] = true;
> +
> +    test_precopy_common(args);
>  }

-- 
Peter Xu