[Qemu-devel] [PATCH] test-keyval: fix leaks

[Qemu-devel] [PATCH] test-keyval: fix leaks

[Marc-André Lureau]

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/test-keyval.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index ba19560a22..141ee5d0c4 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -628,6 +628,7 @@ static void test_keyval_visit_alternate(void)
     visit_type_AltNumStr(v, "a", &ans, &error_abort);
     g_assert_cmpint(ans->type, ==, QTYPE_QSTRING);
     g_assert_cmpstr(ans->u.s, ==, "1");
+    qapi_free_AltNumStr(ans);
     visit_type_AltNumInt(v, "a", &ani, &err);
     error_free_or_abort(&err);
     visit_end_struct(v, NULL);
@@ -649,11 +650,14 @@ static void test_keyval_visit_any(void)
     visit_type_any(v, "a", &any, &error_abort);
     qlist = qobject_to_qlist(any);
     g_assert(qlist);
+    qobject_decref(any);
     qstr = qobject_to_qstring(qlist_pop(qlist));
     g_assert_cmpstr(qstring_get_str(qstr), ==, "null");
+    QDECREF(qstr);
     qstr = qobject_to_qstring(qlist_pop(qlist));
     g_assert_cmpstr(qstring_get_str(qstr), ==, "1");
     g_assert(qlist_empty(qlist));
+    QDECREF(qstr);
     visit_check_struct(v, &error_abort);
     visit_end_struct(v, NULL);
     visit_free(v);
-- 
2.12.0.191.gc5d8de91d

Re: [Qemu-devel] [PATCH] test-keyval: fix leaks

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 512 bytes --]

On 04/10/2017 09:21 AM, Marc-André Lureau wrote:
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/test-keyval.c | 4 ++++
>  1 file changed, 4 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

Leaks in the testsuite are not showstoppers, so I'm okay if we defer it
to 2.10 to minimize churn now that we are late in hard freeze.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] test-keyval: fix leaks

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/test-keyval.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/tests/test-keyval.c b/tests/test-keyval.c
> index ba19560a22..141ee5d0c4 100644
> --- a/tests/test-keyval.c
> +++ b/tests/test-keyval.c
> @@ -628,6 +628,7 @@ static void test_keyval_visit_alternate(void)
>      visit_type_AltNumStr(v, "a", &ans, &error_abort);
>      g_assert_cmpint(ans->type, ==, QTYPE_QSTRING);
>      g_assert_cmpstr(ans->u.s, ==, "1");
> +    qapi_free_AltNumStr(ans);
>      visit_type_AltNumInt(v, "a", &ani, &err);
>      error_free_or_abort(&err);
>      visit_end_struct(v, NULL);
> @@ -649,11 +650,14 @@ static void test_keyval_visit_any(void)
>      visit_type_any(v, "a", &any, &error_abort);

@any becomes a strong reference (qobject_input_type_any() increments the
reference count).

>      qlist = qobject_to_qlist(any);

Reference count unchanged.

>      g_assert(qlist);
> +    qobject_decref(any);

Uh, this is unnecessarily dirty: you relinquish the reference before
you're actually done with it.  Works only because there's *another*
reference hiding within @v.  Let's move this ...

>      qstr = qobject_to_qstring(qlist_pop(qlist));
>      g_assert_cmpstr(qstring_get_str(qstr), ==, "null");
> +    QDECREF(qstr);
>      qstr = qobject_to_qstring(qlist_pop(qlist));
>      g_assert_cmpstr(qstring_get_str(qstr), ==, "1");
>      g_assert(qlist_empty(qlist));
> +    QDECREF(qstr);

... here.  Okay to do that on commit?

>      visit_check_struct(v, &error_abort);
>      visit_end_struct(v, NULL);
>      visit_free(v);

With the reference counting cleaned up:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH] test-keyval: fix leaks

[Marc-André Lureau]

Hi

On Wed, Apr 12, 2017 at 3:19 PM Markus Armbruster <armbru@redhat.com> wrote:

> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  tests/test-keyval.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/tests/test-keyval.c b/tests/test-keyval.c
> > index ba19560a22..141ee5d0c4 100644
> > --- a/tests/test-keyval.c
> > +++ b/tests/test-keyval.c
> > @@ -628,6 +628,7 @@ static void test_keyval_visit_alternate(void)
> >      visit_type_AltNumStr(v, "a", &ans, &error_abort);
> >      g_assert_cmpint(ans->type, ==, QTYPE_QSTRING);
> >      g_assert_cmpstr(ans->u.s, ==, "1");
> > +    qapi_free_AltNumStr(ans);
> >      visit_type_AltNumInt(v, "a", &ani, &err);
> >      error_free_or_abort(&err);
> >      visit_end_struct(v, NULL);
> > @@ -649,11 +650,14 @@ static void test_keyval_visit_any(void)
> >      visit_type_any(v, "a", &any, &error_abort);
>
> @any becomes a strong reference (qobject_input_type_any() increments the
> reference count).
>
> >      qlist = qobject_to_qlist(any);
>
> Reference count unchanged.
>
> >      g_assert(qlist);
> > +    qobject_decref(any);
>
> Uh, this is unnecessarily dirty: you relinquish the reference before
> you're actually done with it.  Works only because there's *another*
> reference hiding within @v.  Let's move this ...
>
> >      qstr = qobject_to_qstring(qlist_pop(qlist));
> >      g_assert_cmpstr(qstring_get_str(qstr), ==, "null");
> > +    QDECREF(qstr);
> >      qstr = qobject_to_qstring(qlist_pop(qlist));
> >      g_assert_cmpstr(qstring_get_str(qstr), ==, "1");
> >      g_assert(qlist_empty(qlist));
> > +    QDECREF(qstr);
>
> ... here.  Okay to do that on commit?
>

 sure, makes sense


> >      visit_check_struct(v, &error_abort);
> >      visit_end_struct(v, NULL);
> >      visit_free(v);
>
> With the reference counting cleaned up:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
>
thanks
-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] test-keyval: fix leaks

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Wed, Apr 12, 2017 at 3:19 PM Markus Armbruster <armbru@redhat.com> wrote:
>
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> >  tests/test-keyval.c | 4 ++++
>> >  1 file changed, 4 insertions(+)
>> >
>> > diff --git a/tests/test-keyval.c b/tests/test-keyval.c
>> > index ba19560a22..141ee5d0c4 100644
>> > --- a/tests/test-keyval.c
>> > +++ b/tests/test-keyval.c
>> > @@ -628,6 +628,7 @@ static void test_keyval_visit_alternate(void)
>> >      visit_type_AltNumStr(v, "a", &ans, &error_abort);
>> >      g_assert_cmpint(ans->type, ==, QTYPE_QSTRING);
>> >      g_assert_cmpstr(ans->u.s, ==, "1");
>> > +    qapi_free_AltNumStr(ans);
>> >      visit_type_AltNumInt(v, "a", &ani, &err);
>> >      error_free_or_abort(&err);
>> >      visit_end_struct(v, NULL);
>> > @@ -649,11 +650,14 @@ static void test_keyval_visit_any(void)
>> >      visit_type_any(v, "a", &any, &error_abort);
>>
>> @any becomes a strong reference (qobject_input_type_any() increments the
>> reference count).
>>
>> >      qlist = qobject_to_qlist(any);
>>
>> Reference count unchanged.
>>
>> >      g_assert(qlist);
>> > +    qobject_decref(any);
>>
>> Uh, this is unnecessarily dirty: you relinquish the reference before
>> you're actually done with it.  Works only because there's *another*
>> reference hiding within @v.  Let's move this ...
>>
>> >      qstr = qobject_to_qstring(qlist_pop(qlist));
>> >      g_assert_cmpstr(qstring_get_str(qstr), ==, "null");
>> > +    QDECREF(qstr);
>> >      qstr = qobject_to_qstring(qlist_pop(qlist));
>> >      g_assert_cmpstr(qstring_get_str(qstr), ==, "1");
>> >      g_assert(qlist_empty(qlist));
>> > +    QDECREF(qstr);
>>
>> ... here.  Okay to do that on commit?
>>
>
>  sure, makes sense
>
>
>> >      visit_check_struct(v, &error_abort);
>> >      visit_end_struct(v, NULL);
>> >      visit_free(v);
>>
>> With the reference counting cleaned up:
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>
>>
> thanks

Applied to qapi-next, thanks!