| Message ID | 20180629195544.34263-3-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 29, 2018 at 02:55:44PM -0500, Eric Blake wrote: > Now that we have useful access to the type name as a comment > in the generated qapi-introspect.c, we don't need to regenerate > code with a temporary -u option just to get at type names. > > Signed-off-by: Eric Blake <eblake@redhat.com> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Eric Blake <eblake@redhat.com> writes: > Now that we have useful access to the type name as a comment > in the generated qapi-introspect.c, we don't need to regenerate > code with a temporary -u option just to get at type names. > > Signed-off-by: Eric Blake <eblake@redhat.com> I occasionally feed output of query-qmp-schema to ad hoc Python scripts. My last one searched for object types containing non-string members (directly or indirectly). -u remains useful there, and having to recompile is not a problem. I'd prefer to keep it.
On 07/02/2018 01:48 PM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> Now that we have useful access to the type name as a comment >> in the generated qapi-introspect.c, we don't need to regenerate >> code with a temporary -u option just to get at type names. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> > > I occasionally feed output of query-qmp-schema to ad hoc Python scripts. > My last one searched for object types containing non-string members > (directly or indirectly). -u remains useful there, and having to > recompile is not a problem. I'd prefer to keep it. Would it be any easier to argue for the removal of -u if the generated output resembled: { "ret-type", QLIT_QSTR("1") /* Foo */ }, that is, sticking in the comment everywhere the type name is mangled to an integer, rather than just once per type?
Eric Blake <eblake@redhat.com> writes: > On 07/02/2018 01:48 PM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> Now that we have useful access to the type name as a comment >>> in the generated qapi-introspect.c, we don't need to regenerate >>> code with a temporary -u option just to get at type names. >>> >>> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> I occasionally feed output of query-qmp-schema to ad hoc Python scripts. >> My last one searched for object types containing non-string members >> (directly or indirectly). -u remains useful there, and having to >> recompile is not a problem. I'd prefer to keep it. > > Would it be any easier to argue for the removal of -u if the generated > output resembled: > > { "ret-type", QLIT_QSTR("1") /* Foo */ }, > > that is, sticking in the comment everywhere the type name is mangled > to an integer, rather than just once per type? Doesn't help with my use case. Let me elaborate on it. My ad hoc Python script gets the output of query-qmp-schema like this: pipe = subprocess.Popen('socat %s STDIO' % qmp_socket, shell=True, stdin=subprocess.PIPE, stdout=subprocess.PIPE) (out, err) = pipe.communicate('{ "execute": "qmp_capabilities" }\n' '{ "execute": "query-qmp-schema" }\n') # three lines: greeting, output of qmp_capabilities, query-qmp-schema return json.loads(out.split('\n')[2])['return'] This returns an abstract syntax tree, represented in Python the obvious way. I can then explore it in Python, say search for object types with certain properties. For example, commit 2860b2b2cb8: Thus, the flaw puts an artificial restriction on the QAPI schema: we can't have potentially empty objects and arrays within BlockdevOptions, except when they're optional and "empty" has the same meaning as "absent". --> Our QAPI schema satisfies this restriction (I checked), but it's a trap for the unwary, and a temptation to employ awkward workarounds for the wary. Let's get rid of it. I checked with a Python script that read the schema as shown above. Without -u, I'd have to revert the identifier hiding. I could certainly write some more Python to read the mapping from the generated C, but that feels like busy work.
On 07/03/2018 12:51 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 07/02/2018 01:48 PM, Markus Armbruster wrote: >>> Eric Blake <eblake@redhat.com> writes: >>> >>>> Now that we have useful access to the type name as a comment >>>> in the generated qapi-introspect.c, we don't need to regenerate >>>> code with a temporary -u option just to get at type names. >>>> > # three lines: greeting, output of qmp_capabilities, query-qmp-schema > return json.loads(out.split('\n')[2])['return'] > > This returns an abstract syntax tree, represented in Python the obvious > way. I can then explore it in Python, say search for object types with > certain properties. For example, commit 2860b2b2cb8: > > Thus, the flaw puts an artificial restriction on the QAPI schema: we > can't have potentially empty objects and arrays within > BlockdevOptions, except when they're optional and "empty" has the same > meaning as "absent". > > --> Our QAPI schema satisfies this restriction (I checked), but it's a > trap for the unwary, and a temptation to employ awkward workarounds > for the wary. Let's get rid of it. > > I checked with a Python script that read the schema as shown above. > Without -u, I'd have to revert the identifier hiding. I could certainly > write some more Python to read the mapping from the generated C, but > that feels like busy work. Good argument. Okay, I'm dropping this patch, and tweaking the other patch commit message to explain that -u is still useful, even with the addition of comment aids.
diff --git a/scripts/qapi-gen.py b/scripts/qapi-gen.py index 3d98ca2e0c6..6ec7e481b1b 100755 --- a/scripts/qapi-gen.py +++ b/scripts/qapi-gen.py @@ -26,9 +26,6 @@ def main(argv): help="write output to directory OUTPUT_DIR") parser.add_argument('-p', '--prefix', action='store', default='', help="prefix for symbols") - parser.add_argument('-u', '--unmask-non-abi-names', action='store_true', - dest='unmask', - help="expose non-ABI names in introspection") parser.add_argument('schema', action='store') args = parser.parse_args() @@ -49,7 +46,7 @@ def main(argv): gen_visit(schema, args.output_dir, args.prefix, args.builtins) gen_commands(schema, args.output_dir, args.prefix) gen_events(schema, args.output_dir, args.prefix) - gen_introspect(schema, args.output_dir, args.prefix, args.unmask) + gen_introspect(schema, args.output_dir, args.prefix) gen_doc(schema, args.output_dir, args.prefix) diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py index b37160292bc..8dd4136c0af 100644 --- a/scripts/qapi/introspect.py +++ b/scripts/qapi/introspect.py @@ -58,11 +58,10 @@ def to_c_string(string): class QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor): - def __init__(self, prefix, unmask): + def __init__(self, prefix): QAPISchemaMonolithicCVisitor.__init__( self, prefix, 'qapi-introspect', ' * QAPI/QMP schema introspection', __doc__) - self._unmask = unmask self._schema = None self._qlits = [] self._used_types = [] @@ -104,8 +103,6 @@ const QLitObject %(c_name)s = %(c_string)s; return not isinstance(entity, QAPISchemaType) def _name(self, name): - if self._unmask: - return name if name not in self._name_map: self._name_map[name] = '%d' % len(self._name_map) return self._name_map[name] @@ -131,8 +128,7 @@ const QLitObject %(c_name)s = %(c_string)s; def _gen_qlit(self, name, mtype, obj): if mtype not in ('command', 'event', 'builtin', 'array'): - if not self._unmask: - obj['comment'] = name + obj['comment'] = name name = self._name(name) obj['name'] = name obj['meta-type'] = mtype @@ -188,7 +184,7 @@ const QLitObject %(c_name)s = %(c_string)s; self._gen_qlit(name, 'event', {'arg-type': self._use_type(arg_type)}) -def gen_introspect(schema, output_dir, prefix, opt_unmask): - vis = QAPISchemaGenIntrospectVisitor(prefix, opt_unmask) +def gen_introspect(schema, output_dir, prefix): + vis = QAPISchemaGenIntrospectVisitor(prefix) schema.visit(vis) vis.write(output_dir)
Now that we have useful access to the type name as a comment in the generated qapi-introspect.c, we don't need to regenerate code with a temporary -u option just to get at type names. Signed-off-by: Eric Blake <eblake@redhat.com> --- scripts/qapi-gen.py | 5 +---- scripts/qapi/introspect.py | 12 ++++-------- 2 files changed, 5 insertions(+), 12 deletions(-)