Re: [Qemu-devel] [PATCH v5 25/28] qapi: Drop tests for inline nested structs

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
> ---
>  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 <address@hidden>