diff mbox

[v5,25/28] qapi: Drop tests for inline nested structs

Message ID 1427227433-5030-26-git-send-email-eblake@redhat.com
State New
Headers show

Commit Message

Eric Blake March 24, 2015, 8:03 p.m. UTC 
A future patch will be using a 'name':{dictionary} entry in the
QAPI schema to specify a default value for an optional argument;
but existing use of inline nested structs conflicts with that goal.
This patch fixes the testsuite to avoid inline nested types, by
breaking the nesting into explicit types; it means that the type
is now boxed instead of unboxed in C code, but makes no difference
on the wire.  When touching code to add new allocations, also
convert existing allocations to consistently prefer typesafe
g_new0 over g_malloc0.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/qapi-schema/qapi-schema-test.json | 12 ++++++--
 tests/qapi-schema/qapi-schema-test.out  |  8 +++--
 tests/test-qmp-commands.c               | 17 ++++++-----
 tests/test-qmp-input-visitor.c          | 14 +++++----
 tests/test-qmp-output-visitor.c         | 44 +++++++++++++++------------
 tests/test-visitor-serialization.c      | 53 ++++++++++++++++++---------------
 6 files changed, 87 insertions(+), 61 deletions(-)

Comments

Markus Armbruster March 27, 2015, 10:30 a.m. UTC | #1 
Eric Blake <eblake@redhat.com> writes:

> A future patch will be using a 'name':{dictionary} entry in the
> QAPI schema to specify a default value for an optional argument;
> but existing use of inline nested structs conflicts with that goal.

I'm okay with this rationale as is, just want to mention the more
general motivation again: we want to make the schema language
extensible.

A definition in the schema associates a name with a set of properties in
a certain lexical environment.

Example 1: { 'type': 'Foo', 'data': { MEMBERS... } }
at the top level associates global name 'Foo' with properties (meta-type
struct) and MEMBERS...

Example 2: 'mumble': TYPE
within the MEMBERS... above associates 'mumble' with properties (type
TYPE) and (optional false) within type Foo.

The syntax in example 1 is extensible: when we need another property, we
can add another member, just like we added 'base'.

The syntax in example 2 isn't extensible that way, because the right
hand side can only be a type.

Property optional is encoded in the name: "'*mumble': 'int'" associates
'mumble' with (type int) and (optional true).  Convenient, but won't
scale.

What we want to do is change the core syntax to

    NAME: { 'type': TYPE }

with syntactic sugar

    NAME: TYPE    -->  NAME:  { 'type': TYPE, 'optional': false }
    *ONAME: TYPE  -->  ONAME: { 'type': TYPE, 'optional': true }

where *ONAME is a string starting with '*', and ONAME its tail.

Now we can extend by adding another member.

Naturally, the schema will mostly be written in the sugared form.

Adding optional defaults is an instance of this general "extend the
schema language" pattern.

Feel free to crib from the above if you want more of the motivation in
the commit log,

> This patch fixes the testsuite to avoid inline nested types, by
> breaking the nesting into explicit types; it means that the type
> is now boxed instead of unboxed in C code, but makes no difference
> on the wire.

Yes, this patch actually adds boxing.

The QAPI code generator boxes too much for my taste, but this series is
not the place to mess with that.

>               When touching code to add new allocations, also
> convert existing allocations to consistently prefer typesafe
> g_new0 over g_malloc0.

Again, I can't see new uses of g_new0().

> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  tests/qapi-schema/qapi-schema-test.json | 12 ++++++--
>  tests/qapi-schema/qapi-schema-test.out  |  8 +++--
>  tests/test-qmp-commands.c               | 17 ++++++-----
>  tests/test-qmp-input-visitor.c          | 14 +++++----
>  tests/test-qmp-output-visitor.c         | 44 +++++++++++++++------------
>  tests/test-visitor-serialization.c      | 53 ++++++++++++++++++---------------
>  6 files changed, 87 insertions(+), 61 deletions(-)
>
> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index fb6a350..7aeb490 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -14,11 +14,17 @@
>    'base': 'UserDefZero',
>    'data': { 'string': 'str', '*enum1': 'EnumOne' } }
>
> +{ 'type': 'UserDefTwoDictDict',
> +  'data': { 'userdef': 'UserDefOne', 'string': 'str' } }
> +
> +{ 'type': 'UserDefTwoDict',
> +  'data': { 'string1': 'str',
> +            'dict2': 'UserDefTwoDictDict',
> +            '*dict3': 'UserDefTwoDictDict' } }
> +
>  { 'type': 'UserDefTwo',
>    'data': { 'string0': 'str',
> -            'dict1': { 'string1': 'str',
> -                       'dict2': { 'userdef': 'UserDefOne', 'string': 'str' },
> -                       '*dict3': { 'userdef': 'UserDefOne', 'string': 'str' } } } }
> +            'dict1': 'UserDefTwoDict' } }
>
>  # for testing unions
>  { 'type': 'UserDefA',
[...]
> diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
> index 9189cd2..dc199d3 100644
> --- a/tests/test-qmp-commands.c
> +++ b/tests/test-qmp-commands.c
> @@ -33,12 +33,15 @@ UserDefTwo *qmp_user_def_cmd2(UserDefOne *ud1a,
>
>      ret = g_malloc0(sizeof(UserDefTwo));
>      ret->string0 = strdup("blah1");
> -    ret->dict1.string1 = strdup("blah2");
> -    ret->dict1.dict2.userdef = ud1c;
> -    ret->dict1.dict2.string = strdup("blah3");
> -    ret->dict1.has_dict3 = true;
> -    ret->dict1.dict3.userdef = ud1d;
> -    ret->dict1.dict3.string = strdup("blah4");
> +    ret->dict1 = g_malloc0(sizeof(UserDefTwoDict));

Hmm, you could g_new0(UserDefTwoDict) here.  More of the same below, and
in the previous patch.

> +    ret->dict1->string1 = strdup("blah2");
> +    ret->dict1->dict2 = g_malloc0(sizeof(UserDefTwoDictDict));
> +    ret->dict1->dict2->userdef = ud1c;
> +    ret->dict1->dict2->string = strdup("blah3");
> +    ret->dict1->dict3 = g_malloc0(sizeof(UserDefTwoDictDict));
> +    ret->dict1->has_dict3 = true;
> +    ret->dict1->dict3->userdef = ud1d;
> +    ret->dict1->dict3->string = strdup("blah4");
>
>      return ret;
>  }
[...]

With either the commit message corrected not to claim use of g_new0(),
or the patch updated to actually do it:

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

Patch

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index fb6a350..7aeb490 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -14,11 +14,17 @@ 
   'base': 'UserDefZero',
   'data': { 'string': 'str', '*enum1': 'EnumOne' } }

+{ 'type': 'UserDefTwoDictDict',
+  'data': { 'userdef': 'UserDefOne', 'string': 'str' } }
+
+{ 'type': 'UserDefTwoDict',
+  'data': { 'string1': 'str',
+            'dict2': 'UserDefTwoDictDict',
+            '*dict3': 'UserDefTwoDictDict' } }
+
 { 'type': 'UserDefTwo',
   'data': { 'string0': 'str',
-            'dict1': { 'string1': 'str',
-                       'dict2': { 'userdef': 'UserDefOne', 'string': 'str' },
-                       '*dict3': { 'userdef': 'UserDefOne', 'string': 'str' } } } }
+            'dict1': 'UserDefTwoDict' } }

 # for testing unions
 { 'type': 'UserDefA',
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 4d73bc0..251f6f1 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -2,7 +2,9 @@ 
  OrderedDict([('type', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),
  OrderedDict([('type', 'UserDefZero'), ('data', OrderedDict([('integer', 'int')]))]),
  OrderedDict([('type', 'UserDefOne'), ('base', 'UserDefZero'), ('data', OrderedDict([('string', 'str'), ('*enum1', 'EnumOne')]))]),
- OrderedDict([('type', 'UserDefTwo'), ('data', OrderedDict([('string0', 'str'), ('dict1', OrderedDict([('string1', 'str'), ('dict2', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')])), ('*dict3', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')]))]))]))]),
+ OrderedDict([('type', 'UserDefTwoDictDict'), ('data', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')]))]),
+ OrderedDict([('type', 'UserDefTwoDict'), ('data', OrderedDict([('string1', 'str'), ('dict2', 'UserDefTwoDictDict'), ('*dict3', 'UserDefTwoDictDict')]))]),
+ OrderedDict([('type', 'UserDefTwo'), ('data', OrderedDict([('string0', 'str'), ('dict1', 'UserDefTwoDict')]))]),
  OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
  OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
  OrderedDict([('type', 'UserDefC'), ('data', OrderedDict([('string1', 'str'), ('string2', 'str')]))]),
@@ -27,7 +29,9 @@ 
 [OrderedDict([('type', 'NestedEnumsOne'), ('data', OrderedDict([('enum1', 'EnumOne'), ('*enum2', 'EnumOne'), ('enum3', 'EnumOne'), ('*enum4', 'EnumOne')]))]),
  OrderedDict([('type', 'UserDefZero'), ('data', OrderedDict([('integer', 'int')]))]),
  OrderedDict([('type', 'UserDefOne'), ('base', 'UserDefZero'), ('data', OrderedDict([('string', 'str'), ('*enum1', 'EnumOne')]))]),
- OrderedDict([('type', 'UserDefTwo'), ('data', OrderedDict([('string0', 'str'), ('dict1', OrderedDict([('string1', 'str'), ('dict2', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')])), ('*dict3', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')]))]))]))]),
+ OrderedDict([('type', 'UserDefTwoDictDict'), ('data', OrderedDict([('userdef', 'UserDefOne'), ('string', 'str')]))]),
+ OrderedDict([('type', 'UserDefTwoDict'), ('data', OrderedDict([('string1', 'str'), ('dict2', 'UserDefTwoDictDict'), ('*dict3', 'UserDefTwoDictDict')]))]),
+ OrderedDict([('type', 'UserDefTwo'), ('data', OrderedDict([('string0', 'str'), ('dict1', 'UserDefTwoDict')]))]),
  OrderedDict([('type', 'UserDefA'), ('data', OrderedDict([('boolean', 'bool')]))]),
  OrderedDict([('type', 'UserDefB'), ('data', OrderedDict([('integer', 'int')]))]),
  OrderedDict([('type', 'UserDefC'), ('data', OrderedDict([('string1', 'str'), ('string2', 'str')]))]),
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index 9189cd2..dc199d3 100644
--- a/tests/test-qmp-commands.c
+++ b/tests/test-qmp-commands.c
@@ -33,12 +33,15 @@  UserDefTwo *qmp_user_def_cmd2(UserDefOne *ud1a,

     ret = g_malloc0(sizeof(UserDefTwo));
     ret->string0 = strdup("blah1");
-    ret->dict1.string1 = strdup("blah2");
-    ret->dict1.dict2.userdef = ud1c;
-    ret->dict1.dict2.string = strdup("blah3");
-    ret->dict1.has_dict3 = true;
-    ret->dict1.dict3.userdef = ud1d;
-    ret->dict1.dict3.string = strdup("blah4");
+    ret->dict1 = g_malloc0(sizeof(UserDefTwoDict));
+    ret->dict1->string1 = strdup("blah2");
+    ret->dict1->dict2 = g_malloc0(sizeof(UserDefTwoDictDict));
+    ret->dict1->dict2->userdef = ud1c;
+    ret->dict1->dict2->string = strdup("blah3");
+    ret->dict1->dict3 = g_malloc0(sizeof(UserDefTwoDictDict));
+    ret->dict1->has_dict3 = true;
+    ret->dict1->dict3->userdef = ud1d;
+    ret->dict1->dict3->string = strdup("blah4");

     return ret;
 }
@@ -204,7 +207,7 @@  static void test_dealloc_partial(void)
     assert(ud2 != NULL);
     assert(ud2->string0 != NULL);
     assert(strcmp(ud2->string0, text) == 0);
-    assert(ud2->dict1.dict2.userdef == NULL);
+    assert(ud2->dict1 == NULL);

     /* confirm & release construction error */
     assert(err != NULL);
diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c
index 115fb22..b961953 100644
--- a/tests/test-qmp-input-visitor.c
+++ b/tests/test-qmp-input-visitor.c
@@ -261,13 +261,15 @@  static void test_visitor_in_struct_nested(TestInputVisitorData *data,
     g_assert(!err);

     check_and_free_str(udp->string0, "string0");
-    check_and_free_str(udp->dict1.string1, "string1");
-    g_assert_cmpint(udp->dict1.dict2.userdef->base->integer, ==, 42);
-    check_and_free_str(udp->dict1.dict2.userdef->string, "string");
-    check_and_free_str(udp->dict1.dict2.string, "string2");
-    g_assert(udp->dict1.has_dict3 == false);
+    check_and_free_str(udp->dict1->string1, "string1");
+    g_assert_cmpint(udp->dict1->dict2->userdef->base->integer, ==, 42);
+    check_and_free_str(udp->dict1->dict2->userdef->string, "string");
+    check_and_free_str(udp->dict1->dict2->string, "string2");
+    g_assert(udp->dict1->has_dict3 == false);

-    g_free(udp->dict1.dict2.userdef);
+    g_free(udp->dict1->dict2->userdef);
+    g_free(udp->dict1->dict2);
+    g_free(udp->dict1);
     g_free(udp);
 }

diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c
index 525c36a..955d4f0 100644
--- a/tests/test-qmp-output-visitor.c
+++ b/tests/test-qmp-output-visitor.c
@@ -244,19 +244,23 @@  static void test_visitor_out_struct_nested(TestOutputVisitorData *data,
     ud2 = g_malloc0(sizeof(*ud2));
     ud2->string0 = g_strdup(strings[0]);

-    ud2->dict1.string1 = g_strdup(strings[1]);
-    ud2->dict1.dict2.userdef = g_malloc0(sizeof(UserDefOne));
-    ud2->dict1.dict2.userdef->string = g_strdup(string);
-    ud2->dict1.dict2.userdef->base = g_new0(UserDefZero, 1);
-    ud2->dict1.dict2.userdef->base->integer = value;
-    ud2->dict1.dict2.string = g_strdup(strings[2]);
+    ud2->dict1 = g_malloc0(sizeof(*ud2->dict1));
+    ud2->dict1->string1 = g_strdup(strings[1]);

-    ud2->dict1.has_dict3 = true;
-    ud2->dict1.dict3.userdef = g_malloc0(sizeof(UserDefOne));
-    ud2->dict1.dict3.userdef->string = g_strdup(string);
-    ud2->dict1.dict3.userdef->base = g_new0(UserDefZero, 1);
-    ud2->dict1.dict3.userdef->base->integer = value;
-    ud2->dict1.dict3.string = g_strdup(strings[3]);
+    ud2->dict1->dict2 = g_malloc0(sizeof(*ud2->dict1->dict2));
+    ud2->dict1->dict2->userdef = g_malloc0(sizeof(UserDefOne));
+    ud2->dict1->dict2->userdef->string = g_strdup(string);
+    ud2->dict1->dict2->userdef->base = g_new0(UserDefZero, 1);
+    ud2->dict1->dict2->userdef->base->integer = value;
+    ud2->dict1->dict2->string = g_strdup(strings[2]);
+
+    ud2->dict1->dict3 = g_malloc0(sizeof(*ud2->dict1->dict3));
+    ud2->dict1->has_dict3 = true;
+    ud2->dict1->dict3->userdef = g_malloc0(sizeof(UserDefOne));
+    ud2->dict1->dict3->userdef->string = g_strdup(string);
+    ud2->dict1->dict3->userdef->base = g_new0(UserDefZero, 1);
+    ud2->dict1->dict3->userdef->base->integer = value;
+    ud2->dict1->dict3->string = g_strdup(strings[3]);

     visit_type_UserDefTwo(data->ov, &ud2, "unused", &err);
     g_assert(!err);
@@ -407,13 +411,15 @@  static void test_visitor_out_list_qapi_free(TestOutputVisitorData *data,
         p->value = g_malloc0(sizeof(*p->value));

         p->value->string0 = g_strdup(string);
-        p->value->dict1.string1 = g_strdup(string);
-        p->value->dict1.dict2.userdef = g_malloc0(sizeof(UserDefOne));
-        p->value->dict1.dict2.userdef->string = g_strdup(string);
-        p->value->dict1.dict2.userdef->base = g_new0(UserDefZero, 1);
-        p->value->dict1.dict2.userdef->base->integer = 42;
-        p->value->dict1.dict2.string = g_strdup(string);
-        p->value->dict1.has_dict3 = false;
+        p->value->dict1 = g_malloc0(sizeof(UserDefTwoDict));
+        p->value->dict1->string1 = g_strdup(string);
+        p->value->dict1->dict2 = g_malloc0(sizeof(UserDefTwoDictDict));
+        p->value->dict1->dict2->userdef = g_malloc0(sizeof(UserDefOne));
+        p->value->dict1->dict2->userdef->string = g_strdup(string);
+        p->value->dict1->dict2->userdef->base = g_new0(UserDefZero, 1);
+        p->value->dict1->dict2->userdef->base->integer = 42;
+        p->value->dict1->dict2->string = g_strdup(string);
+        p->value->dict1->has_dict3 = false;

         p->next = head;
         head = p;
diff --git a/tests/test-visitor-serialization.c b/tests/test-visitor-serialization.c
index 133f882..40169ff 100644
--- a/tests/test-visitor-serialization.c
+++ b/tests/test-visitor-serialization.c
@@ -253,18 +253,21 @@  static UserDefTwo *nested_struct_create(void)
 {
     UserDefTwo *udnp = g_malloc0(sizeof(*udnp));
     udnp->string0 = strdup("test_string0");
-    udnp->dict1.string1 = strdup("test_string1");
-    udnp->dict1.dict2.userdef = g_malloc0(sizeof(UserDefOne));
-    udnp->dict1.dict2.userdef->base = g_new0(UserDefZero, 1);
-    udnp->dict1.dict2.userdef->base->integer = 42;
-    udnp->dict1.dict2.userdef->string = strdup("test_string");
-    udnp->dict1.dict2.string = strdup("test_string2");
-    udnp->dict1.has_dict3 = true;
-    udnp->dict1.dict3.userdef = g_malloc0(sizeof(UserDefOne));
-    udnp->dict1.dict3.userdef->base = g_new0(UserDefZero, 1);
-    udnp->dict1.dict3.userdef->base->integer = 43;
-    udnp->dict1.dict3.userdef->string = strdup("test_string");
-    udnp->dict1.dict3.string = strdup("test_string3");
+    udnp->dict1 = g_malloc0(sizeof(*udnp->dict1));
+    udnp->dict1->string1 = strdup("test_string1");
+    udnp->dict1->dict2 = g_malloc0(sizeof(*udnp->dict1->dict2));
+    udnp->dict1->dict2->userdef = g_malloc0(sizeof(UserDefOne));
+    udnp->dict1->dict2->userdef->base = g_new0(UserDefZero, 1);
+    udnp->dict1->dict2->userdef->base->integer = 42;
+    udnp->dict1->dict2->userdef->string = strdup("test_string");
+    udnp->dict1->dict2->string = strdup("test_string2");
+    udnp->dict1->dict3 = g_malloc0(sizeof(*udnp->dict1->dict3));
+    udnp->dict1->has_dict3 = true;
+    udnp->dict1->dict3->userdef = g_malloc0(sizeof(UserDefOne));
+    udnp->dict1->dict3->userdef->base = g_new0(UserDefZero, 1);
+    udnp->dict1->dict3->userdef->base->integer = 43;
+    udnp->dict1->dict3->userdef->string = strdup("test_string");
+    udnp->dict1->dict3->string = strdup("test_string3");
     return udnp;
 }

@@ -273,18 +276,20 @@  static void nested_struct_compare(UserDefTwo *udnp1, UserDefTwo *udnp2)
     g_assert(udnp1);
     g_assert(udnp2);
     g_assert_cmpstr(udnp1->string0, ==, udnp2->string0);
-    g_assert_cmpstr(udnp1->dict1.string1, ==, udnp2->dict1.string1);
-    g_assert_cmpint(udnp1->dict1.dict2.userdef->base->integer, ==,
-                    udnp2->dict1.dict2.userdef->base->integer);
-    g_assert_cmpstr(udnp1->dict1.dict2.userdef->string, ==,
-                    udnp2->dict1.dict2.userdef->string);
-    g_assert_cmpstr(udnp1->dict1.dict2.string, ==, udnp2->dict1.dict2.string);
-    g_assert(udnp1->dict1.has_dict3 == udnp2->dict1.has_dict3);
-    g_assert_cmpint(udnp1->dict1.dict3.userdef->base->integer, ==,
-                    udnp2->dict1.dict3.userdef->base->integer);
-    g_assert_cmpstr(udnp1->dict1.dict3.userdef->string, ==,
-                    udnp2->dict1.dict3.userdef->string);
-    g_assert_cmpstr(udnp1->dict1.dict3.string, ==, udnp2->dict1.dict3.string);
+    g_assert_cmpstr(udnp1->dict1->string1, ==, udnp2->dict1->string1);
+    g_assert_cmpint(udnp1->dict1->dict2->userdef->base->integer, ==,
+                    udnp2->dict1->dict2->userdef->base->integer);
+    g_assert_cmpstr(udnp1->dict1->dict2->userdef->string, ==,
+                    udnp2->dict1->dict2->userdef->string);
+    g_assert_cmpstr(udnp1->dict1->dict2->string, ==,
+                    udnp2->dict1->dict2->string);
+    g_assert(udnp1->dict1->has_dict3 == udnp2->dict1->has_dict3);
+    g_assert_cmpint(udnp1->dict1->dict3->userdef->base->integer, ==,
+                    udnp2->dict1->dict3->userdef->base->integer);
+    g_assert_cmpstr(udnp1->dict1->dict3->userdef->string, ==,
+                    udnp2->dict1->dict3->userdef->string);
+    g_assert_cmpstr(udnp1->dict1->dict3->string, ==,
+                    udnp2->dict1->dict3->string);
 }

 static void nested_struct_cleanup(UserDefTwo *udnp)