Re: [Qemu-devel] [PATCH 1/2] qapi: Add comments to aid debugging generated introspection

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 06/29/2018 02:55 PM, Eric Blake wrote:
>> We consciously chose in commit 1a9a507b to hide QAPI type names
>> from the generated introspection output, but added a command line
>> option -u to unmask the type name when doing a debug build.  At
>> that time, we generated a monolithic C string, so there was no
>> better way to do things (we could not really inject comments).
>>
>> Later, in commit 7d0f982b, we switched the generation to output
>> a QLit object, in part to make it easier for future addition of
>> conditional compilation.  But this switch has also made it easier
>> to interject strategic comments.  That commit also forgot to
>> delete some now-stale comments about long generated line lengths.
>>
>> For now, type name debug aid comments are only output once per
>> meta-type, rather than at all uses of the number used to encode
>> the type to the introspection data.  But this is still a lot
>> more convenient than having to regenerate the file with the
>> unmask operation temporarily turned on.
> ...
>
>> Signed-off-by: Eric Blake <address@hidden>
>> ---
>>   scripts/qapi/introspect.py | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>
> Misses a change to docs/devel/qapi-code.gen.tx:
>
> diff --git i/docs/devel/qapi-code-gen.txt w/docs/devel/qapi-code-gen.txt
> index 88a70e4d451..0c98c7d7b6c 100644
> --- i/docs/devel/qapi-code-gen.txt
> +++ w/docs/devel/qapi-code-gen.txt
> @@ -1392,6 +1392,7 @@ Example:
>              { }
>          })),
>          QLIT_QDICT(((QLitDictEntry[]) {
> +            /* q_empty */
>              { "members", QLIT_QLIST(((QLitObject[]) {
>                  { }
>              })) },

Yes.

>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index 6ad198ae5b5..b37160292bc 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -35,10 +35,14 @@ def to_qlit(obj, level=0, suppress_first_indent=False):
>>      elif isinstance(obj, dict):
>>          elts = []
>>          for key, value in sorted(obj.items()):
>> +            if key == 'comment':
>> +                continue
>>              elts.append(indent(level + 1) + '{ %s, %s }' %
>>                          (to_c_string(key), to_qlit(value, level + 1, True)))
>>          elts.append(indent(level + 1) + '{}')
>>          ret += 'QLIT_QDICT(((QLitDictEntry[]) {\n'
>> +        if obj.get('comment'):
>> +            ret += indent(level + 1) + '/* %s */\n' % obj['comment']
>>          ret += ',\n'.join(elts) + '\n'
>>          ret += indent(level) + '}))'
>>      elif isinstance(obj, bool):

to_qlit() converts from a natural representation of JSON in Python to
JSON text.

Your patch breaks it for JSON objects that have a member named
'comment'.

Related work: Marc-André's "[PATCH v6 09/15] qapi-introspect: add
preprocessor conditions to generated QLit" uses tuples to augment JSON.
He uses them for conditionals, but the technique could be generalized,
say from his (jobj, ifcond) to (jobj, { 'if': ifcond, '#': comment }.

>> @@ -78,7 +82,6 @@ class 
>> QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor):
>>          for typ in self._used_types:
>>              typ.visit(self)
>>          # generate C
>> -        # TODO can generate awfully long lines
>>          name = c_name(self._prefix, protect=False) + 'qmp_schema_qlit'
>>          self._genh.add(mcgen('''
>>  #include "qapi/qmp/qlit.h"
>> @@ -118,8 +121,8 @@ const QLitObject %(c_name)s = %(c_string)s;
>>           if typ not in self._used_types:
>>               self._used_types.append(typ)
>>          # Clients should examine commands and events, not types.  Hide
>> -        # type names to reduce the temptation.  Also saves a few
>> -        # characters.
>> +        # type names as integers to reduce the temptation, but provide
>> +        # comments for debugging aid.
>
> When I wrote the patch, I assumed the final sentence of the comment
> was talking about fewer characters in the generated .c file (I nuked
> it, since adding comments makes for a longer, not shorter, generated
> .c file); but now that I'm writing this email, I see it could also
> refer to fewer characters sent over the wire in the QMP command (which

Yes, that was my intent.

> length is unchanged by this patch, but was a 13KiB savings compared to
> the unmasked version, per commit 1a9a507b).  So feel free to bikeshed
> this comment.

I think I'd merely clarify the comment's last sentence by adding "on the
wire", ...

>>          if isinstance(typ, QAPISchemaBuiltinType):
>>              return typ.name
>>          if isinstance(typ, QAPISchemaArrayType):
>> @@ -128,6 +131,8 @@ 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:

... and perhaps briefly explain here why we generate a comment.

>> +                obj['comment'] = name
>>              name = self._name(name)
>>          obj['name'] = name
>>          obj['meta-type'] = mtype