Re: [PATCH v2] tests: qmp-cmd-test: fix memory leak

[Markus Armbruster]

Li Qiang <liq3ea@gmail.com> writes:

> Markus Armbruster <armbru@redhat.com> 于2020年7月16日周四 下午1:59写道：
>>
>> Li Qiang <liq3ea@163.com> writes:
>>
>> > Properly free each test response to avoid memory leak and separate
>> > qtest_qmp() calls with spare lines, in a consistent manner.
>> >
>> > Fixes: 5b88849e7b9("tests/qmp-cmd-test: Add
>> > qmp/object-add-failure-modes"
>>
>> The patch also fixes leaks introduced in 442b09b83d and 9fc719b869,
>> actually.  At least it should, but the patch appears to be incomplete.

442b09b83d was fine, actually.

9fc719b869 wasn't, and your second patch hunk fixes it.  Please add a
"Fixes: 9fc719b869' line to the commit message.

>> >
>> > Reviewed-by: Eric Auger <eric.auger@redhat.com>
>> > Signed-off-by: Li Qiang <liq3ea@163.com>
>> > ---
>> > Change sincve v1: add detailed commit message
>> >
>> > tests/qtest/qmp-cmd-test.c | 13 +++++++++++++
>> >  1 file changed, 13 insertions(+)
>> >
>> > diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
>> > index c68f99f659..f7b1aa7fdc 100644
>> > --- a/tests/qtest/qmp-cmd-test.c
>> > +++ b/tests/qtest/qmp-cmd-test.c
>> > @@ -230,6 +230,8 @@ static void test_object_add_failure_modes(void)
>>    static void test_object_add_failure_modes(void)
>>    {
>>        QTestState *qts;
>>        QDict *resp;
>>
>>        /* attempt to create an object without props */
>>        qts = qtest_init(common_args);
>>        resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
>>                         " {'qom-type': 'memory-backend-ram', 'id': 'ram1' } 
>> }");
>>        g_assert_nonnull(resp);
>>        qmp_assert_error_class(resp, "GenericError");
>>
>> Doesn't @resp leak here, too?
>
> No, qmp_assert_error_class will call qobject_unref(rsp) will so will not leak.

You're right.

With the additional Fixes:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

> In fact, I think this is a inconsistent for 'qtest_qmp'.
> I think we can apply this patch first and then change the
> 'qmp_assert_error_class' or/and others
> to free resp. And just let the caller of 'qtest_qmp' frees unref the rsp.

Do you mean "not to free @resp"?

> What's your idea?

Rename it to qmp_expect_error_and_unref()?