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

[Marc-André Lureau]

Hi

On Wed, Aug 16, 2017 at 12:21 PM, Markus Armbruster <address@hidden> wrote:
> 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".

ok

>
>>
>> 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.

It's not about suppressing whole indent, just the first.

I suggest you rewrite the code in a follow-up patch if you have an
idea how to do it.

I'd like this apporach in general for the whole series, since it is
long and tedious to deal with that many patches, and not always easy
to understand what you want.  If something works, it will be easier to
get it done and improve the code as follow-up. This approach worked
better when we ended qapi-doc for ex, and you followed up soon after
with 50 more patches ;).

>
>> +    def indent(level):
>> +        return level * 4 * ' '
>
> Blank line before and after nested function definition, please.

ok

>
>> +    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.

I can't find to_c_string(), I added one

>
>>      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.

ok

>
>> +        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.
>

ok

> 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).

hmm

>
>> +                 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.
>

ok



-- 
Marc-André Lureau