Re: [Qemu-devel] [PATCH for-2.9 02/47] qapi: Make doc comments optional where we don't need them

[Eric Blake]

On 03/13/2017 01:18 AM, Markus Armbruster wrote:
> Since we added the documentation generator in commit 3313b61, doc
> comments are mandatory.  That's a very good idea for a schema that
> needs to be documented, but has proven to be annoying for testing.

As I've found out while rebasing my JSON output visitor as well as
patches to allow anonymous bases to flat unions.  Thanks, this will help me!

> 
> Make doc comments optional again, but add a new directive
> 
>     { 'pragma': { 'doc-required': true } }
> 
> to let a QAPI schema require them.

I like it.  It's extensible to other pragmas, as well; and reading
ahead, it looks like you did a good job of flagging unknown pragmas.

> 
> Require documentation in the schemas we actually want documented:
> qapi-schema.json and qga/qapi-schema.json.
> 
> We could probably make qapi2texi.py cope with incomplete
> documentation, but for now, simply make it refuse to run unless the
> schema has 'doc-required': true.

I'd rather fail early on our non-documented stuff then risk broken
corner cases; so I think you made the right decision.

> 
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>  docs/qapi-code-gen.txt             | 40 
> +++++++++++++++++++++++++-------------

> @@ -277,6 +280,11 @@ class QAPISchemaParser(object):
>                                         "Value of 'include' must be a string")
>                  self._include(include, info, os.path.dirname(abs_fname),
>                                previously_included)
> +            elif "pragma" in expr:
> +                if len(expr) != 1:
> +                    raise QAPISemError(info, "Invalid 'pragma' directive")

You may also want to check that you have an actual dictionary; otherwise...

> +                for name, value in expr['pragma'].iteritems():

calling .iteritems() can lead to some funky python messages. For
example, tweaking qapi-schema.json to use

{ 'pragma': [ { 'doc-required': true } ] }

=>

$ make
  GEN     qmp-commands.h
Traceback (most recent call last):
  File "/home/eblake/qemu/scripts/qapi-commands.py", line 317, in <module>
    schema = QAPISchema(input_file)
  File "/home/eblake/qemu/scripts/qapi.py", line 1496, in __init__
    parser = QAPISchemaParser(open(fname, "r"))
  File "/home/eblake/qemu/scripts/qapi.py", line 286, in __init__
    for name, value in expr['pragma'].iteritems():
AttributeError: 'list' object has no attribute 'iteritems'
Makefile:431: recipe for target 'qmp-commands.h' failed

Not the end of the world, but we've done a nice job elsewhere of
avoiding cryptic python traces.

> +        global doc_required
> +        if name == 'doc-required':
> +            if not isinstance(value, bool):
> +                raise QAPISemError(info,
> +                                   "Pragma 'doc-required' must be boolean")
> +            doc_required = value

No testsuite coverage of this message?

If you decide to not bother, or to defer error message(s) cleanup to
followups (especially since we are running short on time for fixing the
actual doc regression from going into 2.9), then using this patch as-is
can have:

Reviewed-by: Eric Blake <address@hidden>

Of course, if you spin a v2 that actually addresses my concerns, it
probably will be more involved than something that can trivially keep my
R-b (in part because you'll probably also want a testsuite addition to
cover any new error message...).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature