[Qemu-devel] [PATCH v2 0/2] qapi: easier debugging of introspection file

[Qemu-devel] [PATCH v2 0/2] qapi: easier debugging of introspection file

[Eric Blake]

When inspecting the generated qapi-introspect.c (for debugging,
or to see what QAPI changes are user visible vs. internal only),
the fact that we've intentionally masked names from the QMP client
makes it harder to tie back generated code back to the original
QAPI .json files.  We have a -u switch to qapi-gen for temporarily
bypassing the name masking, but that's a rather heavy-handed
tactic just for some temporary debugging.  Better is to just make
the generated file include strategic comments.

v1 was sent back in June, but was stalled due to 3.0 hard freeze.
Now that 3.1 is open, it's time to revisit this.

Since then:
- drop the original patch 2 (deleting --unmask proved controversial) [Markus]
- rebase patch on top of Marc-Andre's conditional work [Markus]
- update documentation to match code change [Eric]
- split out a preliminary cleanup patch [pep8]

Eric Blake (2):
  qapi: Minor introspect.py cleanups
  qapi: Add comments to aid debugging generated introspection

 docs/devel/qapi-code-gen.txt |  1 +
 scripts/qapi/introspect.py   | 34 ++++++++++++++++++++++++----------
 2 files changed, 25 insertions(+), 10 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v2 1/2] qapi: Minor introspect.py cleanups

[Eric Blake]

Commit 7d0f982b changed generated introspection output to no longer
produce long lines in the generated .c file, but failed to adjust
comments to match.  Add some clarity that the shorter length that
matters most is the overall QMP response on the wire.

Commit 25b1ef31 triggers a pep8 formatting nit.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi/introspect.py | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 189a4edabab..43e81a06938 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -89,7 +89,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"
@@ -129,8 +128,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.  Also, it
+        # saves a few characters on the wire.
         if isinstance(typ, QAPISchemaBuiltinType):
             return typ.name
         if isinstance(typ, QAPISchemaArrayType):
@@ -185,7 +184,7 @@ const QLitObject %(c_name)s = %(c_string)s;
         arg_type = arg_type or self._schema.the_empty_object_type
         ret_type = ret_type or self._schema.the_empty_object_type
         obj = {'arg-type': self._use_type(arg_type),
-               'ret-type': self._use_type(ret_type) }
+               'ret-type': self._use_type(ret_type)}
         if allow_oob:
             obj['allow-oob'] = allow_oob
         self._gen_qlit(name, 'command', obj, ifcond)
-- 
2.17.1

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

[Eric Blake]

We consciously chose in commit 1a9a507b to hide QAPI type names
from the introspection output on the wire, but added a command
line option -u to unmask the type name when doing a debug build.
The unmask option still remains useful to some other forms of
automated analysis, so it will not be removed; however, when it
is not in use, the generated .c file can be hard to read.  At
the time when we first introduced masking, the generated file
consisted only of a monolithic C string, so there was no clean
way to inject any 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.  In fact, commit d626b6c1 took advantage
of this by passing a tuple instead of a bare object for encoding
the output of conditionals.  By extending that tuple, we can now
interject strategic comments.

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 within the introspection data.  But this is still a lot
more convenient than having to regenerate the file with the
unmask operation temporarily turned on - merely search the
generated file for '"NNN" =' to learn the corresponding source
name and associated definition of type NNN.

The generated qapi-introspect.c changes only with the addition
of comments, such as:

| @@ -14755,6 +15240,7 @@
|          { "name", QLIT_QSTR("[485]"), },
|          {}
|      })),
| +    /* "485" = QCryptoBlockInfoLUKSSlot */
|      QLIT_QDICT(((QLitDictEntry[]) {
|          { "members", QLIT_QLIST(((QLitObject[]) {
|              QLIT_QDICT(((QLitDictEntry[]) {

Signed-off-by: Eric Blake <eblake@redhat.com>

---
v2: rebase on conditional code additions, drop patch to remove -u,
update documentation to match
---
 docs/devel/qapi-code-gen.txt |  1 +
 scripts/qapi/introspect.py   | 27 +++++++++++++++++++++------
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index c2e11465f0f..9d5b1409e72 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -1412,6 +1412,7 @@ Example:
             { "name", QLIT_QSTR("Event") },
             { }
         })),
+        /* "0" = q_empty */
         QLIT_QDICT(((QLitDictEntry[]) {
             { "members", QLIT_QLIST(((QLitObject[]) {
                 { }
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 43e81a06938..67d6106f77b 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -19,12 +19,17 @@ def to_qlit(obj, level=0, suppress_first_indent=False):
         return level * 4 * ' '

     if isinstance(obj, tuple):
-        ifobj, ifcond = obj
-        ret = gen_if(ifcond)
+        ifobj, extra = obj
+        ifcond = extra.get('if')
+        comment = extra.get('comment')
+        ret = ''
+        if comment:
+            ret += indent(level) + '/* %s */\n' % comment
+        if ifcond:
+            ret += gen_if(ifcond)
         ret += to_qlit(ifobj, level)
-        endif = gen_endif(ifcond)
-        if endif:
-            ret += '\n' + endif
+        if ifcond:
+            ret += '\n' + gen_endif(ifcond)
         return ret

     ret = ''
@@ -137,11 +142,21 @@ const QLitObject %(c_name)s = %(c_string)s;
         return self._name(typ.name)

     def _gen_qlit(self, name, mtype, obj, ifcond):
+        extra = {}
         if mtype not in ('command', 'event', 'builtin', 'array'):
+            if not self._unmask:
+                # Output a comment to make it easy to map masked names
+                # back to the source when reading the generated output.
+                extra['comment'] = '"%s" = %s' % (self._name(name), name)
             name = self._name(name)
         obj['name'] = name
         obj['meta-type'] = mtype
-        self._qlits.append((obj, ifcond))
+        if ifcond:
+            extra['if'] = ifcond
+        if extra:
+            self._qlits.append((obj, extra))
+        else:
+            self._qlits.append(obj)

     def _gen_member(self, member):
         ret = {'name': member.name, 'type': self._use_type(member.type)}
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 1/2] qapi: Minor introspect.py cleanups

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> Commit 7d0f982b changed generated introspection output to no longer
> produce long lines in the generated .c file, but failed to adjust
> comments to match.  Add some clarity that the shorter length that
> matters most is the overall QMP response on the wire.
>
> Commit 25b1ef31 triggers a pep8 formatting nit.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> We consciously chose in commit 1a9a507b to hide QAPI type names
> from the introspection output on the wire, but added a command
> line option -u to unmask the type name when doing a debug build.
> The unmask option still remains useful to some other forms of
> automated analysis, so it will not be removed; however, when it
> is not in use, the generated .c file can be hard to read.  At
> the time when we first introduced masking, the generated file
> consisted only of a monolithic C string, so there was no clean
> way to inject any 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.  In fact, commit d626b6c1 took advantage
> of this by passing a tuple instead of a bare object for encoding
> the output of conditionals.  By extending that tuple, we can now
> interject strategic comments.
>
> 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 within the introspection data.  But this is still a lot
> more convenient than having to regenerate the file with the
> unmask operation temporarily turned on - merely search the
> generated file for '"NNN" =' to learn the corresponding source
> name and associated definition of type NNN.
>
> The generated qapi-introspect.c changes only with the addition
> of comments, such as:
>
> | @@ -14755,6 +15240,7 @@
> |          { "name", QLIT_QSTR("[485]"), },
> |          {}
> |      })),
> | +    /* "485" = QCryptoBlockInfoLUKSSlot */
> |      QLIT_QDICT(((QLitDictEntry[]) {
> |          { "members", QLIT_QLIST(((QLitObject[]) {
> |              QLIT_QDICT(((QLitDictEntry[]) {
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v2: rebase on conditional code additions, drop patch to remove -u,
> update documentation to match
> ---
>  docs/devel/qapi-code-gen.txt |  1 +
>  scripts/qapi/introspect.py   | 27 +++++++++++++++++++++------
>  2 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index c2e11465f0f..9d5b1409e72 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -1412,6 +1412,7 @@ Example:
>              { "name", QLIT_QSTR("Event") },
>              { }
>          })),
> +        /* "0" = q_empty */
>          QLIT_QDICT(((QLitDictEntry[]) {
>              { "members", QLIT_QLIST(((QLitObject[]) {
>                  { }

Let's rebase onto my "[PATCH 0/2] qapi: A whitespace touch-up, and a doc
update".  The appended fixup needs to be squashed in then.  Happy to do
that in my tree.

> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 43e81a06938..67d6106f77b 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -19,12 +19,17 @@ def to_qlit(obj, level=0, suppress_first_indent=False):
>          return level * 4 * ' '
>
>      if isinstance(obj, tuple):
> -        ifobj, ifcond = obj
> -        ret = gen_if(ifcond)
> +        ifobj, extra = obj
> +        ifcond = extra.get('if')
> +        comment = extra.get('comment')
> +        ret = ''
> +        if comment:
> +            ret += indent(level) + '/* %s */\n' % comment
> +        if ifcond:
> +            ret += gen_if(ifcond)
>          ret += to_qlit(ifobj, level)
> -        endif = gen_endif(ifcond)
> -        if endif:
> -            ret += '\n' + endif
> +        if ifcond:
> +            ret += '\n' + gen_endif(ifcond)
>          return ret
>
>      ret = ''
> @@ -137,11 +142,21 @@ const QLitObject %(c_name)s = %(c_string)s;
>          return self._name(typ.name)
>
>      def _gen_qlit(self, name, mtype, obj, ifcond):
> +        extra = {}
>          if mtype not in ('command', 'event', 'builtin', 'array'):
> +            if not self._unmask:
> +                # Output a comment to make it easy to map masked names
> +                # back to the source when reading the generated output.
> +                extra['comment'] = '"%s" = %s' % (self._name(name), name)
>              name = self._name(name)
>          obj['name'] = name
>          obj['meta-type'] = mtype
> -        self._qlits.append((obj, ifcond))
> +        if ifcond:
> +            extra['if'] = ifcond
> +        if extra:
> +            self._qlits.append((obj, extra))
> +        else:
> +            self._qlits.append(obj)
>
>      def _gen_member(self, member):
>          ret = {'name': member.name, 'type': self._use_type(member.type)}

Reviewed-by: Markus Armbruster <armbru@redhat.com>


diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index a4091a7d78..f9990808de 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -1430,7 +1430,7 @@ Example:
             { "name", QLIT_QSTR("MY_EVENT"), },
             {}
         })),
-        /* "0" = q_empty */
+        /* "0" = q_obj_my-command-arg */
         QLIT_QDICT(((QLitDictEntry[]) {
             { "members", QLIT_QLIST(((QLitObject[]) {
                 QLIT_QDICT(((QLitDictEntry[]) {
@@ -1444,6 +1444,7 @@ Example:
             { "name", QLIT_QSTR("0"), },
             {}
         })),
+        /* "1" = UserDefOne */
         QLIT_QDICT(((QLitDictEntry[]) {
             { "members", QLIT_QLIST(((QLitObject[]) {
                 QLIT_QDICT(((QLitDictEntry[]) {
@@ -1463,6 +1464,7 @@ Example:
             { "name", QLIT_QSTR("1"), },
             {}
         })),
+        /* "2" = q_empty */
         QLIT_QDICT(((QLitDictEntry[]) {
             { "members", QLIT_QLIST(((QLitObject[]) {
                 {}

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

[Eric Blake]

On 08/28/2018 07:27 AM, Markus Armbruster wrote:

>> ---
>> v2: rebase on conditional code additions, drop patch to remove -u,
>> update documentation to match

My update wasn't very accurate, as you discovered :)

> 
> Let's rebase onto my "[PATCH 0/2] qapi: A whitespace touch-up, and a doc
> update".  The appended fixup needs to be squashed in then.  Happy to do
> that in my tree.

Yes, that makes the most sense.  Go ahead!

> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 
> 
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index a4091a7d78..f9990808de 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -1430,7 +1430,7 @@ Example:
>               { "name", QLIT_QSTR("MY_EVENT"), },
>               {}
>           })),
> -        /* "0" = q_empty */
> +        /* "0" = q_obj_my-command-arg */
>           QLIT_QDICT(((QLitDictEntry[]) {
>               { "members", QLIT_QLIST(((QLitObject[]) {
>                   QLIT_QDICT(((QLitDictEntry[]) {
> @@ -1444,6 +1444,7 @@ Example:
>               { "name", QLIT_QSTR("0"), },
>               {}
>           })),
> +        /* "1" = UserDefOne */
>           QLIT_QDICT(((QLitDictEntry[]) {
>               { "members", QLIT_QLIST(((QLitObject[]) {
>                   QLIT_QDICT(((QLitDictEntry[]) {
> @@ -1463,6 +1464,7 @@ Example:
>               { "name", QLIT_QSTR("1"), },
>               {}
>           })),
> +        /* "2" = q_empty */
>           QLIT_QDICT(((QLitDictEntry[]) {
>               { "members", QLIT_QLIST(((QLitObject[]) {
>                   {}
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2 0/2] qapi: easier debugging of introspection file

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> When inspecting the generated qapi-introspect.c (for debugging,
> or to see what QAPI changes are user visible vs. internal only),
> the fact that we've intentionally masked names from the QMP client
> makes it harder to tie back generated code back to the original
> QAPI .json files.  We have a -u switch to qapi-gen for temporarily
> bypassing the name masking, but that's a rather heavy-handed
> tactic just for some temporary debugging.  Better is to just make
> the generated file include strategic comments.
>
> v1 was sent back in June, but was stalled due to 3.0 hard freeze.
> Now that 3.1 is open, it's time to revisit this.
>
> Since then:
> - drop the original patch 2 (deleting --unmask proved controversial) [Markus]
> - rebase patch on top of Marc-Andre's conditional work [Markus]
> - update documentation to match code change [Eric]
> - split out a preliminary cleanup patch [pep8]

Queued.  Thanks!