[PATCH 0/5] qtest/qom-test: Leak plug, faster, better coverage

[PATCH 0/5] qtest/qom-test: Leak plug, faster, better coverage

[Markus Armbruster]

Markus Armbruster (5):
  qtest/qom-test: Plug memory leak with -p
  qtest/qom-test: Shallow testing of qom-list / qom-get
  qtest/qom-test: Traverse entire QOM tree
  qtest/qom-test: Don't bother to execute QMP command quit
  MAINTAINERS: Cover tests/qtest/qom-test.c

 MAINTAINERS            |  1 +
 tests/qtest/qom-test.c | 11 +++--------
 2 files changed, 4 insertions(+), 8 deletions(-)

-- 
2.49.0

[PATCH 1/5] qtest/qom-test: Plug memory leak with -p

[Markus Armbruster]

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 <armbru@redhat.com>
---
 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);
 }
 
-- 
2.49.0

Re: [PATCH 1/5] qtest/qom-test: Plug memory leak with -p

[Steven Sistare]

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 <armbru@redhat.com>
> ---
>   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.

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.

- Steve

Re: [PATCH 1/5] qtest/qom-test: Plug memory leak with -p

[Markus Armbruster]

Steven Sistare <steven.sistare@oracle.com> 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 <armbru@redhat.com>
>> ---
>>  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

Re: [PATCH 1/5] qtest/qom-test: Plug memory leak with -p

[Steven Sistare]

On 8/5/2025 2:54 AM, Markus Armbruster wrote:
> Steven Sistare <steven.sistare@oracle.com> 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 <armbru@redhat.com>
>>> ---
>>>   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

Agreed.

- Steve

[PATCH 2/5] qtest/qom-test: Shallow testing of qom-list / qom-get

[Markus Armbruster]

This test traverses the QOM sub-tree rooted at /machine with a
combination of qom-list and qom-get.  In my x86_64 testing, it runs
almost 12000 QMP commands in 34 seconds.  With -m slow, we test more
machines, and it takes almost 84000 commands in almost four minutes.

Since commit 3dd93992ffb (tests/qtest/qom-test: unit test for
qom-list-get), the test traverses this tree a second time, with
qom-list-get.  In my x86_64 testing, this takes some 200 QMP commands
and around two seconds, and some 1100 in just under 12s with -m slow.

Traversing the entire tree is useful, because it exercise the QOM
property getters.  Traversing it twice not so much.

Make the qom-list / qom-get test shallow unless -m slow is given:
don't recurse.  Cuts the number of commands to around 600, and run
time to under 5s for me.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/qtest/qom-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/qom-test.c b/tests/qtest/qom-test.c
index cb5dbfe329..40bdc3639f 100644
--- a/tests/qtest/qom-test.c
+++ b/tests/qtest/qom-test.c
@@ -180,7 +180,7 @@ static void test_properties(QTestState *qts, const char *path, bool recurse)
         links = g_slist_delete_link(links, links);
     }
     while (children) {
-        test_properties(qts, children->data, true);
+        test_properties(qts, children->data, g_test_slow());
         g_free(children->data);
         children = g_slist_delete_link(children, children);
     }
-- 
2.49.0

Re: [PATCH 2/5] qtest/qom-test: Shallow testing of qom-list / qom-get

[Steven Sistare]

On 7/25/2025 9:50 AM, Markus Armbruster wrote:
> This test traverses the QOM sub-tree rooted at /machine with a
> combination of qom-list and qom-get.  In my x86_64 testing, it runs
> almost 12000 QMP commands in 34 seconds.  With -m slow, we test more
> machines, and it takes almost 84000 commands in almost four minutes.
> 
> Since commit 3dd93992ffb (tests/qtest/qom-test: unit test for
> qom-list-get), the test traverses this tree a second time, with
> qom-list-get.  In my x86_64 testing, this takes some 200 QMP commands
> and around two seconds, and some 1100 in just under 12s with -m slow.
> 
> Traversing the entire tree is useful, because it exercise the QOM
> property getters.  Traversing it twice not so much.
> 
> Make the qom-list / qom-get test shallow unless -m slow is given:
> don't recurse.  Cuts the number of commands to around 600, and run
> time to under 5s for me.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/qtest/qom-test.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/qom-test.c b/tests/qtest/qom-test.c
> index cb5dbfe329..40bdc3639f 100644
> --- a/tests/qtest/qom-test.c
> +++ b/tests/qtest/qom-test.c
> @@ -180,7 +180,7 @@ static void test_properties(QTestState *qts, const char *path, bool recurse)
>           links = g_slist_delete_link(links, links);
>       }
>       while (children) {
> -        test_properties(qts, children->data, true);
> +        test_properties(qts, children->data, g_test_slow());
>           g_free(children->data);
>           children = g_slist_delete_link(children, children);
>       }

Nice optimization.  These tests were annoyingly slow!

Reviewed-by: Steve Sistare <steven.sistare@oracle.com>

[PATCH 3/5] qtest/qom-test: Traverse entire QOM tree

[Markus Armbruster]

This test traverses the QOM sub-tree rooted at /machine.  Traverse the
entire tree instead.

The x86_64 test runs some 40 additional QMP commands, and stays under
5s for me.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/qtest/qom-test.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/qtest/qom-test.c b/tests/qtest/qom-test.c
index 40bdc3639f..d358b69c7e 100644
--- a/tests/qtest/qom-test.c
+++ b/tests/qtest/qom-test.c
@@ -211,7 +211,7 @@ static void test_machine(gconstpointer data)
 
     test_properties(qts, "/machine", true);
 
-    qlist_append_str(paths, "/machine");
+    qlist_append_str(paths, "/");
     test_list_get(qts, paths);
     test_list_get_value(qts);
 
-- 
2.49.0

Re: [PATCH 3/5] qtest/qom-test: Traverse entire QOM tree

[Steven Sistare]

On 7/25/2025 9:50 AM, Markus Armbruster wrote:
> This test traverses the QOM sub-tree rooted at /machine.  Traverse the
> entire tree instead.
> 
> The x86_64 test runs some 40 additional QMP commands, and stays under
> 5s for me.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/qtest/qom-test.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/qom-test.c b/tests/qtest/qom-test.c
> index 40bdc3639f..d358b69c7e 100644
> --- a/tests/qtest/qom-test.c
> +++ b/tests/qtest/qom-test.c
> @@ -211,7 +211,7 @@ static void test_machine(gconstpointer data)
>   
>       test_properties(qts, "/machine", true);
>   
> -    qlist_append_str(paths, "/machine");
> +    qlist_append_str(paths, "/");
>       test_list_get(qts, paths);
>       test_list_get_value(qts);

Reviewed-by: Steve Sistare <steven.sistare@oracle.com>

[PATCH 4/5] qtest/qom-test: Don't bother to execute QMP command quit

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/qtest/qom-test.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/tests/qtest/qom-test.c b/tests/qtest/qom-test.c
index d358b69c7e..6421f2d9d9 100644
--- a/tests/qtest/qom-test.c
+++ b/tests/qtest/qom-test.c
@@ -215,10 +215,6 @@ static void test_machine(gconstpointer data)
     test_list_get(qts, paths);
     test_list_get_value(qts);
 
-    response = qtest_qmp(qts, "{ 'execute': 'quit' }");
-    g_assert(qdict_haskey(response, "return"));
-    qobject_unref(response);
-
     qtest_quit(qts);
 }
 
-- 
2.49.0

Re: [PATCH 4/5] qtest/qom-test: Don't bother to execute QMP command quit

[Steven Sistare]

On 7/25/2025 9:50 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/qtest/qom-test.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/tests/qtest/qom-test.c b/tests/qtest/qom-test.c
> index d358b69c7e..6421f2d9d9 100644
> --- a/tests/qtest/qom-test.c
> +++ b/tests/qtest/qom-test.c
> @@ -215,10 +215,6 @@ static void test_machine(gconstpointer data)
>       test_list_get(qts, paths);
>       test_list_get_value(qts);
>   
> -    response = qtest_qmp(qts, "{ 'execute': 'quit' }");
> -    g_assert(qdict_haskey(response, "return"));
> -    qobject_unref(response);
> -
>       qtest_quit(qts);
>   }

IMO the quit command improves test coverage, albeit by a small amount.
It guarantees that qemu did not die after returning the qom result to
the client.

- Steve

Re: [PATCH 4/5] qtest/qom-test: Don't bother to execute QMP command quit

[Markus Armbruster]

Steven Sistare <steven.sistare@oracle.com> writes:

> On 7/25/2025 9:50 AM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   tests/qtest/qom-test.c | 4 ----
>>   1 file changed, 4 deletions(-)
>> diff --git a/tests/qtest/qom-test.c b/tests/qtest/qom-test.c
>> index d358b69c7e..6421f2d9d9 100644
>> --- a/tests/qtest/qom-test.c
>> +++ b/tests/qtest/qom-test.c
>> @@ -215,10 +215,6 @@ static void test_machine(gconstpointer data)
>>      test_list_get(qts, paths);
>>      test_list_get_value(qts);
>> -    response = qtest_qmp(qts, "{ 'execute': 'quit' }");
>> -    g_assert(qdict_haskey(response, "return"));
>> -    qobject_unref(response);
>> -
>>      qtest_quit(qts);
>>   }
>
> IMO the quit command improves test coverage, albeit by a small amount.
> It guarantees that qemu did not die after returning the qom result to
> the client.

What if it dies afte returning the quit response?

Detecting QEMU dying on us is the test harness's job.  Check out
qtest_check_status() called by qtest_wait_qemu() called by
qtest_kill_qemu() called by qtest_quit() called by the test.

For what it's worth, the only other qtest using the quit command is
machine-none-test.c.

In qtests, quit races with the test harness's termination of QEMU.  The
quit command requests immediate shutdown.  By the time the test receives
the response, the QEMU process may be alive (still shutting down) or
dead.  If dead, it's in zombie state.  qtest_quit() then kill()s it some
more (does nothing for zombies), and finally reaps it with waitpid().
Works, but the race between quit and kill give me a queasy feeling.

Can't say whether the Windows code handles this robustly.

Re: [PATCH 4/5] qtest/qom-test: Don't bother to execute QMP command quit

[Steven Sistare]

On 8/5/2025 2:06 AM, Markus Armbruster wrote:
> Steven Sistare <steven.sistare@oracle.com> writes:
>> On 7/25/2025 9:50 AM, Markus Armbruster wrote:
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>    tests/qtest/qom-test.c | 4 ----
>>>    1 file changed, 4 deletions(-)
>>> diff --git a/tests/qtest/qom-test.c b/tests/qtest/qom-test.c
>>> index d358b69c7e..6421f2d9d9 100644
>>> --- a/tests/qtest/qom-test.c
>>> +++ b/tests/qtest/qom-test.c
>>> @@ -215,10 +215,6 @@ static void test_machine(gconstpointer data)
>>>       test_list_get(qts, paths);
>>>       test_list_get_value(qts);
>>> -    response = qtest_qmp(qts, "{ 'execute': 'quit' }");
>>> -    g_assert(qdict_haskey(response, "return"));
>>> -    qobject_unref(response);
>>> -
>>>       qtest_quit(qts);
>>>    }
>>
>> IMO the quit command improves test coverage, albeit by a small amount.
>> It guarantees that qemu did not die after returning the qom result to
>> the client.
> 
> What if it dies afte returning the quit response?
> 
> Detecting QEMU dying on us is the test harness's job.  Check out
> qtest_check_status() called by qtest_wait_qemu() called by
> qtest_kill_qemu() called by qtest_quit() called by the test.
> 
> For what it's worth, the only other qtest using the quit command is
> machine-none-test.c.
> 
> In qtests, quit races with the test harness's termination of QEMU.  The
> quit command requests immediate shutdown.  By the time the test receives
> the response, the QEMU process may be alive (still shutting down) or
> dead.  If dead, it's in zombie state.  qtest_quit() then kill()s it some
> more (does nothing for zombies), and finally reaps it with waitpid().
> Works, but the race between quit and kill give me a queasy feeling.
> 
> Can't say whether the Windows code handles this robustly.

OK.

Reviewed-by: Steve Sistare <steven.sistare@oracle.com>

[PATCH 5/5] MAINTAINERS: Cover tests/qtest/qom-test.c

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index a462345618..12e7767909 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3445,6 +3445,7 @@ F: qom/
 F: tests/unit/check-qom-interface.c
 F: tests/unit/check-qom-proplist.c
 F: tests/unit/test-qdev-global-props.c
+F: tests/qtest/qom-test.c
 
 QOM boilerplate conversion script
 M: Eduardo Habkost <eduardo@habkost.net>
-- 
2.49.0

Re: [PATCH 0/5] qtest/qom-test: Leak plug, faster, better coverage

[Markus Armbruster]

Steven, this is follow-up to your work.  Care to review?