From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59272) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZITgD-00069I-K5 for qemu-devel@nongnu.org; Thu, 23 Jul 2015 23:29:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZITgA-0004En-9P for qemu-devel@nongnu.org; Thu, 23 Jul 2015 23:29:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56547) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZITg9-0004E8-Sx for qemu-devel@nongnu.org; Thu, 23 Jul 2015 23:29:46 -0400 References: <1435782155-31412-1-git-send-email-armbru@redhat.com> <1435782155-31412-46-git-send-email-armbru@redhat.com> From: Eric Blake Message-ID: <55B1B121.7040404@redhat.com> Date: Thu, 23 Jul 2015 21:29:37 -0600 MIME-Version: 1.0 In-Reply-To: <1435782155-31412-46-git-send-email-armbru@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="RlGoj096hqQKHBIc17hS2rKlc81i15LXq" Subject: Re: [Qemu-devel] [PATCH RFC v2 45/47] qapi: New QMP command query-schema for QMP schema introspection List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , qemu-devel@nongnu.org Cc: kwolf@redhat.com, berto@igalia.com, mdroth@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --RlGoj096hqQKHBIc17hS2rKlc81i15LXq Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 07/01/2015 02:22 PM, Markus Armbruster wrote: > Caution, rough edges. No joke. It doesn't even compile without this fixup to a rebase snafu (see [0] below): diff --git i/scripts/qapi-types.py w/scripts/qapi-types.py index 79e8d24..12f3767 100644 --- i/scripts/qapi-types.py +++ w/scripts/qapi-types.py @@ -184,6 +184,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): self.fwdecl =3D None self.fwdefn =3D None self.btin =3D None + def visit_begin(self, schema): self.decl =3D '' self.defn =3D '' self.fwdecl =3D '' I already know you'll be editing this further, but here's some things to look for. >=20 > qapi/introspect.json defines the introspection schema. It should do > for uses other than QMP. > FIXME it's almost entirely devoid of comments. It generates quite a bit of code to support itself :) qapi-types.c | 383 ++++++++++++++++++++++++++++ qapi-types.h | 293 +++++++++++++++++++++ qapi-visit.c | 797 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ qapi-visit.h | 26 + 4 files changed, 1499 insertions(+) >=20 > The introspection schema does not reflect all the rules and > restrictions that apply to QAPI schemata. A valid QAPI schema has an > introspection value conforming to the introspection schema, but the > converse is not true. >=20 > Introspection lowers away a number of schema details: >=20 > * The built-in types are declared with their JSON type. >=20 > TODO Should we map all the integer types to just int? Just int for now seems fine. It's easier to add refinement later when we have a proven need, than it is to be stuck advertising too much information now and being stuck with it. >=20 > * Implicit type definitions are made explicit, and given > auto-generated names. These names start with ':' so they don't > clash with the user's names. >=20 > Example: a simple union implicitly defines an enumeration type for > its discriminator. >=20 > * All type references are by name. >=20 > * Base types are flattened. >=20 > * The top type (named 'any') can hold any value. >=20 > * The struct and union types are generalized into an object type. You've mentioned that idea on list quite some time ago, and it appears to be working well. >=20 > * Commands take a single argument and return a single result. >=20 > Dictionary argument/result or list result is an implicit type > definition. >=20 > The empty object type is used when a command takes no arguments or > produces no results. >=20 > The argument is always of object type, but the introspection schema > doesn't reflect that. And if we ever change QMP to allow non-objects for the arguments, introspection will still work. >=20 > The 'gen': false and 'success-response': false directives are > omitted as implementation detail. I understand omitting 'gen' (hmm, I though we could get rid of it by making netdev_add typesafe, but now this patch adds another 'gen':false and for good reason). But 'success-response' is part of the ABI; we already advertise it via qga's 'guest-info' command exposing 'success-response':'bool' for each command. Adding it would allow a single introspection command to learn everything, instead of having to pair introspection with 'guest-info'. On the other hand, as above, it's easier to start thin and add more later if needed, than it is to start full and then have to support it forever. >=20 > * Events carry a single data value. >=20 > Implicit type definition and empty object type use, just like for > commands. >=20 > The value is of object type, but the introspection schema doesn't > reflect that. >=20 > * Types not used by commands or events are omitted. >=20 > Indirect use counts as use. >=20 > * Optional members have a default, which can only be null right now >=20 > Instead of a mandatory "optional" flag, we have an optional default. > No default means mandatory, default null means optional without > default value. Non-null is available for optional with default. >=20 > Alternate members can't have defaults, but the introspection schema > doesn't reflect that. >=20 > * Clients should *not* look up types by name, because type names are > not ABI. Look up the command or event you're interested in, then > follow the references. >=20 > TODO Should we hide the type names to eliminate the temptation? Might be worth doing; I see you have a later patch to experiment with it.= >=20 > * Likewise, the names of alternate members are not ABI, and should not > be examined. >=20 > TODO Should we hide them, too? How, by making the name of an object member optional? >=20 > TODO much of the above should go into docs. Indeed. Hence why this is RFC still. >=20 > New generator scripts/qapi-introspect.py computes an introspection > value for its input, and generates a C variable holding it. >=20 > FIXME it can generate awfully long lines We already have long lines in generated output, but I agree that finding ways to break it up might be nice. Actually, >=20 > A new test-qmp-input-visitor test case feeds its result for both > tests/qapi-schema/qapi-schema-test.json and qapi-schema.json to a > QmpInputVisitor to verify it actually conforms to the schema. >=20 > New QMP command query-schema takes its return value from that > variable. Command documentation is incomplete, and marked FIXME. Its > reply is some 80KiBytes for me right now. $ ll qmp-introspect.[ch] -rw-rw-r--. 1 eblake eblake 89655 Jul 23 17:20 qmp-introspect.c -rw-rw-r--. 1 eblake eblake 358 Jul 23 17:20 qmp-introspect.h >=20 > If this turns out to be too much, we have a couple of options: >=20 > * We can use shorter names in the JSON. Not the QMP style. >=20 > * Optionally return the sub-schema for commands and events given as > arguments. Filtering is probably worth having, but I'm not sure how easy or hard it gets... >=20 > Right now qmp_query_schema() sends the string literal computed by > qmp-introspect.py. To compute sub-schema at run time, we'd have to > duplicate parts of qapi-introspect.py in C. Unattractive. =2E..might be simpler than that. You can do some pre-processing, so that the C code can take shortcuts of having pre-computed dependency information. Instead of one giant string, you instead have an array of structs, one array entry per schema entity: struct foo { const char *name; /* example: ":empty" */ int metatype; /* SCHEMA_META_TYPE_OBJECT */ const char *json; /* "{ 'name': ':empty', 'meta-type': 'object', 'members': [ ] }, " */ int ndepends; /* length of depends array */ int *depends; /* indices of other entities this depends on */ bool visited; /* helper for filtering */ }; If the user requests filtering, you then do a pre-pass that sets array[i]->visited =3D filter_match(), then an iterative pass that says fo= r every array[i]->visited, you also want to set array[array[i]->depends[j]]->visited for each j up to array[i]->ndepends; then finally produce output of "[" + each array[i]->json where visited is true + "]". Of course, adding filtering as a follow-on patch is the only way to go; initial implementation is fine as global information. >=20 > * Let clients cache the output of query-schema. >=20 > It changes only on QEMU upgrades, i.e. rarely. Provide a command > query-schema-hash. Clients can have a cache indexed by hash, and > re-query the schema only when they don't have it cached. Libvirt already caches things. I don't know if having qemu output a hash helps libvirt, or if libvirt won't mind just eating the cost of reading the entire array every time anything else causes it to repopulate the cache (such as timestamp on qemu binary changing). We can play with it later. >=20 > Signed-off-by: Markus Armbruster > --- Missing a change to docs/qapi-code-gen.txt describing the new script and output. > 30 files changed, 345 insertions(+), 12 deletions(-) > create mode 100644 qapi/introspect.json > create mode 100644 scripts/qapi-introspect.py >=20 > diff --git a/.gitignore b/.gitignore > index aed0e1f..a6a02db 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -32,6 +32,7 @@ > /qapi-visit.[ch] > /qapi-event.[ch] > /qmp-commands.h > +/qmp-introspect.[ch] > /qmp-marshal.c > /qemu-doc.html > /qemu-tech.html Missing an addition for the new tests/test-qmp-introspect.h file leftover after 'make'. On second thought, that belongs at [2] below. > +++ b/monitor.c > @@ -74,6 +74,7 @@ > #include "block/qapi.h" > #include "qapi/qmp-event.h" > #include "qapi-event.h" > +#include "qmp-introspect.h" > #include "sysemu/block-backend.h" > =20 > /* for hmp_info_irq/pic */ > @@ -924,6 +925,20 @@ EventInfoList *qmp_query_events(Error **errp) > return ev_list; > } > =20 > +/* > + * Minor hack: generated marshalling suppressed for this command > + * ('gen': false in the schema) so we can simply send the JSON string > + * instead of first parsing it with visit_type_SchemaInfoList() into a= > + * SchemaInfoList, then unparse it right back in the generated output > + * marshaller, every time. > + * Instead, we do it in test-qmp-input-visitor.c, just to make sure > + * qapi-introspect.py's output actually conforms to the schema. > + */ > +static void qmp_query_schema(QDict *qdict, QObject **ret_data, Error *= *errp) > +{ > + *ret_data =3D qobject_from_json(qmp_schema_json); Works for me :) > +++ b/qapi/introspect.json > @@ -0,0 +1,69 @@ > +# -*- Mode: Python -*- > +# > +# QAPI introspection > +# > +# Copyright (C) 2015 Red Hat, Inc. > +# > +# Authors: > +# Markus Armbruster > +# > +# This work is licensed under the terms of the GNU GPL, version 2 or l= ater. > +# See the COPYING file in the top-level directory. > + > +{ 'enum': 'SchemaMetaType', > + 'data': [ 'builtin', 'enum', 'array', 'object', 'alternate', > + 'command', 'event' ] } > + > +{ 'struct': 'SchemaInfoBase', > + 'data': { 'name': 'str', 'meta-type': 'SchemaMetaType' } } > + > +{ 'enum': 'JSONType', > + 'data': [ 'string', 'number', 'int', 'boolean', 'null', > + 'object', 'array', 'value' ] } > + > +{ 'struct': 'SchemaInfoBuiltin', > + 'data': { 'json-type': 'JSONType' } } > + > +{ 'struct': 'SchemaInfoEnum', > + 'data': { 'values': ['str'] } } Do we want to document anything about sort ordering of this list? Is it worth sorting the array by name, to allow clients to bsearch for whether a particular enum value is supported, rather than having to linear search= ? > + > +{ 'struct': 'SchemaInfoArray', > + 'data': { 'element-type': 'str' } } > + > +{ 'struct': 'SchemaInfoObjectMember', > + 'data': { 'name': 'str', 'type': 'str', '*default': 'any' } } > +# @default's type must match @type or be null; as you mentioned above, this sets up a tri-state of mandatory, optional with no default, and (for future extension) optional with default. > + > +{ 'struct': 'SchemaInfoObjectVariant', > + 'data': { 'case': 'str', > + 'members': [ 'SchemaInfoObjectMember' ] } } Would it be simpler to just have: 'data': { 'case': 'str', 'type': 'str' } and make the user refer recursively to the (possibly-implicit) type for the members? In particular, if we ever decide to allow a flat union to have another union as a branch, rather than the current restriction that all branches must be structs, then referring to the type of a branch may be easier than breaking out all members of a struct. And if that's the case, it may have knock-on simplifications to your earlier patches for tracking variants. See [1] below for more thoughts... Do we want to guarantee anything about the sort ordering in this list? > + > +{ 'struct': 'SchemaInfoObject', > + 'data': { 'members': [ 'SchemaInfoObjectMember' ], > + '*tag': 'str', > + '*variants': [ 'SchemaInfoObjectVariant' ] } } or these? > + > +{ 'struct': 'SchemaInfoAlternate', > + 'data': { 'members': [ 'SchemaInfoObjectMember' ] } } Here's an example of what you generated: "{ 'name': 'BlockdevRef', 'meta-type': 'alternate', 'members': [ { 'name': 'definition', 'type': 'BlockdevOptions' }, { 'name': 'reference', 'type': 'str' } ] }, " I think you could get away with something simpler: 'data': { 'types': [ 'str' ] } as in: "{ 'name': 'BlockdevRef', 'meta-type': 'alternate', 'types': [ 'BlockdevOptions', 'str' ] }, " the only worry is whether we might want future extensions, where we'd want additional information per element of that array, vs. being forced to return two arrays in parallel (arrays of structs are more extensible than arrays of strings). Seems like this would be just a > + > +{ 'struct': 'SchemaInfoCommand', > + 'data': { 'args': 'str', 'returns': 'str' } } Again, we can add 'success-return':'bool' later, if desired. > + > +{ 'struct': 'SchemaInfoEvent', > + 'data': { 'data': 'str' } } > + > +{ 'union': 'SchemaInfo', > + 'base': 'SchemaInfoBase', > + 'discriminator': 'meta-type', > + 'data': { > + 'builtin': 'SchemaInfoBuiltin', > + 'enum': 'SchemaInfoEnum', > + 'array': 'SchemaInfoArray', > + 'object': 'SchemaInfoObject', > + 'alternate': 'SchemaInfoAlternate', > + 'command': 'SchemaInfoCommand', > + 'event': 'SchemaInfoEvent' } } > + > +{ 'command': 'query-schema', > + 'returns': [ 'SchemaInfo' ], > + 'gen': false } # just to simplify qmp_query_json() with an optional 'data':{'*filter':['str']} as a possible future extension. Looks nice, in spite of being under-documented! > +++ b/scripts/qapi-commands.py > @@ -265,7 +265,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor= ): > self.defn =3D None > self.regy =3D None > self.visited_rets =3D None > - def visit_begin(self): > + def visit_begin(self, schema): And again my python object-oriented newness is showing through; where I guess all children have to update signatures to still be polymorphic to a parent adding a parameter. Might be worth separating the patch to add the schema parameter so that there is less churn to the existing scripts - or even rebase the series up-front to always use the schema parameter and not change the signature here. > self.decl =3D '' > self.defn =3D '' > self.regy =3D '' > @@ -273,7 +273,8 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor= ): > def visit_end(self): > if not middle_mode: > self.defn +=3D gen_registry(self.regy) > - self.regy =3D None > + self.regy =3D None > + self.visited_rets =3D None Is it worth squashing this reset into the patch where the visitor was first written? > def visit_command(self, name, info, args, rets, gen, success_respo= nse): > if not gen: > return > diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py > index 184a81f..71da7a9 100644 > --- a/scripts/qapi-event.py > +++ b/scripts/qapi-event.py > @@ -139,13 +139,14 @@ class QAPISchemaGenEventVisitor(QAPISchemaVisitor= ): > self.decl =3D None > self.defn =3D None > self.event_names =3D None > - def visit_begin(self): > + def visit_begin(self, schema): > self.decl =3D '' > self.defn =3D '' > self.event_names =3D [] > def visit_end(self): > self.decl +=3D gen_enum(event_enum_name, self.event_names) > self.defn +=3D gen_enum_lookup(event_enum_name, self.event_nam= es) > + self.event_names =3D None and again > def visit_event(self, name, info, data): > self.decl +=3D gen_event_send_decl(name, data) > self.defn +=3D gen_event_send(name, data) > diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py > new file mode 100644 > index 0000000..e7efc4a > --- /dev/null > +++ b/scripts/qapi-introspect.py > @@ -0,0 +1,159 @@ > +# > +# QAPI introspection generator > +# > +# Copyright (C) 2015 Red Hat, Inc. > +# > +# Authors: > +# Markus Armbruster > +# > +# This work is licensed under the terms of the GNU GPL, version 2. > +# See the COPYING file in the top-level directory. > + > +from qapi import * > + > +class QAPISchemaGenIntrospectVisitor(QAPISchemaVisitor): > + def __init__(self): > + self.schema =3D None > + self.jsons =3D None > + self.used_types =3D None > + self.defn =3D None > + self.decl =3D None > + > + def visit_begin(self, schema): > + self.schema =3D schema > + self.jsons =3D [] > + self.used_types =3D [] > + return QAPISchemaType # don't visit types for now > + > + def visit_end(self): > + # visit the types that are actually used > + for typ in self.used_types: > + typ.visit(self) > + self.jsons.sort() > + name =3D prefix + 'qmp_schema_json' > + self.decl =3D mcgen(''' > +extern char %(c_name)s[]; Missing 'const' > +''', > + c_name=3Dc_name(name)) > + self.defn =3D mcgen(''' > +char %(c_name)s[] =3D "[" > + "%(c_jsons)s]"; And again. Also, I'd consider putting the "]" on its own line, like the "[" was, so that you can more easily cut and paste individual lines of generated output (but since JSON doesn't allow trailing comma, I guess the last line is still always going to be special). > +''', > + c_name=3Dc_name(name), > + c_jsons=3D', "\n "'.join(self.jsons)) Cool syntax :) > + self.schema =3D None > + self.jsons =3D None > + self.used_types =3D None > + > + def _use_type(self, typ): > + if typ not in self.used_types: > + self.used_types.append(typ) > + return typ.name > + > + def _gen_json(self, name, mtype, extra): > + self.jsons.append("{ 'name': '%s', 'meta-type': '%s', %s }" > + % (name, mtype, extra)) Problem. We document that our QMP engine accepts 'single-quoted' input as an extension to JSON, but that our output will always be "double-quoted". That means you _must_ write \" everywhere in the generated C code. (Also, you set the style guide earlier of using '' quoting around any text destined for generated C code) self.jsons.append('''{ \"name\": \"%s\", \"meta-type\": \"%s\", %s }''' % (name, mtype, extra)) (at least, I assume '''...\"...''' is nicer than '...\\\"...') > + > + def _gen_members(self, members): > + return ("'members': [ " > + + ", ".join([self._gen_member(m) for m in members]) > + + " ]") and more choice-of-quotes issues. Lots more below; I'll quit pointing them out on this round of review. > + > + def _gen_member(self, member): > + default =3D '' > + if member.optional: > + default =3D ", 'default': null" > + return "{ 'name': '%s', 'type': '%s'%s }" \ > + % (member.name, self._use_type(member.type), default) > + > + def _gen_variants(self, tag_name, variants): > + return ("'tag': '%s'" % tag_name > + + ", 'variants': [ " > + + ", ".join([self._gen_variant(v) for v in variants]) > + + " ]") > + > + def _gen_variant(self, variant): > + if variant.flat: > + members =3D self._gen_members(variant.type.members) > + else: > + members =3D "'members': [ { 'name': 'data', 'type': '%s' }= ]" \ > + % self._use_type(variant.type) > + return "{ 'case': '%s', %s }" % (variant.name, members) [1] Ah, so .flat is still in use here, to avoid having to create implicit types everywhere. But if we create implicit types for simple unions, and just track variants by their case name/type instead of case name/[members], it will allow us to have a union as a case branch (I don't know that we need that much flexibility), and not have to worry about exposing .flat everywhere. It may even result in a smaller JSON string (you'd have to play with it to know for sure). > + > + def visit_builtin_type(self, name, info, json_type): > + self._gen_json(name, 'builtin', > + "'json-type': '%s'" % json_type) > + > + def visit_enum_type(self, name, info, values): > + self._gen_json(name, 'enum', > + "'values': [ %s ]" % ", ".join(["'%s'" % v > + for v in values= ])) > + > + def visit_array_type(self, name, info, element_type): > + self._gen_json(name, 'array', > + "'element-type': '%s'" % self._use_type(element= _type)) > + > + def visit_object_type_flat(self, name, info, members, variants): > + extra =3D self._gen_members(members) > + if variants: > + extra +=3D ", " + self._gen_variants(variants.tag_name or = "type", > + variants.variants) > + self._gen_json(name, 'object', extra) > + Do we really need two types of object visitors, or can you reuse the signature all the other visitors have? > + def visit_alternate_type(self, name, info, variants): > + self._gen_json(name, 'alternate', > + self._gen_members(variants.variants)) > + > + def visit_command(self, name, info, args, rets, gen, success_respo= nse): > + args =3D args or self.schema.the_empty_object_type > + rets =3D rets or self.schema.the_empty_object_type > + self._gen_json(name, 'command', > + "'args': '%s', 'returns': '%s'" \ > + % (self._use_type(args), self._use_type(rets)))= > + > + def visit_event(self, name, info, data): > + data =3D data or self.schema.the_empty_object_type > + self._gen_json(name, 'event', "'data': '%s'" % self._use_type(= data)) > + > +(input_file, output_dir, do_c, do_h, prefix, dummy) =3D parse_command_= line() > + > +++ b/scripts/qapi-types.py > @@ -184,7 +184,6 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): > self.fwdecl =3D None > self.fwdefn =3D None > self.btin =3D None > - def visit_begin(self): > self.decl =3D '' Rebase botch mentioned above [0] > self.defn =3D '' > self.fwdecl =3D '' > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index 2813bb3..7a03292 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -326,7 +326,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):= > self.decl =3D None > self.defn =3D None > self.btin =3D None > - def visit_begin(self): > + def visit_begin(self, schema): > self.decl =3D '' > self.defn =3D '' > self.btin =3D guardstart('QAPI_VISIT_BUILTIN_VISITOR_DECL') > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 6ddb33e..7f9a159 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -764,7 +764,7 @@ class QAPISchemaEntity(object): > pass > =20 > class QAPISchemaVisitor(object): > - def visit_begin(self): > + def visit_begin(self, schema): I don't know enough python to know if making schema optional in the parent class affects what the child class is allowed to implement while still overriding things. > pass > def visit_end(self): > pass > @@ -776,6 +776,8 @@ class QAPISchemaVisitor(object): > pass > def visit_object_type(self, name, info, base, members, variants): > pass > + def visit_object_type_flat(self, name, info, members, variants): > + pass > def visit_alternate_type(self, name, info, variants): > pass > def visit_command(self, name, info, args, rets, gen, success_respo= nse): > @@ -892,6 +894,8 @@ class QAPISchemaObjectType(QAPISchemaType): > def visit(self, visitor): > visitor.visit_object_type(self.name, self.info, > self.base, self.local_members, self.= variants) > + visitor.visit_object_type_flat(self.name, self.info, > + self.members, self.variants) Instead of having two object visitor signatures, would it be worth having a boolean parameter to QAPISchema.visit() that says whether to pass base, members as (base type, local members) vs. (None, all members)?= > =20 > class QAPISchemaObjectTypeMember(object): > def __init__(self, name, typ, optional): > @@ -1042,6 +1046,9 @@ class QAPISchema(object): > ('bool', 'boolean', 'bool', 'false'), > ('any', 'value', 'QObject' + pointer_suffix , '= NULL')]: > self._def_builtin_type(*t) > + self.the_empty_object_type =3D QAPISchemaObjectType(':empty', = None, None, > + [], None) Cool global. Worth adding in a separate patch? > + self._def_entity(self.the_empty_object_type) > =20 > def _make_implicit_enum_type(self, name, values): > name =3D name + 'Kind' > @@ -1180,9 +1187,10 @@ class QAPISchema(object): > ent.check(self) > =20 > def visit(self, visitor): > - visitor.visit_begin() > + ignore =3D visitor.visit_begin(self) > for name in sorted(self.entity_dict.keys()): > - self.entity_dict[name].visit(visitor) > + if not ignore or not isinstance(self.entity_dict[name], ig= nore): > + self.entity_dict[name].visit(visitor) So this lets introspection bypass visiting types on the first pass, and then collect used types during visit_end(). It means you can only bypass a single metatype, but that is sufficient for your use; I don't know if there is any better idiom for this paradigm. > visitor.visit_end() > =20 > # > diff --git a/tests/.gitignore b/tests/.gitignore > index dc813c2..dda86cc 100644 > --- a/tests/.gitignore > +++ b/tests/.gitignore > @@ -19,6 +19,7 @@ test-opts-visitor > test-qapi-event.[ch] > test-qapi-types.[ch] > test-qapi-visit.[ch] > +test-qapi-introspect.[ch] [2] Ah, maybe this is the file that wasn't quite right. > test-qdev-global-props > test-qemu-opts > test-qmp-commands > diff --git a/tests/Makefile b/tests/Makefile > index 60b82e2..7b1bf92 100644 > --- a/tests/Makefile > +++ b/tests/Makefile > @@ -253,7 +253,8 @@ check-qapi-schema-y :=3D $(addprefix tests/qapi-sch= ema/, \ > struct-base-clash.json struct-base-clash-deep.json ) > =20 > GENERATED_HEADERS +=3D tests/test-qapi-types.h tests/test-qapi-visit.h= \ > - tests/test-qmp-commands.h tests/test-qapi-event.h > + tests/test-qmp-commands.h tests/test-qapi-event.h \ > + tests/test-qmp-introspect.h > =20 > test-obj-y =3D tests/check-qint.o tests/check-qstring.o tests/check-qd= ict.o \ > tests/check-qlist.o tests/check-qfloat.o tests/check-qjson.o \ > @@ -266,7 +267,7 @@ test-obj-y =3D tests/check-qint.o tests/check-qstri= ng.o tests/check-qdict.o \ > tests/rcutorture.o tests/test-rcu-list.o > =20 > test-qapi-obj-y =3D tests/test-qapi-visit.o tests/test-qapi-types.o \ > - tests/test-qapi-event.o > + tests/test-qapi-event.o tests/test-qmp-introspect.o > =20 > $(test-obj-y): QEMU_INCLUDES +=3D -Itests > QEMU_CFLAGS +=3D -I$(SRC_PATH)/tests > @@ -327,6 +328,11 @@ $(SRC_PATH)/tests/qapi-schema/qapi-schema-test.jso= n $(SRC_PATH)/scripts/qapi-eve > $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-event.py \ > $(gen-out-type) -o tests -p "test-" $<, \ > " GEN $@") > +tests/test-qmp-introspect.c tests/test-qmp-introspect.h :\ > +$(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json $(SRC_PATH)/script= s/qapi-introspect.py $(qapi-py) > + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-introspect.py= \ > + $(gen-out-type) -o tests -p "test-" $<, \ > + " GEN $@") > =20 > tests/test-string-output-visitor$(EXESUF): tests/test-string-output-vi= sitor.o $(test-qapi-obj-y) libqemuutil.a libqemustub.a > tests/test-string-input-visitor$(EXESUF): tests/test-string-input-visi= tor.o $(test-qapi-obj-y) libqemuutil.a libqemustub.a > diff --git a/tests/qapi-schema/alternate-good.out b/tests/qapi-schema/a= lternate-good.out > index 0cbdfa1..aede1ae 100644 > --- a/tests/qapi-schema/alternate-good.out > +++ b/tests/qapi-schema/alternate-good.out > @@ -1,3 +1,4 @@ > +object :empty > alternate Alt Again, adding the magic :empty object in a separate patch from the new introspection code may help minimize the number of files being touched with the new code. > +++ b/tests/test-qmp-input-visitor.c > @@ -17,6 +17,9 @@ > #include "qapi/qmp-input-visitor.h" > #include "test-qapi-types.h" > #include "test-qapi-visit.h" > +#include "test-qmp-introspect.h" > +#include "qmp-introspect.h" > +#include "qapi-visit.h" > #include "qapi/qmp/types.h" > =20 > typedef struct TestInputVisitorData { > @@ -660,6 +663,31 @@ static void test_visitor_in_native_list_number(Tes= tInputVisitorData *data, > qapi_free_UserDefNativeListUnion(cvalue); > } > =20 > +static void do_test_visitor_in_qmp_introspect(TestInputVisitorData *da= ta, > + const char *schema_json)= > +{ > + SchemaInfoList *schema =3D NULL; > + Error *err =3D NULL; > + Visitor *v; > + > + v =3D visitor_input_test_init_raw(data, schema_json); > + > + visit_type_SchemaInfoList(v, &schema, NULL, &err); > + if (err) > + fprintf(stderr, "%s", error_get_pretty(err)); > + g_assert(!err); Don't you want: visit_type_SchemaInfoList(..., &error_abort); > + g_assert(schema); > + > + qapi_free_SchemaInfoList(schema); > +} > + > +static void test_visitor_in_qmp_introspect(TestInputVisitorData *data,= > + const void *unused) > +{ > + do_test_visitor_in_qmp_introspect(data, test_qmp_schema_json); > + do_test_visitor_in_qmp_introspect(data, qmp_schema_json); > +} > + > static void input_visitor_test_add(const char *testpath, > TestInputVisitorData *data, > void (*test_func)(TestInputVisitorD= ata *data, const void *user_data)) > @@ -753,6 +781,9 @@ int main(int argc, char **argv) > input_visitor_test_add("/visitor/input/native_list/number", > &in_visitor_data, > test_visitor_in_native_list_number); > + input_visitor_test_add("/visitor/input/qmp_introspect", > + &in_visitor_data, > + test_visitor_in_qmp_introspect); > =20 > g_test_run(); > =20 >=20 Starting to shape up nicely. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --RlGoj096hqQKHBIc17hS2rKlc81i15LXq Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJVsbEhAAoJEKeha0olJ0NqhpAIAInckm/Wrvu1OQsZQQtB4xs7 MtqJ52gCsO9wHspVbfAT0NV0ou+mahIwAM6jucxAE+YMnv5uAqBmA/79AU/pXccJ PwcVBz8ObeOE2vGuqFLD/dasIDnCaegYKFQfwKkqGF1pcGmXBWRXAbGDUfgYFsC4 vGp+9qhXRZbIYVgSayNnE6GkFeMZgRQqPqmyYlUqHC59hO0pd2emO8LAUpzzLKff xPN0T1vsG+7Q4odyL+9nYMXNFXMHC9HKJT3U55YPe7n+WyoQTjqGcSwHuQcFIWwB sstZrE/ElqSdMhhkUixtB2MTWzsR+zas/i9ry/AU5cdeeuFPocU3uxFxDEktc9A= =srT9 -----END PGP SIGNATURE----- --RlGoj096hqQKHBIc17hS2rKlc81i15LXq--