Steven Sistare <[email protected]> writes:
> On 7/25/2025 9:50 AM, Markus Armbruster wrote:
>> The machine name g_strdup()ed by add_machine_test_case() is freed by
>> test_machine(). Since the former runs for all machines, whereas the
>> latter runs only for the selected test case's machines, this leaks the
>> names of machines not selected, if any. Harmless, but fix it anyway:
>> there is no need to dup in the first place, so don't.
>>
>> Signed-off-by: Markus Armbruster <[email protected]>
>> ---
>> tests/qtest/qom-test.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/tests/qtest/qom-test.c b/tests/qtest/qom-test.c
>> index 4ade1c728c..cb5dbfe329 100644
>> --- a/tests/qtest/qom-test.c
>> +++ b/tests/qtest/qom-test.c
>> @@ -220,7 +220,6 @@ static void test_machine(gconstpointer data)
>> qobject_unref(response);
>> qtest_quit(qts);
>> - g_free((void *)machine);
>> }
>>
>> static void add_machine_test_case(const char *mname)
>> @@ -228,7 +227,7 @@ static void add_machine_test_case(const char *mname)
>> char *path;
>> path = g_strdup_printf("qom/%s", mname);
>> - qtest_add_data_func(path, g_strdup(mname), test_machine);
>> + qtest_add_data_func(path, mname, test_machine);
>> g_free(path);
>> }
>
> This will break if qtest_cb_for_every_machine ever composes a machine name on
> the
> stack and passes that to add_machine_test_case. Unlikely, but the strdup
> makes it
> future proof.
Hmm.
qtest obtains machine names via QMP on demand. This is
qtest_get_machines(). Once gotten, they live forever.
Used to live forever, actually: commit 41b2eba4e5c (tests/qtest: Allow
qtest_get_machines to use an alternate QEMU binary) throws them away
when qtest_get_machines() is asked for another QEMU's machine names.
migrate_start() does that. It appears get each one's machine names
twice, because it switches back and forth. Wasteful.
Anyway, you have a point: more stupid shit happens below the hood than I
expected, and even more may be added in the future.
Moreover, at least test-hmp has the same leak. Plugging it here and not
there makes no sense. I'm dropping this patch for now.
> Also, mname is not the only leak. path is also leaked when only a subset of
> tests are run:
>
> qtest_add_data_func(path, ...)
> gchar *path = g_strdup_printf(...)
> g_test_add_data_func(path, ...)
>
> Leaking seems to be par for this course. Maybe not worth fixing.
valgrind shows the machine name leak my patch plugs. It does not show
@path leaking.
Let's have a closer look:
static void add_machine_test_case(const char *mname)
{
char *path;
path = g_strdup_printf("qom/%s", mname);
qtest_add_data_func(path, mname, test_machine);
g_free(path);
}
Always frees @path.
void qtest_add_data_func(const char *str, const void *data,
void (*fn)(const void *))
{
gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str);
g_test_add_data_func(path, data, fn);
g_free(path);
}
Always frees @path.
g_test_add_data_func()'s contract[*] on its first argument: "The data is
owned by the caller of the function."
I can't see a leak.
[*] https://docs.gtk.org/glib/func.test_add_data_func.html
