Markus Armbruster <arm...@redhat.com> 于2020年7月16日周四 下午5:52写道：
>
> Li Qiang <liq...@gmail.com> writes:
>
> > Markus Armbruster <arm...@redhat.com> 于2020年7月16日周四 下午1:59写道：
> >>
> >> Li Qiang <liq...@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.au...@redhat.com>
> >> > Signed-off-by: Li Qiang <liq...@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 <arm...@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
>
