Re: [PATCH 1/8] tests/qtest/migration: Fix leak of migration tests data

[Fabiano Rosas <farosas@suse.de>]

Prasad Pandit <ppandit@redhat.com> writes:

> On Tue, 10 Mar 2026 at 19:26, Fabiano Rosas <farosas@suse.de> wrote:
>> When the migration-test is invoked with the '-p' flag (to run a single
>> test), the glib code won't call the destroy function for the
>> not-executed tests, causing the MigrationTest wrapper data to leak.
>
> * With -p, only test data for that test should be loaded, right?
> Freeing the data for non-executed tests does not seem right.
>

Hm, I don't know what you mean by loaded. Glib will call only the
specific test, passing the data that was registered along with that
test. That registration is done beforehand with the glib_add_data_func
functions. We need to allocate that somehow. If we allocated it, we must
free it.

Ideally, glib would provide a hook for setup and another one for
teardown, which would work like (I think) you expect. But it doesn't do
that. The "destroy" hook is semantically "undo what was done in the
test", not free resources allocated outside of glib.

Some simpler tests can avoid dynamically allocation, but migration-test
is a bit more complex, so I don't see a way to do that at the moment.

>> This doesn't affect make check, but affects debugging use-cases where
>> having a leak pop up in ASAN output is extra annoying.
>>
>> Fix by adding the tests data to a list and freeing them all at the end
>> of migration-test execution. Any tests actually dispatched by glib
>> will have the destroy function called as usual.
>>
>> Note that migration_test_add_suffix() is altered to call
>> migration_test_add() so that there's only one place adding the data to
>> the list.
>>
>> Performance is not an issue at the moment, we have < 100 tests.
>>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  tests/qtest/migration/framework.c      |  2 ++
>>  tests/qtest/migration/migration-util.c | 19 ++++++++++++++-----
>>  tests/qtest/migration/migration-util.h |  2 +-
>>  3 files changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/tests/qtest/migration/framework.c b/tests/qtest/migration/framework.c
>> index 0bfc241914..b9371372de 100644
>> --- a/tests/qtest/migration/framework.c
>> +++ b/tests/qtest/migration/framework.c
>> @@ -1162,5 +1162,7 @@ int migration_env_clean(MigrationTestEnv *env)
>>      }
>>      g_free(tmpfs);
>>
>> +    migration_tests_free();
>> +
>>      return ret;
>>  }
>> diff --git a/tests/qtest/migration/migration-util.c b/tests/qtest/migration/migration-util.c
>> index c2462306a1..2648ad7f61 100644
>> --- a/tests/qtest/migration/migration-util.c
>> +++ b/tests/qtest/migration/migration-util.c
>> @@ -38,6 +38,7 @@
>>  #include "linux/kvm.h"
>>  #endif
>>
>> +GQueue *tests;
>>
>>  static char *SocketAddress_to_str(SocketAddress *addr)
>>  {
>> @@ -243,6 +244,8 @@ static void migration_test_destroy(gpointer data)
>>  {
>>      MigrationTest *test = (MigrationTest *)data;
>>
>> +    g_queue_remove(tests, test);
>> +
>>      g_free(test->data);
>>      g_free(test->name);
>>      g_free(test);
>> @@ -268,21 +271,27 @@ void migration_test_add(const char *path,
>>
>>      qtest_add_data_func_full(path, test, migration_test_wrapper,
>>                               migration_test_destroy);
>> +    if (!tests) {
>> +        tests = g_queue_new();
>> +    }
>> +    g_queue_push_tail(tests, test);
>>  }
>>
>>  void migration_test_add_suffix(const char *path, const char *suffix,
>>                                 void (*fn)(char *name, MigrateCommon *args))
>>  {
>> -    MigrationTest *test = g_new0(MigrationTest, 1);
>> +    g_autofree char *name = NULL;
>>
>>      g_assert(g_str_has_suffix(path, "/"));
>>      g_assert(!g_str_has_prefix(suffix, "/"));
>>
>> -    test->func = fn;
>> -    test->name = g_strconcat(path, suffix, NULL);
>> +    name = g_strconcat(path, suffix, NULL);
>> +    migration_test_add(name, fn);
>> +}
>>
>> -    qtest_add_data_func_full(test->name, test, migration_test_wrapper,
>> -                             migration_test_destroy);
>> +void migration_tests_free(void)
>> +{
>> +    g_queue_free_full(tests, migration_test_destroy);
>>  }
>>
>>  #ifdef O_DIRECT
>> diff --git a/tests/qtest/migration/migration-util.h b/tests/qtest/migration/migration-util.h
>> index e73d69bab0..694773e594 100644
>> --- a/tests/qtest/migration/migration-util.h
>> +++ b/tests/qtest/migration/migration-util.h
>> @@ -59,5 +59,5 @@ void migration_test_add_suffix(const char *path, const char *suffix,
>>                                 void (*fn)(char *name, MigrateCommon *args));
>>  char *migrate_get_connect_uri(QTestState *who);
>>  void migrate_set_ports(QTestState *to, QList *channel_list);
>> -
>> +void migration_tests_free(void);
>>  #endif /* MIGRATION_UTIL_H */
>> --
>
> * Change looks okay. Still I think there could be a way to load only
> the data required for the specific test.
> Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
>
> Thank you.
> ---
>   - Prasad