* [PATCH v2] tests: qmp-cmd-test: fix memory leak
@ 2020-07-15 15:41 Li Qiang
2020-07-16 5:59 ` Markus Armbruster
2020-07-16 7:56 ` Thomas Huth
0 siblings, 2 replies; 7+ messages in thread
From: Li Qiang @ 2020-07-15 15:41 UTC (permalink / raw)
To: armbru, thuth, lvivier, pbonzini, eric.auger; +Cc: Li Qiang, liq3ea, qemu-devel
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"
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)
" 'props': {'size': 1048576 } } }");
g_assert_nonnull(resp);
g_assert(qdict_haskey(resp, "return"));
+ qobject_unref(resp);
+
resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
" {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
" 'props': {'size': 1048576 } } }");
@@ -241,6 +243,7 @@ static void test_object_add_failure_modes(void)
" {'id': 'ram1' } }");
g_assert_nonnull(resp);
g_assert(qdict_haskey(resp, "return"));
+ qobject_unref(resp);
/* attempt to create an object with a property of a wrong type */
resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
@@ -249,17 +252,20 @@ static void test_object_add_failure_modes(void)
g_assert_nonnull(resp);
/* now do it right */
qmp_assert_error_class(resp, "GenericError");
+
resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
" {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
" 'props': {'size': 1048576 } } }");
g_assert_nonnull(resp);
g_assert(qdict_haskey(resp, "return"));
+ qobject_unref(resp);
/* delete ram1 object */
resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':"
" {'id': 'ram1' } }");
g_assert_nonnull(resp);
g_assert(qdict_haskey(resp, "return"));
+ qobject_unref(resp);
/* attempt to create an object without the id */
resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
@@ -267,18 +273,21 @@ static void test_object_add_failure_modes(void)
" 'props': {'size': 1048576 } } }");
g_assert_nonnull(resp);
qmp_assert_error_class(resp, "GenericError");
+
/* now do it right */
resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
" {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
" 'props': {'size': 1048576 } } }");
g_assert_nonnull(resp);
g_assert(qdict_haskey(resp, "return"));
+ qobject_unref(resp);
/* delete ram1 object */
resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':"
" {'id': 'ram1' } }");
g_assert_nonnull(resp);
g_assert(qdict_haskey(resp, "return"));
+ qobject_unref(resp);
/* attempt to set a non existing property */
resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
@@ -286,23 +295,27 @@ static void test_object_add_failure_modes(void)
" 'props': {'sized': 1048576 } } }");
g_assert_nonnull(resp);
qmp_assert_error_class(resp, "GenericError");
+
/* now do it right */
resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
" {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
" 'props': {'size': 1048576 } } }");
g_assert_nonnull(resp);
g_assert(qdict_haskey(resp, "return"));
+ qobject_unref(resp);
/* delete ram1 object without id */
resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':"
" {'ida': 'ram1' } }");
g_assert_nonnull(resp);
+ qobject_unref(resp);
/* delete ram1 object */
resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':"
" {'id': 'ram1' } }");
g_assert_nonnull(resp);
g_assert(qdict_haskey(resp, "return"));
+ qobject_unref(resp);
/* delete ram1 object that does not exist anymore*/
resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':"
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] tests: qmp-cmd-test: fix memory leak
2020-07-15 15:41 [PATCH v2] tests: qmp-cmd-test: fix memory leak Li Qiang
@ 2020-07-16 5:59 ` Markus Armbruster
2020-07-16 6:43 ` Li Qiang
2020-07-16 7:56 ` Thomas Huth
1 sibling, 1 reply; 7+ messages in thread
From: Markus Armbruster @ 2020-07-16 5:59 UTC (permalink / raw)
To: Li Qiang; +Cc: lvivier, thuth, liq3ea, qemu-devel, eric.auger, pbonzini
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.
>
> 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?
/* attempt to create an object without qom-type */
resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
" {'id': 'ram1' } }");
g_assert_nonnull(resp);
qmp_assert_error_class(resp, "GenericError");
Likewise.
/* attempt to delete an object that does not exist */
resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':"
" {'id': 'ram1' } }");
g_assert_nonnull(resp);
qmp_assert_error_class(resp, "GenericError");
Likewise.
/* attempt to create 2 objects with duplicate id */
resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
" {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
> " 'props': {'size': 1048576 } } }");
> g_assert_nonnull(resp);
> g_assert(qdict_haskey(resp, "return"));
> + qobject_unref(resp);
> +
> resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
> " {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
> " 'props': {'size': 1048576 } } }");
g_assert_nonnull(resp);
qmp_assert_error_class(resp, "GenericError");
Likewise.
/* delete ram1 object */
resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':"
> @@ -241,6 +243,7 @@ static void test_object_add_failure_modes(void)
> " {'id': 'ram1' } }");
> g_assert_nonnull(resp);
> g_assert(qdict_haskey(resp, "return"));
> + qobject_unref(resp);
>
> /* attempt to create an object with a property of a wrong type */
> resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
" {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
" 'props': {'size': '1048576' } } }");
> @@ -249,17 +252,20 @@ static void test_object_add_failure_modes(void)
> g_assert_nonnull(resp);
> /* now do it right */
> qmp_assert_error_class(resp, "GenericError");
Likewise.
> +
> resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
> " {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
> " 'props': {'size': 1048576 } } }");
> g_assert_nonnull(resp);
> g_assert(qdict_haskey(resp, "return"));
> + qobject_unref(resp);
>
> /* delete ram1 object */
> resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':"
> " {'id': 'ram1' } }");
> g_assert_nonnull(resp);
> g_assert(qdict_haskey(resp, "return"));
> + qobject_unref(resp);
>
> /* attempt to create an object without the id */
> resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
" {'qom-type': 'memory-backend-ram',"
> @@ -267,18 +273,21 @@ static void test_object_add_failure_modes(void)
> " 'props': {'size': 1048576 } } }");
> g_assert_nonnull(resp);
> qmp_assert_error_class(resp, "GenericError");
Likewise.
> +
> /* now do it right */
> resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
> " {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
> " 'props': {'size': 1048576 } } }");
> g_assert_nonnull(resp);
> g_assert(qdict_haskey(resp, "return"));
> + qobject_unref(resp);
>
> /* delete ram1 object */
> resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':"
> " {'id': 'ram1' } }");
> g_assert_nonnull(resp);
> g_assert(qdict_haskey(resp, "return"));
> + qobject_unref(resp);
>
> /* attempt to set a non existing property */
> resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
" {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
> @@ -286,23 +295,27 @@ static void test_object_add_failure_modes(void)
> " 'props': {'sized': 1048576 } } }");
> g_assert_nonnull(resp);
> qmp_assert_error_class(resp, "GenericError");
Likewise.
> +
> /* now do it right */
> resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
> " {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
> " 'props': {'size': 1048576 } } }");
> g_assert_nonnull(resp);
> g_assert(qdict_haskey(resp, "return"));
> + qobject_unref(resp);
>
> /* delete ram1 object without id */
> resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':"
> " {'ida': 'ram1' } }");
> g_assert_nonnull(resp);
> + qobject_unref(resp);
>
> /* delete ram1 object */
> resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':"
> " {'id': 'ram1' } }");
> g_assert_nonnull(resp);
> g_assert(qdict_haskey(resp, "return"));
> + qobject_unref(resp);
>
> /* delete ram1 object that does not exist anymore*/
> resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':"
" {'id': 'ram1' } }");
g_assert_nonnull(resp);
qmp_assert_error_class(resp, "GenericError");
Likewise.
qtest_quit(qts);
}
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] tests: qmp-cmd-test: fix memory leak
2020-07-16 5:59 ` Markus Armbruster
@ 2020-07-16 6:43 ` Li Qiang
2020-07-16 9:52 ` Markus Armbruster
0 siblings, 1 reply; 7+ messages in thread
From: Li Qiang @ 2020-07-16 6:43 UTC (permalink / raw)
To: Markus Armbruster
Cc: Laurent Vivier, Thomas Huth, Li Qiang, Qemu Developers,
Auger Eric, Paolo Bonzini
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.
>
> >
> > 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.
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.
What's your idea?
Thanks,
Li Qiang
>
> /* attempt to create an object without qom-type */
> resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
> " {'id': 'ram1' } }");
> g_assert_nonnull(resp);
> qmp_assert_error_class(resp, "GenericError");
>
> Likewise.
>
> /* attempt to delete an object that does not exist */
> resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':"
> " {'id': 'ram1' } }");
> g_assert_nonnull(resp);
> qmp_assert_error_class(resp, "GenericError");
>
> Likewise.
>
> /* attempt to create 2 objects with duplicate id */
> resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
> " {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
> > " 'props': {'size': 1048576 } } }");
> > g_assert_nonnull(resp);
> > g_assert(qdict_haskey(resp, "return"));
> > + qobject_unref(resp);
> > +
> > resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
> > " {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
> > " 'props': {'size': 1048576 } } }");
> g_assert_nonnull(resp);
> qmp_assert_error_class(resp, "GenericError");
>
> Likewise.
>
> /* delete ram1 object */
> resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':"
> > @@ -241,6 +243,7 @@ static void test_object_add_failure_modes(void)
> > " {'id': 'ram1' } }");
> > g_assert_nonnull(resp);
> > g_assert(qdict_haskey(resp, "return"));
> > + qobject_unref(resp);
> >
> > /* attempt to create an object with a property of a wrong type */
> > resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
> " {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
> " 'props': {'size': '1048576' } } }");
> > @@ -249,17 +252,20 @@ static void test_object_add_failure_modes(void)
> > g_assert_nonnull(resp);
> > /* now do it right */
> > qmp_assert_error_class(resp, "GenericError");
>
> Likewise.
>
> > +
> > resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
> > " {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
> > " 'props': {'size': 1048576 } } }");
> > g_assert_nonnull(resp);
> > g_assert(qdict_haskey(resp, "return"));
> > + qobject_unref(resp);
> >
> > /* delete ram1 object */
> > resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':"
> > " {'id': 'ram1' } }");
> > g_assert_nonnull(resp);
> > g_assert(qdict_haskey(resp, "return"));
> > + qobject_unref(resp);
> >
> > /* attempt to create an object without the id */
> > resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
> " {'qom-type': 'memory-backend-ram',"
> > @@ -267,18 +273,21 @@ static void test_object_add_failure_modes(void)
> > " 'props': {'size': 1048576 } } }");
> > g_assert_nonnull(resp);
> > qmp_assert_error_class(resp, "GenericError");
>
> Likewise.
>
> > +
> > /* now do it right */
> > resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
> > " {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
> > " 'props': {'size': 1048576 } } }");
> > g_assert_nonnull(resp);
> > g_assert(qdict_haskey(resp, "return"));
> > + qobject_unref(resp);
> >
> > /* delete ram1 object */
> > resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':"
> > " {'id': 'ram1' } }");
> > g_assert_nonnull(resp);
> > g_assert(qdict_haskey(resp, "return"));
> > + qobject_unref(resp);
> >
> > /* attempt to set a non existing property */
> > resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
> " {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
> > @@ -286,23 +295,27 @@ static void test_object_add_failure_modes(void)
> > " 'props': {'sized': 1048576 } } }");
> > g_assert_nonnull(resp);
> > qmp_assert_error_class(resp, "GenericError");
>
> Likewise.
>
> > +
> > /* now do it right */
> > resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
> > " {'qom-type': 'memory-backend-ram', 'id': 'ram1',"
> > " 'props': {'size': 1048576 } } }");
> > g_assert_nonnull(resp);
> > g_assert(qdict_haskey(resp, "return"));
> > + qobject_unref(resp);
> >
> > /* delete ram1 object without id */
> > resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':"
> > " {'ida': 'ram1' } }");
> > g_assert_nonnull(resp);
> > + qobject_unref(resp);
> >
> > /* delete ram1 object */
> > resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':"
> > " {'id': 'ram1' } }");
> > g_assert_nonnull(resp);
> > g_assert(qdict_haskey(resp, "return"));
> > + qobject_unref(resp);
> >
> > /* delete ram1 object that does not exist anymore*/
> > resp = qtest_qmp(qts, "{'execute': 'object-del', 'arguments':"
> " {'id': 'ram1' } }");
> g_assert_nonnull(resp);
> qmp_assert_error_class(resp, "GenericError");
>
> Likewise.
>
> qtest_quit(qts);
> }
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] tests: qmp-cmd-test: fix memory leak
2020-07-15 15:41 [PATCH v2] tests: qmp-cmd-test: fix memory leak Li Qiang
2020-07-16 5:59 ` Markus Armbruster
@ 2020-07-16 7:56 ` Thomas Huth
1 sibling, 0 replies; 7+ messages in thread
From: Thomas Huth @ 2020-07-16 7:56 UTC (permalink / raw)
To: Li Qiang, armbru, lvivier, pbonzini, eric.auger; +Cc: liq3ea, qemu-devel
On 15/07/2020 17.41, Li Qiang wrote:
> 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"
>
> 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(+)
Thanks, queued to https://gitlab.com/huth/qemu/-/commits/qtest-next/
Thomas
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] tests: qmp-cmd-test: fix memory leak
2020-07-16 6:43 ` Li Qiang
@ 2020-07-16 9:52 ` Markus Armbruster
2020-07-16 10:33 ` Li Qiang
0 siblings, 1 reply; 7+ messages in thread
From: Markus Armbruster @ 2020-07-16 9:52 UTC (permalink / raw)
To: Li Qiang
Cc: Laurent Vivier, Thomas Huth, Li Qiang, Qemu Developers,
Auger Eric, Paolo Bonzini
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()?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] tests: qmp-cmd-test: fix memory leak
2020-07-16 9:52 ` Markus Armbruster
@ 2020-07-16 10:33 ` Li Qiang
2020-07-16 10:47 ` Thomas Huth
0 siblings, 1 reply; 7+ messages in thread
From: Li Qiang @ 2020-07-16 10:33 UTC (permalink / raw)
To: Markus Armbruster
Cc: Laurent Vivier, Thomas Huth, Li Qiang, Qemu Developers,
Auger Eric, Paolo Bonzini
Markus Armbruster <armbru@redhat.com> 于2020年7月16日周四 下午5:52写道：
>
> 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.
Hi Thomas,
Could you do this minor adjustment?
Add also add Markus's r-b tag.
>
> >> >
> >> > 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"?
Yes, the 'qmp_assert_error_class' not free @resp.
>
> > What's your idea?
>
> Rename it to qmp_expect_error_and_unref()?
In fact I prefer qmp_assert_error_class has no reason unref @resp.
So the user of ''qtest_qmp' will has a unify processing of the @resp.
resp = qtest_qmp()
use resp
free resp.
If we just rename to 'qmp_expect_error_and_unref'.
The user still should remember this. If not they will have UAF or mem
leak issue.
Thanks,
Li Qiang
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] tests: qmp-cmd-test: fix memory leak
2020-07-16 10:33 ` Li Qiang
@ 2020-07-16 10:47 ` Thomas Huth
0 siblings, 0 replies; 7+ messages in thread
From: Thomas Huth @ 2020-07-16 10:47 UTC (permalink / raw)
To: Li Qiang, Markus Armbruster
Cc: Laurent Vivier, Auger Eric, Li Qiang, Qemu Developers, Paolo Bonzini
On 16/07/2020 12.33, Li Qiang wrote:
> Markus Armbruster <armbru@redhat.com> 于2020年7月16日周四 下午5:52写道：
>>
>> 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.
>
> Hi Thomas,
>
> Could you do this minor adjustment?
> Add also add Markus's r-b tag.
Sure, I'll add it!
Thomas
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-07-16 10:47 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-15 15:41 [PATCH v2] tests: qmp-cmd-test: fix memory leak Li Qiang
2020-07-16 5:59 ` Markus Armbruster
2020-07-16 6:43 ` Li Qiang
2020-07-16 9:52 ` Markus Armbruster
2020-07-16 10:33 ` Li Qiang
2020-07-16 10:47 ` Thomas Huth
2020-07-16 7:56 ` Thomas Huth
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.