[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 04/26] qapi: generate a literal qobject for intr
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 04/26] qapi: generate a literal qobject for introspection |
Date: |
Wed, 16 Aug 2017 12:21:21 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) |
Marc-André Lureau <address@hidden> writes:
> Replace the generated json string with a literal qobject. The later is
> easier to deal with, at run time, as well as compile time:
at run time as well as compile time
> #if blocks
> can be more easily added than in a json string.
Future tense, e.g. "adding #if conditionals will be easier".
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
> scripts/qapi-introspect.py | 83
> +++++++++++++++++++++-----------------
> monitor.c | 2 +-
> tests/test-qobject-input-visitor.c | 10 +++--
> docs/devel/qapi-code-gen.txt | 29 ++++++++-----
> 4 files changed, 72 insertions(+), 52 deletions(-)
>
> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
> index 032bcea491..fc72cdb66d 100644
> --- a/scripts/qapi-introspect.py
> +++ b/scripts/qapi-introspect.py
> @@ -12,72 +12,74 @@
> from qapi import *
>
>
> -# Caveman's json.dumps() replacement (we're stuck at Python 2.4)
> -# TODO try to use json.dumps() once we get unstuck
> -def to_json(obj, level=0):
> +def to_qlit(obj, level=0, first_indent=True):
Suggest suppress_indent=False. I prefer defaulting to False.
> + def indent(level):
> + return level * 4 * ' '
Blank line before and after nested function definition, please.
> + ret = ''
> + if first_indent:
> + ret += indent(level)
> if obj is None:
> - ret = 'null'
> + ret += 'QLIT_QNULL'
> elif isinstance(obj, str):
> - ret = '"' + obj.replace('"', r'\"') + '"'
> + ret += 'QLIT_QSTR(' + '"' + obj.replace('"', r'\"') + '"' + ')'
Why not
ret += 'QLIT_QSTR("' + obj.replace('"', r'\"') + '")'
Hmm, make that
ret += 'QLIT_QSTR(' + to_c_string(obj) + ')'
because it makes the meaning more obvious, and it's also more robust: it
doubles backslashes.
> elif isinstance(obj, list):
> - elts = [to_json(elt, level + 1)
> + elts = [to_qlit(elt, level + 1)
> for elt in obj]
> - ret = '[' + ', '.join(elts) + ']'
> + elts.append(indent(level + 1) + "{ }")
I'd prefer "{}". More of the same below.
> + ret += 'QLIT_QLIST(((QLitObject[]) {\n'
> + ret += ',\n'.join(elts) + '\n'
> + ret += indent(level) + '}))'
The extra pair of parenthesis in QLIT_QLIST(( ... )) is slightly ugly.
Not this patch's fault. Same for QLIT_QDICT(( ... )) below.
> elif isinstance(obj, dict):
> - elts = ['"%s": %s' % (key.replace('"', r'\"'),
> - to_json(obj[key], level + 1))
> - for key in sorted(obj.keys())]
> - ret = '{' + ', '.join(elts) + '}'
> + elts = [ indent(level + 1) + '{ "%s", %s }' %
> + (key.replace('"', r'\"'), to_qlit(obj[key], level + 1,
> False))
$ pep8 scripts/qapi-introspect.py
scripts/qapi-introspect.py:33:17: E201 whitespace after '['
Also, break lines at operators with the least precedence, not in the
middle of sub-expressions.
However
elts = [indent(level + 1)
+ ('{ "%s", %s }'
% (to_c_string(key),
to_qlit(obj[key], level + 1, suppress_indent=True)))
for key in sorted(obj.keys())]
is still illegible. I'm afraid this is simply too complex for a list
comprehension. Try rewriting as a loop.
Another option would be separating off indentation: generate the C
initializer unindented, then feed it to a stupid indenter that counts
parentheses (round, square and curly).
> + for key in sorted(obj.keys())]
> + elts.append(indent(level + 1) + '{ }')
> + ret += 'QLIT_QDICT(((QLitDictEntry[]) {\n'
> + ret += ',\n'.join(elts) + '\n'
> + ret += indent(level) + '}))'
> else:
> assert False # not implemented
> - if level == 1:
> - ret = '\n' + ret
> return ret
>
>
> -def to_c_string(string):
> - return '"' + string.replace('\\', r'\\').replace('"', r'\"') + '"'
> -
> -
> class QAPISchemaGenIntrospectVisitor(QAPISchemaVisitor):
> def __init__(self, unmask):
> self._unmask = unmask
> self.defn = None
> self.decl = None
> self._schema = None
> - self._jsons = None
> + self._qlits = None
> self._used_types = None
> self._name_map = None
>
> def visit_begin(self, schema):
> self._schema = schema
> - self._jsons = []
> + self._qlits = []
> self._used_types = []
> self._name_map = {}
>
> def visit_end(self):
> # visit the types that are actually used
> - jsons = self._jsons
> - self._jsons = []
> + qlits = self._qlits
> + self._qlits = []
> for typ in self._used_types:
> typ.visit(self)
> # generate C
> # TODO can generate awfully long lines
> - jsons.extend(self._jsons)
> - name = c_name(prefix, protect=False) + 'qmp_schema_json'
> + qlits.extend(self._qlits)
> + name = c_name(prefix, protect=False) + 'qmp_schema_qlit'
> self.decl = mcgen('''
> -extern const char %(c_name)s[];
> +extern const QLitObject %(c_name)s;
> ''',
> c_name=c_name(name))
> - lines = to_json(jsons).split('\n')
> - c_string = '\n '.join([to_c_string(line) for line in lines])
> + c_string = to_qlit(qlits)
The value is simple, and it's used just once. Let's eliminate the
variable.
> self.defn = mcgen('''
> -const char %(c_name)s[] = %(c_string)s;
> +const QLitObject %(c_name)s = %(c_string)s;
> ''',
> c_name=c_name(name),
> c_string=c_string)
> self._schema = None
> - self._jsons = None
> + self._qlits = None
> self._used_types = None
> self._name_map = None
>
> @@ -111,12 +113,12 @@ const char %(c_name)s[] = %(c_string)s;
> return '[' + self._use_type(typ.element_type) + ']'
> return self._name(typ.name)
>
> - def _gen_json(self, name, mtype, obj):
> + def _gen_qlit(self, name, mtype, obj):
> if mtype not in ('command', 'event', 'builtin', 'array'):
> name = self._name(name)
> obj['name'] = name
> obj['meta-type'] = mtype
> - self._jsons.append(obj)
> + self._qlits.append(obj)
>
> def _gen_member(self, member):
> ret = {'name': member.name, 'type': self._use_type(member.type)}
> @@ -132,24 +134,24 @@ const char %(c_name)s[] = %(c_string)s;
> return {'case': variant.name, 'type': self._use_type(variant.type)}
>
> def visit_builtin_type(self, name, info, json_type):
> - self._gen_json(name, 'builtin', {'json-type': json_type})
> + self._gen_qlit(name, 'builtin', {'json-type': json_type})
>
> def visit_enum_type(self, name, info, values, prefix):
> - self._gen_json(name, 'enum', {'values': values})
> + self._gen_qlit(name, 'enum', {'values': values})
>
> def visit_array_type(self, name, info, element_type):
> element = self._use_type(element_type)
> - self._gen_json('[' + element + ']', 'array', {'element-type':
> element})
> + self._gen_qlit('[' + element + ']', 'array', {'element-type':
> element})
>
> def visit_object_type_flat(self, name, info, members, variants):
> obj = {'members': [self._gen_member(m) for m in members]}
> if variants:
> obj.update(self._gen_variants(variants.tag_member.name,
> variants.variants))
> - self._gen_json(name, 'object', obj)
> + self._gen_qlit(name, 'object', obj)
>
> def visit_alternate_type(self, name, info, variants):
> - self._gen_json(name, 'alternate',
> + self._gen_qlit(name, 'alternate',
> {'members': [{'type': self._use_type(m.type)}
> for m in variants.variants]})
>
> @@ -157,13 +159,13 @@ const char %(c_name)s[] = %(c_string)s;
> gen, success_response, boxed):
> arg_type = arg_type or self._schema.the_empty_object_type
> ret_type = ret_type or self._schema.the_empty_object_type
> - self._gen_json(name, 'command',
> + self._gen_qlit(name, 'command',
> {'arg-type': self._use_type(arg_type),
> 'ret-type': self._use_type(ret_type)})
>
> def visit_event(self, name, info, arg_type, boxed):
> arg_type = arg_type or self._schema.the_empty_object_type
> - self._gen_json(name, 'event', {'arg-type': self._use_type(arg_type)})
> + self._gen_qlit(name, 'event', {'arg-type': self._use_type(arg_type)})
>
> # Debugging aid: unmask QAPI schema's type names
> # We normally mask them, because they're not QMP wire ABI
> @@ -205,11 +207,18 @@ h_comment = '''
>
> fdef.write(mcgen('''
> #include "qemu/osdep.h"
> +#include "qapi/qmp/qlit.h"
> #include "%(prefix)sqmp-introspect.h"
>
> ''',
> prefix=prefix))
>
> +fdecl.write(mcgen('''
> +#include "qemu/osdep.h"
> +#include "qapi/qmp/qlit.h"
> +
> +'''))
> +
> schema = QAPISchema(input_file)
> gen = QAPISchemaGenIntrospectVisitor(opt_unmask)
> schema.visit(gen)
> diff --git a/monitor.c b/monitor.c
> index 6d040e620f..a1773d5bc7 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -953,7 +953,7 @@ EventInfoList *qmp_query_events(Error **errp)
> static void qmp_query_qmp_schema(QDict *qdict, QObject **ret_data,
> Error **errp)
> {
> - *ret_data = qobject_from_json(qmp_schema_json, &error_abort);
> + *ret_data = qobject_from_qlit(&qmp_schema_qlit);
> }
>
> /*
> diff --git a/tests/test-qobject-input-visitor.c
> b/tests/test-qobject-input-visitor.c
> index bcf02617dc..1969733971 100644
> --- a/tests/test-qobject-input-visitor.c
> +++ b/tests/test-qobject-input-visitor.c
> @@ -1247,24 +1247,26 @@ static void
> test_visitor_in_fail_alternate(TestInputVisitorData *data,
> }
>
> static void do_test_visitor_in_qmp_introspect(TestInputVisitorData *data,
> - const char *schema_json)
> + const QLitObject *qlit)
> {
> SchemaInfoList *schema = NULL;
> + QObject *obj = qobject_from_qlit(qlit);
> Visitor *v;
>
> - v = visitor_input_test_init_raw(data, schema_json);
> + v = qobject_input_visitor_new(obj);
>
> visit_type_SchemaInfoList(v, NULL, &schema, &error_abort);
> g_assert(schema);
>
> qapi_free_SchemaInfoList(schema);
> + qobject_decref(obj);
> }
>
> 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);
> + do_test_visitor_in_qmp_introspect(data, &test_qmp_schema_qlit);
> + do_test_visitor_in_qmp_introspect(data, &qmp_schema_qlit);
> }
>
These tests are only marginally useful now. Before, they ensured that a
qapi-introspect.py generating invalid JSON fails at "make check" compile
time. Such bugs should now fail when we compile the generated
qapi-introspect.c.
> int main(int argc, char **argv)
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index 9903ac4c19..885c61b52f 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -1295,18 +1295,27 @@ Example:
> #ifndef EXAMPLE_QMP_INTROSPECT_H
> #define EXAMPLE_QMP_INTROSPECT_H
>
> - extern const char example_qmp_schema_json[];
> + extern const QLitObject qmp_schema_qlit;
>
> #endif
> $ cat qapi-generated/example-qmp-introspect.c
> [Uninteresting stuff omitted...]
>
> - const char example_qmp_schema_json[] = "["
> - "{\"arg-type\": \"0\", \"meta-type\": \"event\", \"name\":
> \"MY_EVENT\"}, "
> - "{\"arg-type\": \"1\", \"meta-type\": \"command\", \"name\":
> \"my-command\", \"ret-type\": \"2\"}, "
> - "{\"members\": [], \"meta-type\": \"object\", \"name\": \"0\"}, "
> - "{\"members\": [{\"name\": \"arg1\", \"type\": \"[2]\"}],
> \"meta-type\": \"object\", \"name\": \"1\"}, "
> - "{\"members\": [{\"name\": \"integer\", \"type\": \"int\"},
> {\"default\": null, \"name\": \"string\", \"type\": \"str\"}], \"meta-type\":
> \"object\", \"name\": \"2\"}, "
> - "{\"element-type\": \"2\", \"meta-type\": \"array\", \"name\":
> \"[2]\"}, "
> - "{\"json-type\": \"int\", \"meta-type\": \"builtin\", \"name\":
> \"int\"}, "
> - "{\"json-type\": \"string\", \"meta-type\": \"builtin\", \"name\":
> \"str\"}]";
> + const QLitObject example_qmp_schema_qlit = QLIT_QLIST(((QLitObject[]) {
> + QLIT_QDICT(((QLitDictEntry[]) {
> + { "arg-type", QLIT_QSTR("0") },
> + { "meta-type", QLIT_QSTR("event") },
> + { "name", QLIT_QSTR("Event") },
> + { }
> + })),
> + QLIT_QDICT(((QLitDictEntry[]) {
> + { "members", QLIT_QLIST(((QLitObject[]) {
> + { }
> + })) },
> + { "meta-type", QLIT_QSTR("object") },
> + { "name", QLIT_QSTR("0") },
> + { }
> + })),
> + ....
Ellipsis is three dots, not four :)
Output is no longer complete (less file boilerplate). Not an issue now,
but it might become one when we make the examples testable. We can
restore the deleted output then.
> + { }
> + }));
- Re: [Qemu-devel] [PATCH 04/26] qapi: generate a literal qobject for introspection,
Markus Armbruster <=