[Qemu-devel] [PATCH 0/2] qmp output visitor test improvement

[Qemu-devel] [PATCH 0/2] qmp output visitor test improvement

[Eric Blake]

Given the threads last week with Pino's claim that we had a
memory leak, and Markus' counter-claim that the proposed
patch was reverting an earlier double-free fix, I ended up
enhancing the testsuite to prove under valgrind whether we
indeed have a problem.  The good news is that the existing
visitor code is correct, but having more tests will help
prevent future regression attempts :)

Eric Blake (2):
  tests: Simplify expected error checking for qmp output
  tests: Enhance qmp output to cover partial visit

 tests/test-qmp-output-visitor.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/2] tests: Simplify expected error checking for qmp output

[Eric Blake]

Minor test cleanup noticed while writing a new test.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/test-qmp-output-visitor.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 513d71f..f7213d2 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -141,13 +141,11 @@ static void test_visitor_out_enum_errors(TestOutputVisitorData *data,
                                          const void *unused)
 {
     EnumOne i, bad_values[] = { ENUM_ONE__MAX, -1 };
-    Error *err;
+    Error *err = NULL;

     for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
-        err = NULL;
         visit_type_EnumOne(data->ov, "unused", &bad_values[i], &err);
-        g_assert(err);
-        error_free(err);
+        error_free_or_abort(&err);
         visitor_reset(data);
     }
 }
@@ -243,16 +241,14 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
     EnumOne bad_values[] = { ENUM_ONE__MAX, -1 };
     UserDefOne u = {0};
     UserDefOne *pu = &u;
-    Error *err;
+    Error *err = NULL;
     int i;

     for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
-        err = NULL;
         u.has_enum1 = true;
         u.enum1 = bad_values[i];
         visit_type_UserDefOne(data->ov, "unused", &pu, &err);
-        g_assert(err);
-        error_free(err);
+        error_free_or_abort(&err);
         visitor_reset(data);
     }
 }
-- 
2.7.4

[Qemu-devel] [PATCH 2/2] tests: Enhance qmp output to cover partial visit

[Eric Blake]

Add a test that proves (at least when run under valgrind) that
we are correctly handling allocated memory even when a visit
is aborted in the middle for whatever other reason.

See commit f24582d "qapi: fix double free in
qmp_output_visitor_cleanup()" for a fix that was lacking
testsuite exposure prior to this patch.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/test-qmp-output-visitor.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index f7213d2..2368171 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -254,6 +254,16 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
 }


+static void test_visitor_out_partial_visit(TestOutputVisitorData *data,
+                                           const void *unused)
+{
+    /* Check that aborting mid-visit doesn't leak or double-free */
+    visit_start_struct(data->ov, NULL, NULL, 0, &error_abort);
+    visit_start_struct(data->ov, "nested", NULL, 0, &error_abort);
+    visitor_reset(data);
+}
+
+
 static void test_visitor_out_list(TestOutputVisitorData *data,
                                   const void *unused)
 {
@@ -817,6 +827,8 @@ int main(int argc, char **argv)
                             &out_visitor_data, test_visitor_out_struct_nested);
     output_visitor_test_add("/visitor/output/struct-errors",
                             &out_visitor_data, test_visitor_out_struct_errors);
+    output_visitor_test_add("/visitor/output/partial-visit",
+                            &out_visitor_data, test_visitor_out_partial_visit);
     output_visitor_test_add("/visitor/output/list",
                             &out_visitor_data, test_visitor_out_list);
     output_visitor_test_add("/visitor/output/any",
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/2] tests: Enhance qmp output to cover partial visit

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Add a test that proves (at least when run under valgrind) that
> we are correctly handling allocated memory even when a visit
> is aborted in the middle for whatever other reason.
>
> See commit f24582d "qapi: fix double free in
> qmp_output_visitor_cleanup()" for a fix that was lacking
> testsuite exposure prior to this patch.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/test-qmp-output-visitor.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
> index f7213d2..2368171 100644
> --- a/tests/test-qmp-output-visitor.c
> +++ b/tests/test-qmp-output-visitor.c
> @@ -254,6 +254,16 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
>  }
>
>
> +static void test_visitor_out_partial_visit(TestOutputVisitorData *data,
> +                                           const void *unused)
> +{
> +    /* Check that aborting mid-visit doesn't leak or double-free */
> +    visit_start_struct(data->ov, NULL, NULL, 0, &error_abort);
> +    visit_start_struct(data->ov, "nested", NULL, 0, &error_abort);
> +    visitor_reset(data);

Add a second test that additionally visits a member?  Or is visiting the
"nested" member enough?

> +}
> +
> +
>  static void test_visitor_out_list(TestOutputVisitorData *data,
>                                    const void *unused)
>  {
> @@ -817,6 +827,8 @@ int main(int argc, char **argv)
>                              &out_visitor_data, test_visitor_out_struct_nested);
>      output_visitor_test_add("/visitor/output/struct-errors",
>                              &out_visitor_data, test_visitor_out_struct_errors);
> +    output_visitor_test_add("/visitor/output/partial-visit",
> +                            &out_visitor_data, test_visitor_out_partial_visit);
>      output_visitor_test_add("/visitor/output/list",
>                              &out_visitor_data, test_visitor_out_list);
>      output_visitor_test_add("/visitor/output/any",

Would you be willing to do the same for lists and alternates?

Speaking of alternates, visitor.h's virtual walk example covers struct
and list, but not alternate.

Re: [Qemu-devel] [PATCH 1/2] tests: Simplify expected error checking for qmp output

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Minor test cleanup noticed while writing a new test.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 2/2] tests: Enhance qmp output to cover partial visit

[Eric Blake]

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

On 10/25/2016 06:07 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Add a test that proves (at least when run under valgrind) that
>> we are correctly handling allocated memory even when a visit
>> is aborted in the middle for whatever other reason.
>>
>> See commit f24582d "qapi: fix double free in
>> qmp_output_visitor_cleanup()" for a fix that was lacking
>> testsuite exposure prior to this patch.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>

>> +static void test_visitor_out_partial_visit(TestOutputVisitorData *data,
>> +                                           const void *unused)
>> +{
>> +    /* Check that aborting mid-visit doesn't leak or double-free */
>> +    visit_start_struct(data->ov, NULL, NULL, 0, &error_abort);
>> +    visit_start_struct(data->ov, "nested", NULL, 0, &error_abort);
>> +    visitor_reset(data);
> 
> Add a second test that additionally visits a member?  Or is visiting the
> "nested" member enough?

Visiting "nested" was enough to trigger the particular valgrind trace I
wanted, but more coverage never hurts. I'll send v2...

> Would you be willing to do the same for lists and alternates?

...and it will also cover these.

> 
> Speaking of alternates, visitor.h's virtual walk example covers struct
> and list, but not alternate.

Sounds like my v2 will be a longer series :)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 2/2] tests: Enhance qmp output to cover partial visit

[Eric Blake]

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

On 10/25/2016 08:41 AM, Eric Blake wrote:

>>> +static void test_visitor_out_partial_visit(TestOutputVisitorData *data,
>>> +                                           const void *unused)
>>> +{
>>> +    /* Check that aborting mid-visit doesn't leak or double-free */
>>> +    visit_start_struct(data->ov, NULL, NULL, 0, &error_abort);
>>> +    visit_start_struct(data->ov, "nested", NULL, 0, &error_abort);
>>> +    visitor_reset(data);
>>
>> Add a second test that additionally visits a member?  Or is visiting the
>> "nested" member enough?
> 
> Visiting "nested" was enough to trigger the particular valgrind trace I
> wanted, but more coverage never hurts. I'll send v2...
> 
>> Would you be willing to do the same for lists and alternates?
> 
> ...and it will also cover these.

Aborting in the middle of an alternate is trickier, since the current
code for alternates does not allow a non-NULL *obj to
visit_start_alternate() (that is, no virtual alternate walk is needed;
to output an alternate, just call directly to the branch of interest,
without a need to wrap things in visit_start/end_alternate; and on
input, an alternate is currently useful only into a QAPI C struct).

> 
>>
>> Speaking of alternates, visitor.h's virtual walk example covers struct
>> and list, but not alternate.
> 
> Sounds like my v2 will be a longer series :)

I'm still trying to see whether tweaking the docs to cover alternates
makes much sense; we'll see what the patch looks like, but may decide it
was not worth it.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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