Re: [Qemu-devel] [PATCH 04/26] qapi: generate a literal qobject for introspection

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@redhat.com> 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 <marcandre.lureau@redhat.com>
> ---
>  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.

> +        { }
> +    }));