From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43765) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZK8Xv-0006E8-3K for qemu-devel@nongnu.org; Tue, 28 Jul 2015 13:20:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZK8Xr-0000XB-RI for qemu-devel@nongnu.org; Tue, 28 Jul 2015 13:20:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34220) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZK8Xr-0000WA-Kw for qemu-devel@nongnu.org; Tue, 28 Jul 2015 13:20:03 -0400 From: Markus Armbruster References: <1435782155-31412-1-git-send-email-armbru@redhat.com> <1435782155-31412-41-git-send-email-armbru@redhat.com> <55B164EE.7060507@redhat.com> Date: Tue, 28 Jul 2015 13:31:24 +0200 In-Reply-To: <55B164EE.7060507@redhat.com> (Eric Blake's message of "Thu, 23 Jul 2015 16:04:30 -0600") Message-ID: <87fv48wmb7.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH RFC v2 40/47] qapi: Introduce a first class 'any' type List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, berto@igalia.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com Eric Blake 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 >> --- >> 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 Thanks!