qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v5 13/17] qapi: add qapi2texi script


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v5 13/17] qapi: add qapi2texi script
Date: Wed, 30 Nov 2016 17:06:06 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

Marc-André Lureau <address@hidden> writes:

> As the name suggests, the qapi2texi script converts JSON QAPI
> description into a texi file suitable for different target
> formats (info/man/txt/pdf/html...).
>
> It parses the following kind of blocks:
>
> Free-form:
>
>   ##
>   # = Section
>   # == Subsection
>   #
>   # Some text foo with *emphasis*
>   # 1. with a list
>   # 2. like that
>   #
>   # And some code:
>   # | $ echo foo
>   # | -> do this
>   # | <- get that
>   #
>   ##
>
> Symbol:
>
>   ##
>   # @symbol:
>   #
>   # Symbol body ditto ergo sum. Foo bar
>   # baz ding.
>   #
>   # @arg: foo
>   # @arg: #optional foo

Let's not use @arg twice.

Terminology: I prefer to use "parameter" for formal parameters, and
"argument" for actual arguments.  This matches how The Python Language
Reference uses the terms.

What about

    # @param1: the frob to frobnicate
    # @param2: #optional how hard to frobnicate

>   #
>   # Returns: returns bla bla
>   #          Or bla blah

Repeating "returns" is awkward, and we don't do that in our schemas.

We need a period before "Or", or spell it "or".

What about

    # Returns: the frobnicated frob.
    #          If frob isn't frobnicatable, GenericError.

>   #
>   # Since: version
>   # Notes: notes, comments can have
>   #        - itemized list
>   #        - like this
>   #
>   # Example:
>   #
>   # -> { "execute": "quit" }
>   # <- { "return": {} }
>   #
>   ##
>
> That's roughly following the following EBNF grammar:
>
> api_comment = "##\n" comment "##\n"
> comment = freeform_comment | symbol_comment
> freeform_comment = { "# " text "\n" | "#\n" }
> symbol_comment = "# @" name ":\n" { member | meta | freeform_comment }

Rejects non-empty comments where "#" is not followed by space.  Such
usage is accepted outside doc comments.  Hmm.

> member = "# @" name ':' [ text ] freeform_comment

Are you missing a "\n" before freeform_comment?

> meta = "# " ( "Returns:", "Since:", "Note:", "Notes:", "Example:", 
> "Examples:" ) [ text ] freeform_comment

Likewise.

> text = free-text markdown-like, "#optional" for members

The grammar is ambiguous: a line "# @foo:\n" can be parsed both as
freeform_comment and as symbol_comment.  Since your intent is obvious
enough, it can still serve as documentation.  It's not a suitable
foundation for parsing, though.  Okay for a commit message.

> Thanks to the following json expressions, the documentation is enhanced
> with extra information about the type of arguments and return value
> expected.

I guess you want to say that we enrich the documentation we extract from
comments with information from the actual schema.  Correct?

Missing: a brief discussion of deficiencies.  These include:

* The generated QMP documentation includes internal types

  We use qapi-schema.json both for defining the external QMP interface
  and for defining internal types.  qmp-introspect.py carefully
  separates the two, to not expose internal types.  qapi2texi.py happily
  exposes everything.

  Currently, about a fifth of the types in the generated docs are
  internal:

      AcpiTableOptions
      BiosAtaTranslation
      BlockDeviceMapEntry
      COLOMessage
      COLOMode
      DummyForceArrays
      FailoverStatus
      FloppyDriveType
      ImageCheck
      LostTickPolicy
      MapEntry
      MigrationParameter
      NetClientDriver
      NetFilterDirection
      NetLegacy
      NetLegacyNicOptions
      NetLegacyOptions
      NetLegacyOptionsKind
      Netdev
      NetdevBridgeOptions
      NetdevDumpOptions
      NetdevHubPortOptions
      NetdevL2TPv3Options
      NetdevNetmapOptions
      NetdevNoneOptions
      NetdevSocketOptions
      NetdevTapOptions
      NetdevUserOptions
      NetdevVdeOptions
      NetdevVhostUserOptions
      NumaNodeOptions
      NumaOptions
      NumaOptionsKind
      OnOffAuto
      OnOffSplit
      PreallocMode
      QCryptoBlockCreateOptions
      QCryptoBlockCreateOptionsLUKS
      QCryptoBlockFormat
      QCryptoBlockInfo
      QCryptoBlockInfoBase
      QCryptoBlockInfoQCow
      QCryptoBlockOpenOptions
      QCryptoBlockOptionsBase
      QCryptoBlockOptionsLUKS
      QCryptoBlockOptionsQCow
      QCryptoSecretFormat
      QCryptoTLSCredsEndpoint
      QapiErrorClass
      ReplayMode
      X86CPUFeatureWordInfo
      X86CPURegister32

  Generating documentation for internal types might be useful, but
  letting them pollute QMP interface documentation isn't.  Needs fixing
  before we release.  Until then, needs a FIXME comment in qapi2texi.py.

* Union support is lacking

  The doc string language is adequate for documenting commands, events,
  and non-union types.  It doesn't really handle union variants.  Hardly
  surprising, as you fitted the language do existing practice, and
  existing (mal-)practice is neglecting to document union variant
  members.

* Documentation is lacking

  See review of qapi-code-gen.txt below.

* Doc comment error message positions are imprecise

  They always point to the beginning of the comment.

* Probably more

  We should update this with noteworthy findings during review.  I
  tried, but I suspect I missed some.

> Signed-off-by: Marc-André Lureau <address@hidden>
[Lengthy diffstat snipped...]
>
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index e98d3b6..f16764c 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -350,6 +350,21 @@ qapi-schema += base-cycle-direct.json
>  qapi-schema += base-cycle-indirect.json
>  qapi-schema += command-int.json
>  qapi-schema += comments.json
> +qapi-schema += doc-bad-args.json
> +qapi-schema += doc-bad-symbol.json
> +qapi-schema += doc-duplicated-arg.json
> +qapi-schema += doc-duplicated-return.json
> +qapi-schema += doc-duplicated-since.json
> +qapi-schema += doc-empty-arg.json
> +qapi-schema += doc-empty-section.json
> +qapi-schema += doc-empty-symbol.json
> +qapi-schema += doc-invalid-end.json
> +qapi-schema += doc-invalid-end2.json
> +qapi-schema += doc-invalid-return.json
> +qapi-schema += doc-invalid-section.json
> +qapi-schema += doc-invalid-start.json
> +qapi-schema += doc-missing-expr.json
> +qapi-schema += doc-missing-space.json
>  qapi-schema += double-data.json
>  qapi-schema += double-type.json
>  qapi-schema += duplicate-key.json
> @@ -443,6 +458,8 @@ qapi-schema += union-optional-branch.json
>  qapi-schema += union-unknown.json
>  qapi-schema += unknown-escape.json
>  qapi-schema += unknown-expr-key.json
> +
> +
>  check-qapi-schema-y := $(addprefix tests/qapi-schema/, $(qapi-schema))
>  
>  GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h \
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 4d1b0e4..1b456b4 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -122,6 +122,109 @@ class QAPILineError(Exception):
>              "%s:%d: %s" % (self.info['file'], self.info['line'], self.msg)
>  
>  
> +class QAPIDoc(object):
> +    class Section(object):
> +        def __init__(self, name=""):

name=None feels more natural than "" for an absent optional name.


> +            # optional section name (argument/member or section name)
> +            self.name = name
> +            # the list of strings for this section

Would "list of lines" be more accurate, or less?

> +            self.content = []
> +
> +        def append(self, line):
> +            self.content.append(line)
> +
> +        def __repr__(self):
> +            return "\n".join(self.content).strip()
> +
> +    class ArgSection(Section):
> +        pass

Begs the question what makes ArgSection differ from Section.  I think
it's the semantics of self.name: an ArgSection's name is the parameter
name, a non-ArgSection can either be a "meta" section (name is one of
"Returns"", "Since", "Note", "Notes", "Example", "Examples") or an
anonymous section (name is "").

> +
> +    def __init__(self, parser):
> +        self.parser = parser
> +        self.symbol = None
> +        self.body = QAPIDoc.Section()
> +        # a dict {'arg': ArgSection, ...}

Works.  Alternatevely:

           # dict mapping parameter name to ArgSection

> +        self.args = OrderedDict()
> +        # a list of Section
> +        self.meta = []
> +        # the current section
> +        self.section = self.body
> +        # associated expression and info (set by expression parser)

"will be set by expression parser", or "to be set by expression parser"?

> +        self.expr = None
> +        self.info = None

Clearer now.  The one remark I still have is on the use of "meta" isn't
obvious here.  I think a comment further up explaining the different
kinds of sections would help.

To be honest, I don't get what's "meta" about the "meta" sections :)

> +
> +    def has_meta(self, name):
> +        """Returns True if the doc has a meta section 'name'"""

PEP257[1] demands imperative mood, and punctuation:

           """Return True if the doc has a meta section 'name'."""

Unfortunately, there seems to be no established convention for marking
parameter names.  You put this one in single quotes, which works here,
but could be confusing in other cases.  I'd sidestep it:

           """Return True if we have a meta section with this name."""

or

           """Does a meta section with this name exist?"""

> +        for i in self.meta:
> +            if i.name == name:
> +                return True
> +        return False
> +
> +    def append(self, line):
> +        """Adds a # comment line, to be parsed and added to current 
> section"""

Imperative mood:

    """Add a comment line, to be parsed and added to the current section."""

However, we're not always adding to the current section, we can also
start a new one.  The following avoids suggesting anything about the
current section:

    """Parse a comment line and add it to the documentation."""

When the function name is a verb, and its doc string starts with a
different verb, the name might be suboptimal.  Use your judgement.

If this one-liner is too terse, we can try a multi-line doc string:
    
    """Parse a comment line and add it to the documentation.

    TODO should we tell more about how we parse?

    Args:
        line: the comment starting with '#', with the newline stripped.

    Raises:
        QAPISchemaError: TODO explain error conditions
    """

Format stolen from the Google Python Style Guide[2], because PEP257 is
of not much help there.

Once you start with such elaborate doc strings, pressure will likely
mount to do the same elsewhere.  Could be a distraction right now.  Use
your judgement.

> +        line = line[1:]
> +        if not line:
> +            self._append_freeform(line)
> +            return
> +
> +        if line[0] != ' ':
> +            raise QAPISchemaError(self.parser, "missing space after #")
> +        line = line[1:]

QAPISchemaError takes the error position from its QAPISchemaParser
argument.  In other words, the error position depends on the state of
the parser when it calls this function.  Turns out the position is the
beginning of the comment.  Servicable here.  Action at a distance,
though.

Less strict:

           # strip leading # and space
           line = line[1:]
           if line[0] == ' ':
               line = line[1:]

Also avoids special-casing empty comments.

Perhaps we should treat all leading whitespace the same (I'm not sure):

           # strip leading # and whitespace
           line = line[1:]
           line = line.lstrip()

> +
> +        if self.symbol:
> +            self._append_symbol_line(line)
> +        elif (not self.body.content and
> +              line.startswith("@") and line.endswith(":")):
> +            self.symbol = line[1:-1]
> +            if not self.symbol:
> +                raise QAPISchemaError(self.parser, "Invalid symbol")

Doesn't recognize the symbol when there's anything other than a single
space between # and @.  Pathological case: 'address@hidden:' starting in column
6 looks exactly like '# @foo:', but doesn't work.  Fortunately, your
code rejects the tab there.  Assigning meaning to (leading) whitespace
may make sense, but it must be documented clearly.

Doesn't recognize the symbol when there's crap after ":".  Confusing
when it's invisible crap, i.e. trailing whitespace.  In general, it's
best to parse left to right, like this:

           elif not self.body.content and line.startswith('@'):
               match = valid_name.match(line[1:]);
               if not match:
                   raise QAPISchemaError(self.parser, 'Invalid name')
               if line.startswith(':', match.end(0)):
                   raise QAPISchemaError(self.parser, 'Expected ":")

Here, the error position is clearly suboptimal: it's the beginning of
the comment instead of where the actual error is, namely the beginning /
the end of the name.  More of the same below.  The root cause is
stacking parsers.  I'm not asking you to do anything about it at this
time.

Note my use of "name" rather than "symbol" in the error message.
qapi-code-gen.txt talks about names, not symbols.  Consistent use of
terminology matters.

> +        else:
> +            self._append_freeform(line)
> +
> +    def _append_symbol_line(self, line):
> +        name = line.split(' ', 1)[0]
> +
> +        if name.startswith("@") and name.endswith(":"):
> +            line = line[len(name)+1:]
> +            self._start_args_section(name[1:-1])

Similar issue.  Left to right:

           if re.match(r'\s*@', line):
               match = valid_name.match(line[1:]);
               if not match:
                   raise QAPISchemaError(self.parser, 'Invalid parameter name')
               line = line[match.end(0):]
               if line.startswith(':'):
                   raise QAPISchemaError(self.parser, 'Expected ":")
               line = line[1:]
               self._start_args_section(match.group(0))

Even better would be factoring out the common code to parse '@foo:'.

> +        elif name in ("Returns:", "Since:",
> +                      # those are often singular or plural
> +                      "Note:", "Notes:",
> +                      "Example:", "Examples:"):
> +            line = line[len(name)+1:]
> +            self._start_meta_section(name[:-1])

Since we're re.match()ing already, here's how do to it that way:

           else
               match = re.match(r'\s*(Returns|Since|Notes?|Examples?):', line)
               if match:
                   line = line[match.end(0):]
                   self._start_meta_section(match.group(1))

> +
> +        self._append_freeform(line)
> +
> +    def _start_args_section(self, name):
> +        if not name:
> +            raise QAPISchemaError(self.parser, "Invalid argument name")

parameter name

> +        if name in self.args:
> +            raise QAPISchemaError(self.parser, "'%s' arg duplicated" % name)

Duplicate parameter name

> +        self.section = QAPIDoc.ArgSection(name)
> +        self.args[name] = self.section
> +
> +    def _start_meta_section(self, name):
> +        if name in ("Returns", "Since") and self.has_meta(name):
> +            raise QAPISchemaError(self.parser,
> +                                  "Duplicated '%s' section" % name)
> +        self.section = QAPIDoc.Section(name)
> +        self.meta.append(self.section)
> +
> +    def _append_freeform(self, line):
> +        in_arg = isinstance(self.section, QAPIDoc.ArgSection)
> +        if in_arg and self.section.content and not self.section.content[-1] \
> +           and line and not line[0].isspace():

PEP8[3] prefers parenthesises over backslash:

           if (in_arg and self.section.content and not self.section.content[-1]
               and line and not line[0].isspace()):

> +            # an empty line followed by a non-indented
> +            # comment ends the argument section
> +            self.section = self.body
> +            self._append_freeform(line)
> +            return

Switching back to the self.body section like this reorders the
documentation text.  I still think this is a terrible idea.  A dumb
script is exceedingly unlikely to improve human-written doc comment text
by reordering.  In the rare case it does, the doc comment source should
be reordered.

Here's an example where the doc generator happily creates unintelligible
garbage if I format CpuDefinitionInfo's doc comment in a slightly off
way:

    ##
    # @CpuDefinitionInfo:
    #
    # Virtual CPU definition.
    #
    # @name: the name of the CPU definition
    # 
    # @migration-safe: #optional whether a CPU definition can be safely
    # used for migration in combination with a QEMU compatibility
    # machine when migrating between different QMU versions and between
    # hosts with different sets of (hardware or software)
    # capabilities.
    # 
    # If not provided, information is not available and callers should
    # not assume the CPU definition to be migration-safe. (since 2.8)
    #
    # @static: whether a CPU definition is static and will not change
    # depending on QEMU version, machine type, machine options and
    # accelerator options.  A static model is always
    # migration-safe. (since 2.8)
    #
    # @unavailable-features: #optional List of properties that prevent
    # the CPU model from running in the current host. (since 2.8)
    #
    # @unavailable-features is a list of QOM property names that
    # represent CPU model attributes that prevent the CPU from running.
    # If the QOM property is read-only, that means there's no known
    # way to make the CPU model run in the current host. Implementations
    # that choose not to provide specific information return the
    # property name "type".
    #
    # If the property is read-write, it means that it MAY be possible
    # to run the CPU model in the current host if that property is
    # changed. Management software can use it as hints to suggest or
    # choose an alternative for the user, or just to generate meaningful
    # error messages explaining why the CPU model can't be used.
    # If @unavailable-features is an empty list, the CPU model is
    # runnable using the current host and machine-type.
    # If @unavailable-features is not present, runnability
    # information for the CPU is not available.
    #
    # Since: 1.2.0
    ##
    { 'struct': 'CpuDefinitionInfo',
      'data': { 'name': 'str', '*migration-safe': 'bool', 'static': 'bool',
                '*unavailable-features': [ 'str' ] } }

To detect the problem, you have to read the generated docs attentively.
You know my opinion on our chances for that to happen during
development.

My point is not that we should support this doc comment format.  My
point is that people could conceivably write something like it, and not
get caught in patch review.

I can see three ways out of this swamp:

1. Let sections continue until another one begins.

2. Make text between sections an error.

3. When a section ends, start a new anonymous section.

Can't say offhand which one will work best.

> +        if in_arg or not self.section.name.startswith("Example"):
> +            line = line.strip()

Stripping whitespace is not "Markdown-like", because intendation carries
meaning in Markdown.  Example:

* First item in itemized list
* Second item
    * Sub-item of second item
    * Another sub-item
* Third item

Stripping whitespace destroys the list structure.  If that's what you
want, you get to document where your "Markdown-like" markup is unlike
Markdown :)

Is there a technical reason for stripping whitespace?

See also discussion of space after # above.

> +        self.section.append(line)
> +
> +
>  class QAPISchemaParser(object):
>  
>      def __init__(self, fp, previously_included=[], incl_info=None):
> @@ -137,11 +240,18 @@ class QAPISchemaParser(object):
>          self.line = 1
>          self.line_pos = 0
>          self.exprs = []
> +        self.docs = []
>          self.accept()
>  
>          while self.tok is not None:
>              info = {'file': fname, 'line': self.line,
>                      'parent': self.incl_info}
> +            if self.tok == '#' and self.val.startswith('##'):

How can self.val *not* start with '##' here?

> +                doc = self.get_doc()
> +                doc.info = info

Let's pass info as argument to get_doc(), so we don't have to dot into
doc here.  get_doc() can avoid dotting into doc by passing info to the
constructor.

> +                self.docs.append(doc)
> +                continue
> +
>              expr = self.get_expr(False)
>              if isinstance(expr, dict) and "include" in expr:
>                  if len(expr) != 1:
> @@ -160,6 +270,7 @@ class QAPISchemaParser(object):
>                          raise QAPILineError(info, "Inclusion loop for %s"
>                                              % include)
>                      inf = inf['parent']
> +
>                  # skip multiple include of the same file
>                  if incl_abs_fname in previously_included:
>                      continue
> @@ -171,12 +282,38 @@ class QAPISchemaParser(object):
>                  exprs_include = QAPISchemaParser(fobj, previously_included,
>                                                   info)
>                  self.exprs.extend(exprs_include.exprs)
> +                self.docs.extend(exprs_include.docs)
>              else:
>                  expr_elem = {'expr': expr,
>                               'info': info}
> +                if self.docs and not self.docs[-1].expr:
> +                    self.docs[-1].expr = expr
> +                    expr_elem['doc'] = self.docs[-1]
> +

Attaches the expression to the last doc comment that doesn't already
have one.  A bit sloppy, because there could be non-doc comments in
between, or the doc comment could be in another file.  It'll do, at
least for now.

>                  self.exprs.append(expr_elem)
>  
> -    def accept(self):
> +    def get_doc(self):
> +        if self.val != '##':
> +            raise QAPISchemaError(self, "Junk after '##' at start of "
> +                                  "documentation comment")
> +
> +        doc = QAPIDoc(self)
> +        self.accept(False)
> +        while self.tok == '#':
> +            if self.val.startswith('##'):
> +                # End of doc comment
> +                if self.val != '##':
> +                    raise QAPISchemaError(self, "Junk after '##' at end of "
> +                                          "documentation comment")
> +                self.accept()
> +                return doc
> +            else:
> +                doc.append(self.val)
> +            self.accept(False)
> +
> +        raise QAPISchemaError(self, "Documentation comment must end with 
> '##'")

Let's put this after accept, next to the other get_FOO().

> +
> +    def accept(self, skip_comment=True):
>          while True:
>              self.tok = self.src[self.cursor]
>              self.pos = self.cursor
> @@ -184,7 +321,13 @@ class QAPISchemaParser(object):
>              self.val = None
>  
>              if self.tok == '#':
> +                if self.src[self.cursor] == '#':
> +                    # Start of doc comment
> +                    skip_comment = False
>                  self.cursor = self.src.find('\n', self.cursor)
> +                if not skip_comment:
> +                    self.val = self.src[self.pos:self.cursor]
> +                    return
>              elif self.tok in "{}:,[]":
>                  return
>              elif self.tok == "'":

Copied from review of v3, so I don't forget:

Comment tokens are thrown away as before, except when the parser asks
for them by passing skip_comment=False, or when the comment token starts
with ##.  The parser asks while parsing a doc comment, in get_doc().

This is a backchannel from the parser to the lexer.  I'd rather avoid
such lexer hacks, but I guess we can address that on top.

A comment starting with ## inside an expression is now a syntax error.
For instance, input

    {
    ##

yields

    /dev/stdin:2:1: Expected string or "}"

Rather unfriendly error message, but we can fix that on top.

> @@ -713,7 +856,7 @@ def check_keys(expr_elem, meta, required, optional=[]):
>                                  % (key, meta, name))
>  
>  
> -def check_exprs(exprs):
> +def check_exprs(exprs, strict_doc):

Note: strict_doc=False unless this is qapi2texi.py.

>      global all_names
>  
>      # Learn the types and check for valid expression keys
> @@ -722,6 +865,11 @@ def check_exprs(exprs):
>      for expr_elem in exprs:
>          expr = expr_elem['expr']
>          info = expr_elem['info']
> +
> +        if strict_doc and 'doc' not in expr_elem:
> +            raise QAPILineError(info,
> +                                "Expression missing documentation comment")
> +

Why should we supress this error in programs other than qapi2texi.py?

Can't see a test for this one.

>          if 'enum' in expr:
>              check_keys(expr_elem, 'enum', ['data'], ['prefix'])
>              add_enum(expr['enum'], info, expr['data'])
> @@ -780,6 +928,63 @@ def check_exprs(exprs):
>      return exprs
>  
>  
> +def check_simple_doc(doc):

You call this "free-form" elsewhere.  Pick one name and stick to it.
I think free-form is more descriptive than simple.

> +    if doc.symbol:
> +        raise QAPILineError(doc.info,
> +                            "'%s' documention is not followed by the 
> definition"
> +                            % doc.symbol)

"Documentation for %s is not ..."

> +
> +    body = str(doc.body)
> +    if re.search(r'@\S+:', body, re.MULTILINE):
> +        raise QAPILineError(doc.info,
> +                            "Document body cannot contain @NAME: sections")
> +
> +
> +def check_expr_doc(doc, expr, info):

You call this "symbol" elsewhere.  I think "definition" would be better
than either.

> +    for i in ('enum', 'union', 'alternate', 'struct', 'command', 'event'):
> +        if i in expr:
> +            meta = i
> +            break
> +
> +    name = expr[meta]
> +    if doc.symbol != name:
> +        raise QAPILineError(info, "Definition of '%s' follows documentation"
> +                            " for '%s'" % (name, doc.symbol))
> +    if doc.has_meta('Returns') and 'command' not in expr:
> +        raise QAPILineError(info, "Invalid return documentation")

Suggest something like "'Returns:' is only valid for commands".

We accept 'Returns:' even when the command doesn't return anything,
because we currently use it to document errors, too.  Can't say I like
that, but it's out of scope here.

> +
> +    doc_args = set(doc.args.keys())
> +    if meta == 'union':
> +        data = expr.get('base', [])
> +    else:
> +        data = expr.get('data', [])
> +    if isinstance(data, dict):
> +        data = data.keys()
> +    args = set([name.strip('*') for name in data])

Not fixed since v3:

* Flat union where 'base' is a string, e.g. union UserDefFlatUnion in
  qapi-schema-test.json has base 'UserDefUnionBase', args is set(['a',
  'B', 'e', 'D', 'f', 'i', 'o', 'n', 's', 'r', 'U'])

* Command where 'data' is a string, e.g. user_def_cmd0 in
  qapi-schema-test.json has data 'Empty2', args is set(['E', 'm', 'p',
  '2', 't', 'y'])

* Event where 'data' is a string, no test case handy (hole in test
  coverage)

> +    if meta == 'alternate' or \
> +       (meta == 'union' and not expr.get('discriminator')):
> +        args.add('type')

As explained in review of v3, this is only a subset of the real set of
members.  Computing the exact set is impractical when working with the
abstract syntax tree.  I believe we'll eventually have to rewrite this
code to work with the QAPISchemaEntity instead.

> +    if not doc_args.issubset(args):
> +        raise QAPILineError(info, "Members documentation is not a subset of"
> +                            " API %r > %r" % (list(doc_args), list(args)))
> +
> +
> +def check_docs(docs):
> +    for doc in docs:
> +        for section in doc.args.values() + doc.meta:
> +            content = str(section)
> +            if not content or content.isspace():
> +                raise QAPILineError(doc.info,
> +                                    "Empty doc section '%s'" % section.name)
> +
> +        if not doc.expr:
> +            check_simple_doc(doc)
> +        else:
> +            check_expr_doc(doc, doc.expr, doc.info)
> +
> +    return docs
> +
> +
>  #
>  # Schema compiler frontend
>  #
> @@ -1249,9 +1454,11 @@ class QAPISchemaEvent(QAPISchemaEntity):
>  
>  
>  class QAPISchema(object):
> -    def __init__(self, fname):
> +    def __init__(self, fname, strict_doc=False):
>          try:
> -            self.exprs = check_exprs(QAPISchemaParser(open(fname, 
> "r")).exprs)
> +            parser = QAPISchemaParser(open(fname, "r"))
> +            self.exprs = check_exprs(parser.exprs, strict_doc)
> +            self.docs = check_docs(parser.docs)
>              self._entity_dict = {}
>              self._predefining = True
>              self._def_predefineds()
> diff --git a/scripts/qapi2texi.py b/scripts/qapi2texi.py
> new file mode 100755
> index 0000000..0cec43a
> --- /dev/null
> +++ b/scripts/qapi2texi.py

Still only skimming this one.

> @@ -0,0 +1,331 @@
> +#!/usr/bin/env python
> +# QAPI texi generator
> +#
> +# This work is licensed under the terms of the GNU LGPL, version 2+.
> +# See the COPYING file in the top-level directory.
> +"""This script produces the documentation of a qapi schema in texinfo 
> format"""
> +import re
> +import sys
> +
> +import qapi
> +
> +COMMAND_FMT = """
> address@hidden {type} {{{ret}}} {name} @
> +{{{args}}}
> +
> +{body}
> +
> address@hidden deftypefn
> +
> +""".format
> +
> +ENUM_FMT = """
> address@hidden Enum {name}
> +
> +{body}
> +
> address@hidden deftp
> +
> +""".format
> +
> +STRUCT_FMT = """
> address@hidden {{{type}}} {name} @
> +{{{attrs}}}
> +
> +{body}
> +
> address@hidden deftp
> +
> +""".format
> +
> +EXAMPLE_FMT = """@example
> +{code}
> address@hidden example
> +""".format
> +
> +
> +def subst_strong(doc):
> +    """Replaces *foo* by @strong{foo}"""
> +    return re.sub(r'\*([^_\n]+)\*', r'@emph{\1}', doc)
> +
> +
> +def subst_emph(doc):
> +    """Replaces _foo_ by @emph{foo}"""
> +    return re.sub(r'\s_([^_\n]+)_\s', r' @emph{\1} ', doc)
> +
> +
> +def subst_vars(doc):
> +    """Replaces @var by @code{var}"""
> +    return re.sub(r'@([\w-]+)', r'@code{\1}', doc)
> +
> +
> +def subst_braces(doc):
> +    """Replaces {} with @{ @}"""
> +    return doc.replace("{", "@{").replace("}", "@}")
> +
> +
> +def texi_example(doc):
> +    """Format @example"""
> +    doc = subst_braces(doc).strip('\n')
> +    return EXAMPLE_FMT(code=doc)
> +
> +
> +def texi_comment(doc):
> +    """
> +    Format a comment
> +
> +    Lines starting with:
> +    - |: generates an @example
> +    - =: generates @section
> +    - ==: generates @subsection
> +    - 1. or 1): generates an @enumerate @item
> +    - o/*/-: generates an @itemize list
> +    """
> +    lines = []
> +    doc = subst_braces(doc)
> +    doc = subst_vars(doc)
> +    doc = subst_emph(doc)
> +    doc = subst_strong(doc)
> +    inlist = ""
> +    lastempty = False
> +    for line in doc.split('\n'):
> +        empty = line == ""
> +
> +        if line.startswith("| "):
> +            line = EXAMPLE_FMT(code=line[2:])
> +        elif line.startswith("= "):
> +            line = "@section " + line[2:]
> +        elif line.startswith("== "):
> +            line = "@subsection " + line[3:]
> +        elif re.match("^([0-9]*[.)]) ", line):
> +            if not inlist:
> +                lines.append("@enumerate")
> +                inlist = "enumerate"
> +            line = line[line.find(" ")+1:]
> +            lines.append("@item")
> +        elif re.match("^[o*-] ", line):
> +            if not inlist:
> +                lines.append("@itemize %s" % {'o': "@bullet",
> +                                              '*': "@minus",
> +                                              '-': ""}[line[0]])
> +                inlist = "itemize"
> +            lines.append("@item")
> +            line = line[2:]
> +        elif lastempty and inlist:
> +            lines.append("@end %s\n" % inlist)
> +            inlist = ""
> +
> +        lastempty = empty
> +        lines.append(line)
> +
> +    if inlist:
> +        lines.append("@end %s\n" % inlist)
> +    return "\n".join(lines)
> +
> +
> +def texi_args(expr, key="data"):
> +    """
> +    Format the functions/structure/events.. arguments/members
> +    """
> +    if key not in expr:
> +        return ""
> +
> +    args = expr[key]
> +    arg_list = []
> +    if isinstance(args, str):
> +        arg_list.append(args)
> +    else:
> +        for name, typ in args.iteritems():
> +            # optional arg
> +            if name.startswith("*"):
> +                name = name[1:]
> +                arg_list.append("['%s': @var{%s}]" % (name, typ))
> +            # regular arg
> +            else:
> +                arg_list.append("'%s': @var{%s}" % (name, typ))

Inappropriate use of @var.  @var is for metasyntactic variables,
i.e. something that stands for another piece of text.  typ isn't, it's
the name of a specific QAPI type.  I think you should use @code.

This is the reason why the type names in qemu-qmp-ref.txt are often
mangled, e.g.

 -- Struct: VersionInfo { 'qemu': VERSIONTRIPLE, 'package': STR }

> +
> +    return ", ".join(arg_list)
> +
> +
> +def texi_body(doc):
> +    """
> +    Format the body of a symbol documentation:
> +    - a table of arguments
> +    - followed by "Returns/Notes/Since/Example" sections
> +    """
> +    def _section_order(section):
> +        return {"Returns": 0,
> +                "Note": 1,
> +                "Notes": 1,
> +                "Since": 2,
> +                "Example": 3,
> +                "Examples": 3}[section]
> +
> +    body = "@table @asis\n"
> +    for arg, section in doc.args.iteritems():
> +        desc = str(section)
> +        opt = ''
> +        if desc.startswith("#optional"):
> +            desc = desc[10:]
> +            opt = ' *'
> +        elif desc.endswith("#optional"):
> +            desc = desc[:-10]
> +            opt = ' *'
> +        body += "@item @code{'%s'}%s\n%s\n" % (arg, opt, texi_comment(desc))
> +    body += "@end table\n"
> +    body += texi_comment(str(doc.body))
> +
> +    meta = sorted(doc.meta, key=lambda i: _section_order(i.name))
> +    for section in meta:
> +        name, doc = (section.name, str(section))
> +        func = texi_comment
> +        if name.startswith("Example"):
> +            func = texi_example
> +
> +        body += "address@hidden address@hidden quotation" % \
> +                (name, func(doc))
> +    return body
> +
> +
> +def texi_alternate(expr, doc):
> +    """
> +    Format an alternate to texi
> +    """
> +    args = texi_args(expr)
> +    body = texi_body(doc)
> +    return STRUCT_FMT(type="Alternate",
> +                      name=doc.symbol,
> +                      attrs="[ " + args + " ]",
> +                      body=body)
> +
> +
> +def texi_union(expr, doc):
> +    """
> +    Format an union to texi

I think it's "a union".

> +    """
> +    attrs = "@{ " + texi_args(expr, "base") + " @}"
> +    discriminator = expr.get("discriminator")
> +    if not discriminator:
> +        union = "Flat Union"
> +        discriminator = "type"
> +    else:
> +        union = "Union"

Condition is backwards.

> +    attrs += " + '%s' = [ " % discriminator
> +    attrs += texi_args(expr, "data") + " ]"
> +    body = texi_body(doc)
> +
> +    return STRUCT_FMT(type=union,
> +                      name=doc.symbol,
> +                      attrs=attrs,
> +                      body=body)

You're inventing syntax here.  Example output:

 -- Union: QCryptoBlockOpenOptions { QCryptoBlockOptionsBase } +
          'format' = [ 'qcow': QCRYPTOBLOCKOPTIONSQCOW, 'luks':
          QCRYPTOBLOCKOPTIONSLUKS ]

     The options that are available for all encryption formats when
     opening an existing volume
          Since: 2.6

 -- Flat Union: ImageInfoSpecific { } + 'type' = [ 'qcow2':
          IMAGEINFOSPECIFICQCOW2, 'vmdk': IMAGEINFOSPECIFICVMDK, 'luks':
          QCRYPTOBLOCKINFOLUKS ]

     A discriminated record of image format specific information
     structures.
          Since: 1.7

Note that QCryptoBlockOpenOptions is actually a flat union, and
ImageInfoSpecific a simple union.  As I said, the condition is
backwards.

The meaning of the attrs part is unobvious.  Familiarity with schema
syntax doesn't really help.

Could we simply use schema syntax here?

If not: whatever format you use, you first need to explain it.

> +
> +
> +def texi_enum(expr, doc):
> +    """
> +    Format an enum to texi
> +    """
> +    for i in expr['data']:
> +        if i not in doc.args:
> +            doc.args[i] = ''
> +    body = texi_body(doc)
> +    return ENUM_FMT(name=doc.symbol,
> +                    body=body)
> +
> +
> +def texi_struct(expr, doc):
> +    """
> +    Format a struct to texi
> +    """
> +    args = texi_args(expr)
> +    body = texi_body(doc)
> +    attrs = "@{ " + args + " @}"
> +    base = expr.get("base")
> +    if base:
> +        attrs += " + %s" % base
> +    return STRUCT_FMT(type="Struct",
> +                      name=doc.symbol,
> +                      attrs=attrs,
> +                      body=body)

More syntax invention.  Example output:

 -- Struct: BlockdevOptionsReplication { 'mode': REPLICATIONMODE,
          ['top-id': STR] } + BlockdevOptionsGenericFormat

     ''mode''
          the replication mode
     ''top-id'' *
          In secondary mode, node name or device ID of the root node who
          owns the replication node chain.  Must not be given in primary
          mode.
     Driver specific block device options for replication
          Since: 2.8

Meaning of the attrs part is perhaps more guessable here, but it's still
guesswork.

The meaning of * after ''top-id'' is also unobvious.

Note the redundancy between the attrs part and the body: both state
member names and optionalness.  The body doesn't state member types and
base type.  If we fixed that, we could drop the attrs part and save us
the trouble of defining and explaining a syntax for it.

Let me take a step back.  This document is about the QMP wire format.
There are no such things as simple and flat unions on the wire, only
JSON objects.  QMP introspection duly describes a type's JSON objects,
not how it's defined in QAPI.  I think QMP documentation should ideally
do the same.

QMP introspection uses a common structure for struct, simple and flat
union: common members, variants, and if variants, then the common member
that serves as tag.  See introspect.json for details.

Base types are flattened away.  Appropriate for introspection, but
documentation shouldn't do that.

I wrote "ideally" because it's probably too big a step.  I'm willing to
settle for something less than ideal.

> +
> +
> +def texi_command(expr, doc):
> +    """
> +    Format a command to texi
> +    """
> +    args = texi_args(expr)
> +    ret = expr["returns"] if "returns" in expr else ""
> +    body = texi_body(doc)
> +    return COMMAND_FMT(type="Command",
> +                       name=doc.symbol,
> +                       ret=ret,
> +                       args="(" + args + ")",
> +                       body=body)
> +
> +
> +def texi_event(expr, doc):
> +    """
> +    Format an event to texi
> +    """
> +    args = texi_args(expr)
> +    body = texi_body(doc)
> +    return COMMAND_FMT(type="Event",
> +                       name=doc.symbol,
> +                       ret="",
> +                       args="(" + args + ")",
> +                       body=body)
> +
> +
> +def texi_expr(expr, doc):
> +    """
> +    Format an expr to texi
> +    """
> +    (kind, _) = expr.items()[0]
> +
> +    fmt = {"command": texi_command,
> +           "struct": texi_struct,
> +           "enum": texi_enum,
> +           "union": texi_union,
> +           "alternate": texi_alternate,
> +           "event": texi_event}
> +    try:
> +        fmt = fmt[kind]
> +    except KeyError:
> +        raise ValueError("Unknown expression kind '%s'" % kind)

The try / except converts one kind of error into another.  What does
that buy us?  As far as I can tell, this shouldn't ever happen.

> +
> +    return fmt(expr, doc)
> +
> +
> +def texi(docs):
> +    """
> +    Convert QAPI schema expressions to texi documentation
> +    """
> +    res = []
> +    for doc in docs:
> +        expr = doc.expr
> +        if not expr:
> +            res.append(texi_body(doc))
> +            continue
> +        try:
> +            doc = texi_expr(expr, doc)
> +            res.append(doc)
> +        except:
> +            print >>sys.stderr, "error at @%s" % doc.info
> +            raise
> +
> +    return '\n'.join(res)
> +
> +
> +def main(argv):
> +    """
> +    Takes schema argument, prints result to stdout
> +    """
> +    if len(argv) != 2:
> +        print >>sys.stderr, "%s: need exactly 1 argument: SCHEMA" % argv[0]
> +        sys.exit(1)
> +
> +    schema = qapi.QAPISchema(argv[1], strict_doc=True)
> +    print texi(schema.docs)
> +
> +
> +if __name__ == "__main__":
> +    main(sys.argv)
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index 2841c51..8bc963e 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -45,16 +45,13 @@ QAPI parser does not).  At present, there is no place 
> where a QAPI
>  schema requires the use of JSON numbers or null.
>  
>  Comments are allowed; anything between an unquoted # and the following
> -newline is ignored.  Although there is not yet a documentation
> -generator, a form of stylized comments has developed for consistently
> -documenting details about an expression and when it was added to the
> -schema.  The documentation is delimited between two lines of ##, then
> -the first line names the expression, an optional overview is provided,
> -then individual documentation about each member of 'data' is provided,
> -and finally, a 'Since: x.y.z' tag lists the release that introduced
> -the expression.  Optional members are tagged with the phrase
> -'#optional', often with their default value; and extensions added
> -after the expression was first released are also given a '(since
> +newline is ignored.  The documentation is delimited between two lines
> +of ##, then the first line names the expression, an optional overview
> +is provided, then individual documentation about each member of 'data'
> +is provided, and finally, a 'Since: x.y.z' tag lists the release that
> +introduced the expression.  Optional members are tagged with the
> +phrase '#optional', often with their default value; and extensions
> +added after the expression was first released are also given a '(since
>  x.y.z)' comment.  For example:
>  
>      ##
> @@ -73,12 +70,49 @@ x.y.z)' comment.  For example:
>      #           (Since 2.0)
>      #
>      # Since: 0.14.0
> +    #
> +    # Notes: You can also make a list:
> +    #        - with items
> +    #        - like this
> +    #
> +    # Example:
> +    #
> +    # -> { "execute": ... }
> +    # <- { "return": ... }
> +    #
>      ##
>      { 'struct': 'BlockStats',
>        'data': {'*device': 'str', 'stats': 'BlockDeviceStats',
>                 '*parent': 'BlockStats',
>                 '*backing': 'BlockStats'} }

This example gives an idea of how to document struct types.  But the
reader is left guessing how to document other kinds of definitions.  In
particular, there's no mention of "Returns", "Note" and "Examples"
sections anywhere in this file.  As is, this could do for a tutorial,
but this file is a *reference*, not a tutorial.

For a reference, we need to be more thorough.  A doc comments section on
its own seems advisable.

I guess doc comment examples are best added to the schema code examples
in sections === Struct types ===, ..., === Events ===.

Careful review for completeness is advised.

> +It's also possible to create documentation sections, such as:
> +
> +    ##
> +    # = Section
> +    # == Subsection
> +    #
> +    # Some text foo with *strong* and _emphasis_
> +    # 1. with a list
> +    # 2. like that
> +    #
> +    # And some code:
> +    # | $ echo foo
> +    # | -> do this
> +    # | <- get that
> +    #
> +    ##
> +
> +Text *foo* and _foo_ are for "strong" and "emphasis" styles (they do
> +not work over multiple lines). @foo is used to reference a symbol.
> +
> +Lines starting with the following characters and a space:
> +- | are examples
> +- = are top section
> +- == are subsection
> +- X. or X) are enumerations (X is any number)
> +- o/*/- are itemized list
> +

Is this doc markup documentation complete?

Is it related to any existing text markup language?  Hmm, the commit
message calls it "Markdown-like".  Should we mention that here?

Is all markup valid in all contexts?  For instance, is = Section valid
in symbol blocks?  What does | (or any markup for that matter) do within
an Example section?

>  The schema sets up a series of types, as well as commands and events
>  that will use those types.  Forward references are allowed: the parser
>  scans in two passes, where the first pass learns all type names, and
> diff --git a/tests/qapi-schema/doc-bad-args.err 
> b/tests/qapi-schema/doc-bad-args.err
> new file mode 100644
> index 0000000..a55e003
> --- /dev/null
> +++ b/tests/qapi-schema/doc-bad-args.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/doc-bad-args.json:1: Members documentation is not a subset 
> of API ['a', 'b'] > ['a']
> diff --git a/tests/qapi-schema/doc-bad-args.exit 
> b/tests/qapi-schema/doc-bad-args.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/doc-bad-args.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/doc-bad-args.json 
> b/tests/qapi-schema/doc-bad-args.json
> new file mode 100644
> index 0000000..26992ea
> --- /dev/null
> +++ b/tests/qapi-schema/doc-bad-args.json
> @@ -0,0 +1,6 @@
> +##
> +# @foo:
> +# @a: a
> +# @b: b
> +##
> +{ 'command': 'foo', 'data': {'a': 'int'} }

The existing tests/qapi-schema/*.json explain what is being tested in a
comment.  Perhaps like this:

    # Arguments listed in the doc comment must exist in the actual schema

    ##
    # @foo:
    ...

Likewise for other tests.

> diff --git a/tests/qapi-schema/doc-bad-args.out 
> b/tests/qapi-schema/doc-bad-args.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/doc-bad-symbol.err 
> b/tests/qapi-schema/doc-bad-symbol.err
> new file mode 100644
> index 0000000..9c969d1
> --- /dev/null
> +++ b/tests/qapi-schema/doc-bad-symbol.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/doc-bad-symbol.json:1: Definition of 'foo' follows 
> documentation for 'food'
> diff --git a/tests/qapi-schema/doc-bad-symbol.exit 
> b/tests/qapi-schema/doc-bad-symbol.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/doc-bad-symbol.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/doc-bad-symbol.json 
> b/tests/qapi-schema/doc-bad-symbol.json
> new file mode 100644
> index 0000000..7255152
> --- /dev/null
> +++ b/tests/qapi-schema/doc-bad-symbol.json
> @@ -0,0 +1,4 @@
> +##
> +# @food:
> +##
> +{ 'command': 'foo', 'data': {'a': 'int'} }
> diff --git a/tests/qapi-schema/doc-bad-symbol.out 
> b/tests/qapi-schema/doc-bad-symbol.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/doc-duplicated-arg.err 
> b/tests/qapi-schema/doc-duplicated-arg.err
> new file mode 100644
> index 0000000..88a272b
> --- /dev/null
> +++ b/tests/qapi-schema/doc-duplicated-arg.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/doc-duplicated-arg.json:4:1: 'a' arg duplicated
> diff --git a/tests/qapi-schema/doc-duplicated-arg.exit 
> b/tests/qapi-schema/doc-duplicated-arg.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/doc-duplicated-arg.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/doc-duplicated-arg.json 
> b/tests/qapi-schema/doc-duplicated-arg.json
> new file mode 100644
> index 0000000..f7804c2
> --- /dev/null
> +++ b/tests/qapi-schema/doc-duplicated-arg.json
> @@ -0,0 +1,5 @@
> +##
> +# @foo:
> +# @a:
> +# @a:
> +##
> diff --git a/tests/qapi-schema/doc-duplicated-arg.out 
> b/tests/qapi-schema/doc-duplicated-arg.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/doc-duplicated-return.err 
> b/tests/qapi-schema/doc-duplicated-return.err
> new file mode 100644
> index 0000000..1b02880
> --- /dev/null
> +++ b/tests/qapi-schema/doc-duplicated-return.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/doc-duplicated-return.json:5:1: Duplicated 'Returns' 
> section
> diff --git a/tests/qapi-schema/doc-duplicated-return.exit 
> b/tests/qapi-schema/doc-duplicated-return.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/doc-duplicated-return.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/doc-duplicated-return.json 
> b/tests/qapi-schema/doc-duplicated-return.json
> new file mode 100644
> index 0000000..de7234b
> --- /dev/null
> +++ b/tests/qapi-schema/doc-duplicated-return.json
> @@ -0,0 +1,6 @@
> +##
> +# @foo:
> +#
> +# Returns: 0
> +# Returns: 1
> +##
> diff --git a/tests/qapi-schema/doc-duplicated-return.out 
> b/tests/qapi-schema/doc-duplicated-return.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/doc-duplicated-since.err 
> b/tests/qapi-schema/doc-duplicated-since.err
> new file mode 100644
> index 0000000..72efb2a
> --- /dev/null
> +++ b/tests/qapi-schema/doc-duplicated-since.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/doc-duplicated-since.json:5:1: Duplicated 'Since' section
> diff --git a/tests/qapi-schema/doc-duplicated-since.exit 
> b/tests/qapi-schema/doc-duplicated-since.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/doc-duplicated-since.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/doc-duplicated-since.json 
> b/tests/qapi-schema/doc-duplicated-since.json
> new file mode 100644
> index 0000000..da261a1
> --- /dev/null
> +++ b/tests/qapi-schema/doc-duplicated-since.json
> @@ -0,0 +1,6 @@
> +##
> +# @foo:
> +#
> +# Since: 0
> +# Since: 1
> +##
> diff --git a/tests/qapi-schema/doc-duplicated-since.out 
> b/tests/qapi-schema/doc-duplicated-since.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/doc-empty-arg.err 
> b/tests/qapi-schema/doc-empty-arg.err
> new file mode 100644
> index 0000000..0647eed
> --- /dev/null
> +++ b/tests/qapi-schema/doc-empty-arg.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/doc-empty-arg.json:3:1: Invalid argument name
> diff --git a/tests/qapi-schema/doc-empty-arg.exit 
> b/tests/qapi-schema/doc-empty-arg.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/doc-empty-arg.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/doc-empty-arg.json 
> b/tests/qapi-schema/doc-empty-arg.json
> new file mode 100644
> index 0000000..74f526c
> --- /dev/null
> +++ b/tests/qapi-schema/doc-empty-arg.json
> @@ -0,0 +1,4 @@
> +##
> +# @foo:
> +# @:
> +##
> diff --git a/tests/qapi-schema/doc-empty-arg.out 
> b/tests/qapi-schema/doc-empty-arg.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/doc-empty-section.err 
> b/tests/qapi-schema/doc-empty-section.err
> new file mode 100644
> index 0000000..c940332
> --- /dev/null
> +++ b/tests/qapi-schema/doc-empty-section.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/doc-empty-section.json:1: Empty doc section 'Note'
> diff --git a/tests/qapi-schema/doc-empty-section.exit 
> b/tests/qapi-schema/doc-empty-section.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/doc-empty-section.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/doc-empty-section.json 
> b/tests/qapi-schema/doc-empty-section.json
> new file mode 100644
> index 0000000..ed3867d
> --- /dev/null
> +++ b/tests/qapi-schema/doc-empty-section.json
> @@ -0,0 +1,6 @@
> +##
> +# @foo:
> +#
> +# Note:
> +##
> +{ 'command': 'foo', 'data': {'a': 'int'} }
> diff --git a/tests/qapi-schema/doc-empty-section.out 
> b/tests/qapi-schema/doc-empty-section.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/doc-empty-symbol.err 
> b/tests/qapi-schema/doc-empty-symbol.err
> new file mode 100644
> index 0000000..955dc3c
> --- /dev/null
> +++ b/tests/qapi-schema/doc-empty-symbol.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/doc-empty-symbol.json:2:1: Invalid symbol
> diff --git a/tests/qapi-schema/doc-empty-symbol.exit 
> b/tests/qapi-schema/doc-empty-symbol.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/doc-empty-symbol.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/doc-empty-symbol.json 
> b/tests/qapi-schema/doc-empty-symbol.json
> new file mode 100644
> index 0000000..8a2d662
> --- /dev/null
> +++ b/tests/qapi-schema/doc-empty-symbol.json
> @@ -0,0 +1,3 @@
> +##
> +# @:
> +##
> diff --git a/tests/qapi-schema/doc-empty-symbol.out 
> b/tests/qapi-schema/doc-empty-symbol.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/doc-invalid-end.err 
> b/tests/qapi-schema/doc-invalid-end.err
> new file mode 100644
> index 0000000..5ed8911
> --- /dev/null
> +++ b/tests/qapi-schema/doc-invalid-end.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/doc-invalid-end.json:3:2: Documentation comment must end 
> with '##'
> diff --git a/tests/qapi-schema/doc-invalid-end.exit 
> b/tests/qapi-schema/doc-invalid-end.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/doc-invalid-end.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/doc-invalid-end.json 
> b/tests/qapi-schema/doc-invalid-end.json
> new file mode 100644
> index 0000000..7452718
> --- /dev/null
> +++ b/tests/qapi-schema/doc-invalid-end.json
> @@ -0,0 +1,3 @@
> +##
> +# An invalid comment
> +#
> diff --git a/tests/qapi-schema/doc-invalid-end.out 
> b/tests/qapi-schema/doc-invalid-end.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/doc-invalid-end2.err 
> b/tests/qapi-schema/doc-invalid-end2.err
> new file mode 100644
> index 0000000..acd23bb
> --- /dev/null
> +++ b/tests/qapi-schema/doc-invalid-end2.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/doc-invalid-end2.json:3:1: Junk after '##' at end of 
> documentation comment
> diff --git a/tests/qapi-schema/doc-invalid-end2.exit 
> b/tests/qapi-schema/doc-invalid-end2.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/doc-invalid-end2.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/doc-invalid-end2.json 
> b/tests/qapi-schema/doc-invalid-end2.json
> new file mode 100644
> index 0000000..12e669e
> --- /dev/null
> +++ b/tests/qapi-schema/doc-invalid-end2.json
> @@ -0,0 +1,3 @@
> +##
> +#
> +## invalid
> diff --git a/tests/qapi-schema/doc-invalid-end2.out 
> b/tests/qapi-schema/doc-invalid-end2.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/doc-invalid-return.err 
> b/tests/qapi-schema/doc-invalid-return.err
> new file mode 100644
> index 0000000..c3f2691
> --- /dev/null
> +++ b/tests/qapi-schema/doc-invalid-return.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/doc-invalid-return.json:1: Invalid return documentation
> diff --git a/tests/qapi-schema/doc-invalid-return.exit 
> b/tests/qapi-schema/doc-invalid-return.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/doc-invalid-return.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/doc-invalid-return.json 
> b/tests/qapi-schema/doc-invalid-return.json
> new file mode 100644
> index 0000000..6568b6d
> --- /dev/null
> +++ b/tests/qapi-schema/doc-invalid-return.json
> @@ -0,0 +1,5 @@
> +##
> +# @foo:
> +# Returns: blah
> +##
> +{ 'event': 'foo' }
> diff --git a/tests/qapi-schema/doc-invalid-return.out 
> b/tests/qapi-schema/doc-invalid-return.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/doc-invalid-section.err 
> b/tests/qapi-schema/doc-invalid-section.err
> new file mode 100644
> index 0000000..f4c12aa
> --- /dev/null
> +++ b/tests/qapi-schema/doc-invalid-section.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/doc-invalid-section.json:1: Document body cannot contain 
> @NAME: sections
> diff --git a/tests/qapi-schema/doc-invalid-section.exit 
> b/tests/qapi-schema/doc-invalid-section.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/doc-invalid-section.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/doc-invalid-section.json 
> b/tests/qapi-schema/doc-invalid-section.json
> new file mode 100644
> index 0000000..9afa2f1
> --- /dev/null
> +++ b/tests/qapi-schema/doc-invalid-section.json
> @@ -0,0 +1,4 @@
> +##
> +# freeform
> +# @note: foo
> +##
> diff --git a/tests/qapi-schema/doc-invalid-section.out 
> b/tests/qapi-schema/doc-invalid-section.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/doc-invalid-start.err 
> b/tests/qapi-schema/doc-invalid-start.err
> new file mode 100644
> index 0000000..194c8d7
> --- /dev/null
> +++ b/tests/qapi-schema/doc-invalid-start.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/doc-invalid-start.json:1:1: Junk after '##' at start of 
> documentation comment
> diff --git a/tests/qapi-schema/doc-invalid-start.exit 
> b/tests/qapi-schema/doc-invalid-start.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/doc-invalid-start.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/doc-invalid-start.json 
> b/tests/qapi-schema/doc-invalid-start.json
> new file mode 100644
> index 0000000..f85adfd
> --- /dev/null
> +++ b/tests/qapi-schema/doc-invalid-start.json
> @@ -0,0 +1,3 @@
> +## invalid
> +#
> +##
> diff --git a/tests/qapi-schema/doc-invalid-start.out 
> b/tests/qapi-schema/doc-invalid-start.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/doc-missing-expr.err 
> b/tests/qapi-schema/doc-missing-expr.err
> new file mode 100644
> index 0000000..e4ed135
> --- /dev/null
> +++ b/tests/qapi-schema/doc-missing-expr.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/doc-missing-expr.json:1: 'bar' documention is not followed 
> by the definition
> diff --git a/tests/qapi-schema/doc-missing-expr.exit 
> b/tests/qapi-schema/doc-missing-expr.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/doc-missing-expr.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/doc-missing-expr.json 
> b/tests/qapi-schema/doc-missing-expr.json
> new file mode 100644
> index 0000000..a0be2e1
> --- /dev/null
> +++ b/tests/qapi-schema/doc-missing-expr.json
> @@ -0,0 +1,3 @@
> +##
> +# @bar:
> +##
> diff --git a/tests/qapi-schema/doc-missing-expr.out 
> b/tests/qapi-schema/doc-missing-expr.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/doc-missing-space.err 
> b/tests/qapi-schema/doc-missing-space.err
> new file mode 100644
> index 0000000..37056ce
> --- /dev/null
> +++ b/tests/qapi-schema/doc-missing-space.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/doc-missing-space.json:3:1: missing space after #
> diff --git a/tests/qapi-schema/doc-missing-space.exit 
> b/tests/qapi-schema/doc-missing-space.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/doc-missing-space.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/doc-missing-space.json 
> b/tests/qapi-schema/doc-missing-space.json
> new file mode 100644
> index 0000000..39303de
> --- /dev/null
> +++ b/tests/qapi-schema/doc-missing-space.json
> @@ -0,0 +1,4 @@
> +##
> +# missing space:
> +#wef
> +##
> diff --git a/tests/qapi-schema/doc-missing-space.out 
> b/tests/qapi-schema/doc-missing-space.out
> new file mode 100644
> index 0000000..e69de29
> diff --git a/tests/qapi-schema/qapi-schema-test.json 
> b/tests/qapi-schema/qapi-schema-test.json
> index 1719463..d921ec4 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -3,6 +3,43 @@
>  # This file is a stress test of supported qapi constructs that must
>  # parse and compile correctly.
>  
> +##
> +# = Section
> +# == subsection
> +#
> +# Some text foo with *strong* and _emphasis_
> +# 1. with a list
> +# 2. like that @foo
> +#
> +# And some code:
> +# | $ echo foo
> +# | -> do this
> +# | <- get that
> +#
> +# Note: is not a meta
> +##
> +
> +##
> +# @TestStruct:
> +# @integer: foo
> +#           blah
> +#
> +#           bao
> +#
> +# @boolean: bar
> +# @string: baz
> +#
> +# body with @var
> +#
> +# Example:
> +#
> +# -> { "execute": ... }
> +# <- { "return": ... }
> +#
> +# Since: 2.3
> +# Note: a note
> +#
> +##
>  { 'struct': 'TestStruct',
>    'data': { 'integer': 'int', 'boolean': 'bool', 'string': 'str' } }
>  
> @@ -123,6 +160,27 @@
>    'data': {'ud1a': 'UserDefOne', '*ud1b': 'UserDefOne'},
>    'returns': 'UserDefTwo' }
>  
> +##
> +# Another comment
> +##
> +
> +##
> +# @guest-get-time:
> +#
> +# @a: an integer
> +# @b: #optional integer
> +#
> +# @guest-get-time body
> +#
> +# Returns: returns something
> +#
> +# Example:
> +#
> +# -> { "execute": "guest-get-time", ... }
> +# <- { "return": "42" }
> +#
> +##
> +
>  # Returning a non-dictionary requires a name from the whitelist
>  { 'command': 'guest-get-time', 'data': {'a': 'int', '*b': 'int' },
>    'returns': 'int' }
> diff --git a/tests/qapi-schema/qapi-schema-test.out 
> b/tests/qapi-schema/qapi-schema-test.out
> index 9d99c4e..fde9e06 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -232,3 +232,52 @@ command user_def_cmd1 q_obj_user_def_cmd1-arg -> None
>     gen=True success_response=True boxed=False
>  command user_def_cmd2 q_obj_user_def_cmd2-arg -> UserDefTwo
>     gen=True success_response=True boxed=False
> +doc freeform
> +    body=
> += Section
> +== subsection
> +
> +Some text foo with *strong* and _emphasis_
> +1. with a list
> +2. like that @foo
> +
> +And some code:
> +| $ echo foo
> +| -> do this
> +| <- get that
> +
> +Note: is not a meta
> +doc symbol=TestStruct expr=('struct', 'TestStruct')
> +    arg=integer
> +foo
> +blah
> +
> +bao
> +    arg=boolean
> +bar
> +    arg=string
> +baz
> +    meta=Example
> +-> { "execute": ... }
> +<- { "return": ... }
> +    meta=Since
> +2.3
> +    meta=Note
> +a note
> +    body=
> +body with @var
> +doc freeform
> +    body=
> +Another comment
> +doc symbol=guest-get-time expr=('command', 'guest-get-time')
> +    arg=a
> +an integer
> +    arg=b
> +#optional integer
> +    meta=Returns
> +returns something
> +    meta=Example
> +-> { "execute": "guest-get-time", ... }
> +<- { "return": "42" }
> +    body=
> address@hidden body
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index ef74e2c..22da014 100644
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -55,3 +55,15 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
>  
>  schema = QAPISchema(sys.argv[1])
>  schema.visit(QAPISchemaTestVisitor())
> +
> +for doc in schema.docs:
> +    if doc.symbol:
> +        print 'doc symbol=%s expr=%s' % \
> +            (doc.symbol, doc.expr.items()[0])
> +    else:
> +        print 'doc freeform'
> +    for arg, section in doc.args.iteritems():
> +        print '    arg=%s\n%s' % (arg, section)
> +    for section in doc.meta:
> +        print '    meta=%s\n%s' % (section.name, section)
> +    print '    body=\n%s' % doc.body


[1] https://www.python.org/dev/peps/pep-0257/
[2] https://google.github.io/styleguide/pyguide.html
[3] https://www.python.org/dev/peps/pep-0008/



reply via email to

[Prev in Thread] Current Thread [Next in Thread]