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

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

[Eric Blake]

Well, technically qobject output visitor, but keeping the
cover letter subject unchanged helps for continuity

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 :)

v1 was here:
https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg05699.html

since then:
patch 2 is new
patch 3 (former 2/2) is rebased (now that it is qobject rather
than qmp visitor) and enhanced (list and alternates, in addition
to objects)

Eric Blake (3):
  tests: Simplify expected error checking for qmp output
  qapi: Further enhance visitor virtual walk doc example
  tests: Enhance qobject output to cover partial visit

 include/qapi/visitor.h              | 41 +++++++++++++++++++----------
 tests/test-qobject-output-visitor.c | 51 +++++++++++++++++++++++++++++++------
 2 files changed, 71 insertions(+), 21 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/3] 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>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-qobject-output-visitor.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/tests/test-qobject-output-visitor.c b/tests/test-qobject-output-visitor.c
index 4e2d79c..c2e0f43 100644
--- a/tests/test-qobject-output-visitor.c
+++ b/tests/test-qobject-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 v2 2/3] qapi: Further enhance visitor virtual walk doc example

[Eric Blake]

Markus pointed out that the example given for virtual walks did
not discuss how to do a virtual walk of an alternate type.  It
turns out that for output, we don't need to visit an alternate
(just directly visit the type that we want); and for input,
visit_start_alternate() is not currently wired up for alternates
(it requires a QAPI type, rather than permitting NULL for a
virtual walk).  Also, the example was never updated in commit
3b098d5 about where visit_complete() would fit in.  Improve the
description and example to give more details along these lines.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 include/qapi/visitor.h | 41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 9bb6cba..0d9d74c 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -187,19 +187,25 @@
  *
  * It is also possible to use the visitors to do a virtual walk, where
  * no actual QAPI struct is present.  In this situation, decisions
- * about what needs to be walked are made by the calling code, and
- * structured visits are split between pairs of start and end methods
- * (where the end method must be called if the start function
- * succeeded, even if an intermediate visit encounters an error).
- * Thus, a virtual walk corresponding to '{ "list": [1, 2] }' looks
- * like:
+ * about what needs to be walked are made by the calling code (that
+ * is, there is no use for QAPI alternate types in a virtual walk,
+ * because the code can just directly visit the appropriate type
+ * within the alternate), and structured visits are split between
+ * pairs of start and end methods (where the end method must be called
+ * if the start function succeeded, even if an intermediate visit
+ * encounters an error).  Thus, a virtual output walk of an object
+ * containing a list of alternates between an integer or nested
+ * object, corresponding to '{ "list": [1, { "value": "2" } ] }',
+ * would look like:
  *
  * <example>
  *  Visitor *v;
  *  Error *err = NULL;
- *  int value;
+ *  int i = 1;
+ *  const char *s = "2";
+ *  FOO output;
  *
- *  v = FOO_visitor_new(...);
+ *  v = FOO_visitor_new(..., &output);
  *  visit_start_struct(v, NULL, NULL, 0, &err);
  *  if (err) {
  *      goto out;
@@ -208,16 +214,21 @@
  *  if (err) {
  *      goto outobj;
  *  }
- *  value = 1;
- *  visit_type_int(v, NULL, &value, &err);
+ *  visit_type_int(v, NULL, &i, &err);
  *  if (err) {
  *      goto outlist;
  *  }
- *  value = 2;
- *  visit_type_int(v, NULL, &value, &err);
+ *  visit_type_start(v, NULL, NULL, 0, &err);
  *  if (err) {
- *      goto outlist;
+ *      goto outnest;
+ *  }
+ *  visit_type_str(v, "value", (char **)&s, &err);
+ *  if (err) {
+ *      goto outnest;
  *  }
+ *  visit_check_struct(v, &err);
+ * outnest:
+ *  visit_end_struct(v, NULL);
  * outlist:
  *  visit_end_list(v, NULL);
  *  if (!err) {
@@ -225,6 +236,10 @@
  *  }
  * outobj:
  *  visit_end_struct(v, NULL);
+ *  if (!err) {
+ *      visit_complete(v, &output);
+ *      ...use output...
+ *  }
  * out:
  *  error_propagate(errp, err);
  *  visit_free(v);
-- 
2.7.4

[Qemu-devel] [PATCH v2 3/3] tests: Enhance qobject 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>

---
v2: rebase (and hence retitle), add list & alternate coverage
---
 tests/test-qobject-output-visitor.c | 39 +++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/tests/test-qobject-output-visitor.c b/tests/test-qobject-output-visitor.c
index c2e0f43..fdae0d5 100644
--- a/tests/test-qobject-output-visitor.c
+++ b/tests/test-qobject-output-visitor.c
@@ -254,6 +254,43 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
 }


+static void test_visitor_out_partial_visit(TestOutputVisitorData *data,
+                                           const void *unused)
+{
+    /* Various checks that a mid-visit abort doesn't leak or double-free. */
+    const char *str = "hi";
+    Error *err = NULL;
+    UserDefAlternate uda = { .type = QTYPE_QDICT,
+                             .u.udfu = { .integer = 1,
+                                         .string = (char *) "bye",
+                                         .enum1 = -1 } };
+    UserDefAlternate *obj = &uda;
+
+    /* Abort within a nested object with no data members */
+    visit_start_struct(data->ov, NULL, NULL, 0, &error_abort);
+    visit_start_struct(data->ov, "nested", NULL, 0, &error_abort);
+    visitor_reset(data);
+
+    /* Abort in the middle of a list of strings */
+    visit_start_list(data->ov, "list", NULL, 0, &error_abort);
+    visit_type_str(data->ov, NULL, (char **)&str, &error_abort);
+    visit_type_str(data->ov, NULL, (char **)&str, &error_abort);
+    visitor_reset(data);
+
+    /* Abort in the middle of an alternate. Alternates can't be
+     * virtually visited, so we get to inline the first half of
+     * visit_type_UserDefAlternate(). */
+    visit_start_alternate(data->ov, NULL, (GenericAlternate **)&obj,
+                          sizeof(uda), false, &error_abort);
+    visit_start_struct(data->ov, NULL, NULL, 0, &error_abort);
+    visit_type_UserDefUnionBase_members(data->ov,
+                                        (UserDefUnionBase *)&uda.u.udfu,
+                                        &err);
+    error_free_or_abort(&err);
+    visitor_reset(data);
+}
+
+
 static void test_visitor_out_list(TestOutputVisitorData *data,
                                   const void *unused)
 {
@@ -817,6 +854,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 v2 3/3] tests: Enhance qobject 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>
>
> ---
> v2: rebase (and hence retitle), add list & alternate coverage
> ---
>  tests/test-qobject-output-visitor.c | 39 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>
> diff --git a/tests/test-qobject-output-visitor.c b/tests/test-qobject-output-visitor.c
> index c2e0f43..fdae0d5 100644
> --- a/tests/test-qobject-output-visitor.c
> +++ b/tests/test-qobject-output-visitor.c
> @@ -254,6 +254,43 @@ static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
>  }
>
>
> +static void test_visitor_out_partial_visit(TestOutputVisitorData *data,
> +                                           const void *unused)
> +{
> +    /* Various checks that a mid-visit abort doesn't leak or double-free. */
> +    const char *str = "hi";
> +    Error *err = NULL;
> +    UserDefAlternate uda = { .type = QTYPE_QDICT,
> +                             .u.udfu = { .integer = 1,
> +                                         .string = (char *) "bye",
> +                                         .enum1 = -1 } };
> +    UserDefAlternate *obj = &uda;
> +
> +    /* Abort within a nested object with no data members */
> +    visit_start_struct(data->ov, NULL, NULL, 0, &error_abort);
> +    visit_start_struct(data->ov, "nested", NULL, 0, &error_abort);
> +    visitor_reset(data);
> +
> +    /* Abort in the middle of a list of strings */
> +    visit_start_list(data->ov, "list", NULL, 0, &error_abort);
> +    visit_type_str(data->ov, NULL, (char **)&str, &error_abort);
> +    visit_type_str(data->ov, NULL, (char **)&str, &error_abort);
> +    visitor_reset(data);
> +
> +    /* Abort in the middle of an alternate. Alternates can't be
> +     * virtually visited, so we get to inline the first half of
> +     * visit_type_UserDefAlternate(). */
> +    visit_start_alternate(data->ov, NULL, (GenericAlternate **)&obj,
> +                          sizeof(uda), false, &error_abort);
> +    visit_start_struct(data->ov, NULL, NULL, 0, &error_abort);
> +    visit_type_UserDefUnionBase_members(data->ov,
> +                                        (UserDefUnionBase *)&uda.u.udfu,
> +                                        &err);
> +    error_free_or_abort(&err);

Why does this fail?

> +    visitor_reset(data);
> +}
> +
> +
>  static void test_visitor_out_list(TestOutputVisitorData *data,
>                                    const void *unused)
>  {
> @@ -817,6 +854,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",

Re: [Qemu-devel] [PATCH v2 3/3] tests: Enhance qobject output to cover partial visit

[Eric Blake]

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

On 11/03/2016 11:42 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)
>> +{
>> +    /* Various checks that a mid-visit abort doesn't leak or double-free. */
>> +    const char *str = "hi";
>> +    Error *err = NULL;
>> +    UserDefAlternate uda = { .type = QTYPE_QDICT,
>> +                             .u.udfu = { .integer = 1,
>> +                                         .string = (char *) "bye",
>> +                                         .enum1 = -1 } };

^ Not a valid enum value...


>> +
>> +    /* Abort in the middle of an alternate. Alternates can't be
>> +     * virtually visited, so we get to inline the first half of
>> +     * visit_type_UserDefAlternate(). */
>> +    visit_start_alternate(data->ov, NULL, (GenericAlternate **)&obj,
>> +                          sizeof(uda), false, &error_abort);
>> +    visit_start_struct(data->ov, NULL, NULL, 0, &error_abort);
>> +    visit_type_UserDefUnionBase_members(data->ov,
>> +                                        (UserDefUnionBase *)&uda.u.udfu,
>> +                                        &err);
>> +    error_free_or_abort(&err);
> 
> Why does this fail?

...so visiting the UnionBase_members gripes loudly.  But I see your
point that more comments would be helpful.

-- 
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 v2 3/3] tests: Enhance qobject output to cover partial visit

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 11/03/2016 11:42 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)
>>> +{
>>> +    /* Various checks that a mid-visit abort doesn't leak or double-free. */
>>> +    const char *str = "hi";
>>> +    Error *err = NULL;
>>> +    UserDefAlternate uda = { .type = QTYPE_QDICT,
>>> +                             .u.udfu = { .integer = 1,
>>> +                                         .string = (char *) "bye",
>>> +                                         .enum1 = -1 } };
>
> ^ Not a valid enum value...

Now I see.

>>> +
>>> +    /* Abort in the middle of an alternate. Alternates can't be
>>> +     * virtually visited, so we get to inline the first half of
>>> +     * visit_type_UserDefAlternate(). */
>>> +    visit_start_alternate(data->ov, NULL, (GenericAlternate **)&obj,
>>> +                          sizeof(uda), false, &error_abort);
>>> +    visit_start_struct(data->ov, NULL, NULL, 0, &error_abort);
>>> +    visit_type_UserDefUnionBase_members(data->ov,
>>> +                                        (UserDefUnionBase *)&uda.u.udfu,
>>> +                                        &err);
>>> +    error_free_or_abort(&err);
>> 
>> Why does this fail?
>
> ...so visiting the UnionBase_members gripes loudly.  But I see your
> point that more comments would be helpful.

Would you like to suggest a fixup for me to squash in on commit?

[Qemu-devel] [PATCH v2 3.5/3] fixup! tests: Enhance qobject output to cover partial visit

[Eric Blake]

[adds comments. No commit body change]

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

diff --git a/tests/test-qobject-output-visitor.c b/tests/test-qobject-output-visitor.c
index fdae0d5..7646bb4 100644
--- a/tests/test-qobject-output-visitor.c
+++ b/tests/test-qobject-output-visitor.c
@@ -260,10 +260,12 @@ static void test_visitor_out_partial_visit(TestOutputVisitorData *data,
     /* Various checks that a mid-visit abort doesn't leak or double-free. */
     const char *str = "hi";
     Error *err = NULL;
-    UserDefAlternate uda = { .type = QTYPE_QDICT,
-                             .u.udfu = { .integer = 1,
-                                         .string = (char *) "bye",
-                                         .enum1 = -1 } };
+    UserDefAlternate uda = {
+        .type = QTYPE_QDICT,
+        .u.udfu = { .integer = 1,
+                    .string = (char *) "bye",
+                    .enum1 = -1 } /* intentionally bad */
+    };
     UserDefAlternate *obj = &uda;

     /* Abort within a nested object with no data members */
@@ -286,6 +288,7 @@ static void test_visitor_out_partial_visit(TestOutputVisitorData *data,
     visit_type_UserDefUnionBase_members(data->ov,
                                         (UserDefUnionBase *)&uda.u.udfu,
                                         &err);
+    /* error expected because of bad "enum1" discriminator value */
     error_free_or_abort(&err);
     visitor_reset(data);
 }
-- 
2.7.4