Re: [Qemu-devel] [PATCH RFC v2 40/47] qapi: Introduce a first class 'any' type

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 07/01/2015 02:22 PM, Markus Armbruster wrote:
>> It's first class, because unlike '**', it actually works, i.e. doesn't
>> require 'gen': false.
>
> Generated code grows in order to accommodate visiting the new anyList type:
>
>  qapi-types.c                        |   15 +++++++++++++++
>  qapi-types.h                        |   13 +++++++++++++
>  qapi-visit.c                        |   24 ++++++++++++++++++++++++
>  qapi-visit.h                        |    1 +
>  qga/qapi-generated/qga-qapi-types.h |   13 +++++++++++++
>  qga/qapi-generated/qga-qapi-visit.h |    1 +
>  6 files changed, 67 insertions(+)
>
> But do we really need anyList as a representation of qapi ['any'] ?  Or
> will a later pass that minimizes array creation only to places where it
> is needed help?

Like most generated list types, anyList isn't used.  I didn't feel like
making it a special case.  If the unused list types bother us, we can
generate only the ones actually mentioned in the schema.  May require
mentioning a few we use internally just to have them generated.

>> '**' will go away next.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  docs/qapi-code-gen.txt            |  1 +
>>  include/qapi/visitor-impl.h       |  2 ++
>>  include/qapi/visitor.h            |  1 +
>>  qapi/qapi-dealloc-visitor.c       |  9 +++++++
>>  qapi/qapi-visit-core.c            |  6 +++++
>>  qapi/qmp-input-visitor.c          | 11 ++++++++
>>  qapi/qmp-output-visitor.c         |  9 +++++++
>>  scripts/qapi-types.py             |  1 +
>>  scripts/qapi.py                   |  9 ++++---
>>  tests/qapi-schema/type-bypass.out |  4 +--
>>  tests/test-qmp-input-visitor.c    | 45 +++++++++++++++++++++++++++++++++
>>  tests/test-qmp-output-visitor.c | 53
>> +++++++++++++++++++++++++++++++++++++++
>>  12 files changed, 146 insertions(+), 5 deletions(-)
>
> Touches a bit more this time, but that's okay.
>
>> 
>> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
>> index cb0fe75..bf9e854 100644
>> --- a/docs/qapi-code-gen.txt
>> +++ b/docs/qapi-code-gen.txt
>> @@ -158,6 +158,7 @@ The following types are predefined, and map to C as 
>> follows:
>>    size      uint64_t   like uint64_t, except StringInputVisitor
>>                         accepts size suffixes
>>    bool      bool       JSON true or false
>> +  any       QObject *  any JSON value
>
> And with this, an 'alternate' type is just a special subset of 'any'
> that limits the set of valid JSON values according to the qapi description.
>
>> +++ b/qapi/qapi-visit-core.c
>> @@ -260,6 +260,12 @@ void visit_type_number(Visitor *v, double *obj, const 
>> char *name, Error **errp)
>>      v->type_number(v, obj, name, errp);
>>  }
>>  
>> +void visit_type_any(Visitor *v, QObject **obj, const char *name,
>> +                         Error **errp)
>
> Indentation looks off.

Will fix.

>> @@ -1048,8 +1048,9 @@ class QAPISchema(object):
>>                    ('uint64', 'int',     'uint64_t', '0'),
>>                    ('size',   'int',     'uint64_t', '0'),
>>                    ('bool',   'boolean', 'bool',     'false'),
>> -                  ('**',     'value',   None,       None)]:
>> +                  ('any',    'value',   'QObject' + pointer_suffix , 
>> 'NULL')]:
>>              self._def_builtin_type(*t)
>> +        self.entity_dict['**'] = self.lookup_type('any') # TODO drop this 
>> alias
>>  
>>      def _make_implicit_enum_type(self, name, values):
>>          name = name + 'Kind'
>> @@ -1190,6 +1191,8 @@ class QAPISchema(object):
>>      def visit(self, visitor):
>>          visitor.visit_begin()
>>          for name in sorted(self.entity_dict.keys()):
>> +            if self.entity_dict[name].name != name:
>> +                continue        # ignore alias TODO drop alias and remove
>
> Nice back-compat hacks while you transition into using it.
>
>> +++ b/tests/test-qmp-input-visitor.c
>> @@ -298,6 +298,49 @@ static void test_visitor_in_list(TestInputVisitorData 
>> *data,
>>      qapi_free_UserDefOneList(head);
>>  }
>>  
>> +static void test_visitor_in_any(TestInputVisitorData *data,
>> +                                const void *unused)
>> +{
>
>> +
>> +    v = visitor_input_test_init(data, "-42");
>
> So we prove it accepts ints,...
>
>> + v = visitor_input_test_init(data, "{ 'integer': -42, 'boolean':
>> true, 'string': 'foo' }");
>
> objects,...
>
>> +    visit_type_any(v, &res, NULL, &err);
>> +    g_assert(!err);
>> +    qdict = qobject_to_qdict(res);
>> +    g_assert(qdict);
>
> worth asserting the dictionary has 3 keys?

Can't hurt.

>> +}
>
> ...but no test of string, boolean, or array?

I cut corners to get the thing out: one scalar and one non-scalar type.

We can always add more.

>> +++ b/tests/test-qmp-output-visitor.c
>> @@ -428,6 +428,57 @@ static void 
>> test_visitor_out_list_qapi_free(TestOutputVisitorData *data,
>>      qapi_free_UserDefTwoList(head);
>>  }
>>  
>> +static void test_visitor_out_any(TestOutputVisitorData *data,
>> +                                 const void *unused)
>> +{
>
>> +}
>
> Same tests of 'int' and 'object' but not of other JSON types.
>
> Incomplete tests are still better than no tests, so what you have is a
> good start.
>
> Reviewed-by: Eric Blake <address@hidden>

Thanks!