[Qemu-devel] [PATCH v2 0/6] file-posix: Add dynamic-auto-read-only QAPI feature

[Qemu-devel] [PATCH v2 0/6] file-posix: Add dynamic-auto-read-only QAPI feature

[Kevin Wolf]

This series adds optional feature lists to struct definitions in the
QAPI schema and makes use of them to advertise the new behaviour of
auto-read-only=on in file-posix.

v2:
- Check that features have well-formed names instead of just checking
  that they are strings
- Use QAPISchemaFeature objects instead of dicts
- Catch duplicate feature names (and test that a feature name doesn't
  conflict with an object member)
- Added patch 4 and 5 to add rudimentary support for features to
  QAPIDoc so it doesn't complain about the documentation format
  suggested by Markus. Used that format in the documentation of
  dynamic-auto-read-only.

Kevin Wolf (6):
  qapi: Support features for structs
  tests/qapi-schema: Test for good feature lists in structs
  tests/qapi-schema: Error case tests for features in structs
  qapi: Disentangle QAPIDoc code
  qapi: Allow documentation for features
  file-posix: Add dynamic-auto-read-only QAPI feature

 qapi/block-core.json                          |  13 +-
 qapi/introspect.json                          |   8 +-
 tests/qapi-schema/features-bad-type.json      |   3 +
 .../qapi-schema/features-duplicate-name.json  |   3 +
 tests/qapi-schema/features-missing-name.json  |   3 +
 tests/qapi-schema/features-name-bad-type.json |   3 +
 tests/qapi-schema/features-no-list.json       |   3 +
 tests/qapi-schema/features-unknown-key.json   |   3 +
 tests/qapi-schema/qapi-schema-test.json       |  30 +++
 docs/devel/qapi-code-gen.txt                  |  38 ++++
 scripts/qapi/common.py                        | 209 ++++++++++++++----
 scripts/qapi/doc.py                           |  14 +-
 scripts/qapi/introspect.py                    |   6 +-
 scripts/qapi/types.py                         |   3 +-
 scripts/qapi/visit.py                         |   3 +-
 tests/Makefile.include                        |   6 +
 tests/qapi-schema/double-type.err             |   2 +-
 tests/qapi-schema/features-bad-type.err       |   1 +
 tests/qapi-schema/features-bad-type.exit      |   1 +
 tests/qapi-schema/features-bad-type.out       |   0
 tests/qapi-schema/features-duplicate-name.err |   1 +
 .../qapi-schema/features-duplicate-name.exit  |   1 +
 tests/qapi-schema/features-duplicate-name.out |   0
 tests/qapi-schema/features-missing-name.err   |   1 +
 tests/qapi-schema/features-missing-name.exit  |   1 +
 tests/qapi-schema/features-missing-name.out   |   0
 tests/qapi-schema/features-name-bad-type.err  |   1 +
 tests/qapi-schema/features-name-bad-type.exit |   1 +
 tests/qapi-schema/features-name-bad-type.out  |   0
 tests/qapi-schema/features-no-list.err        |   1 +
 tests/qapi-schema/features-no-list.exit       |   1 +
 tests/qapi-schema/features-no-list.out        |   0
 tests/qapi-schema/features-unknown-key.err    |   2 +
 tests/qapi-schema/features-unknown-key.exit   |   1 +
 tests/qapi-schema/features-unknown-key.out    |   0
 tests/qapi-schema/qapi-schema-test.out        |  32 +++
 tests/qapi-schema/test-qapi.py                |   7 +-
 tests/qapi-schema/unknown-expr-key.err        |   2 +-
 38 files changed, 358 insertions(+), 46 deletions(-)
 create mode 100644 tests/qapi-schema/features-bad-type.json
 create mode 100644 tests/qapi-schema/features-duplicate-name.json
 create mode 100644 tests/qapi-schema/features-missing-name.json
 create mode 100644 tests/qapi-schema/features-name-bad-type.json
 create mode 100644 tests/qapi-schema/features-no-list.json
 create mode 100644 tests/qapi-schema/features-unknown-key.json
 create mode 100644 tests/qapi-schema/features-bad-type.err
 create mode 100644 tests/qapi-schema/features-bad-type.exit
 create mode 100644 tests/qapi-schema/features-bad-type.out
 create mode 100644 tests/qapi-schema/features-duplicate-name.err
 create mode 100644 tests/qapi-schema/features-duplicate-name.exit
 create mode 100644 tests/qapi-schema/features-duplicate-name.out
 create mode 100644 tests/qapi-schema/features-missing-name.err
 create mode 100644 tests/qapi-schema/features-missing-name.exit
 create mode 100644 tests/qapi-schema/features-missing-name.out
 create mode 100644 tests/qapi-schema/features-name-bad-type.err
 create mode 100644 tests/qapi-schema/features-name-bad-type.exit
 create mode 100644 tests/qapi-schema/features-name-bad-type.out
 create mode 100644 tests/qapi-schema/features-no-list.err
 create mode 100644 tests/qapi-schema/features-no-list.exit
 create mode 100644 tests/qapi-schema/features-no-list.out
 create mode 100644 tests/qapi-schema/features-unknown-key.err
 create mode 100644 tests/qapi-schema/features-unknown-key.exit
 create mode 100644 tests/qapi-schema/features-unknown-key.out

-- 
2.20.1

[Qemu-devel] [PATCH v2 1/6] qapi: Support features for structs

[Kevin Wolf]

Sometimes, the behaviour of QEMU changes compatibly, but without a
change in the QMP syntax (usually by allowing values or operations that
previously resulted in an error). QMP clients may still need to know
whether the extension is available.

This allows to add a list of features to struct definitions that will be
made visible to QMP clients through schema introspection.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/introspect.json                   |  8 +++-
 docs/devel/qapi-code-gen.txt           | 38 ++++++++++++++++
 scripts/qapi/common.py                 | 61 +++++++++++++++++++++-----
 scripts/qapi/doc.py                    |  3 +-
 scripts/qapi/introspect.py             |  6 ++-
 scripts/qapi/types.py                  |  3 +-
 scripts/qapi/visit.py                  |  3 +-
 tests/qapi-schema/double-type.err      |  2 +-
 tests/qapi-schema/test-qapi.py         |  3 +-
 tests/qapi-schema/unknown-expr-key.err |  2 +-
 10 files changed, 111 insertions(+), 18 deletions(-)

diff --git a/qapi/introspect.json b/qapi/introspect.json
index 3d22166b2b..3cb6c1aca4 100644
--- a/qapi/introspect.json
+++ b/qapi/introspect.json
@@ -174,6 +174,11 @@
 #            and may even differ from the order of the values of the
 #            enum type of the @tag.
 #
+# @features: names of features that are supported by this version and build and
+#            aren't othervise visible through schema introspection (e.g. change
+#            of behaviour related to an object type that didn't come with a
+#            syntactic change in the schema of the object type) (since: 4.1)
+#
 # Values of this type are JSON object on the wire.
 #
 # Since: 2.5
@@ -181,7 +186,8 @@
 { 'struct': 'SchemaInfoObject',
   'data': { 'members': [ 'SchemaInfoObjectMember' ],
             '*tag': 'str',
-            '*variants': [ 'SchemaInfoObjectVariant' ] } }
+            '*variants': [ 'SchemaInfoObjectVariant' ],
+            '*features': [ 'str' ] } }
 
 ##
 # @SchemaInfoObjectMember:
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index b517b0cfbf..e8ec8ac1de 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -719,6 +719,34 @@ any non-empty complex type (struct, union, or alternate), and a
 pointer to that QAPI type is passed as a single argument.
 
 
+=== Features ===
+
+Sometimes, the behaviour of QEMU changes compatibly, but without a
+change in the QMP syntax (usually by allowing values or operations that
+previously resulted in an error). QMP clients may still need to know
+whether the extension is available.
+
+For this purpose, a list of features can be specified for a struct type.
+This is exposed to the client as a list of string, where each string
+signals that this build of QEMU shows a certain behaviour.
+
+In the schema, features can be specified as simple strings, for example:
+
+{ 'struct': 'TestType',
+  'data': { 'number': 'int' },
+  'features': [ 'allow-negative-numbers' ] }
+
+Another option is to specify features as dictionaries, where the key
+'name' specifies the feature string to be exposed to clients:
+
+{ 'struct': 'TestType',
+  'data': { 'number': 'int' },
+  'features': [ { 'name': 'allow-negative-numbers' } ] }
+
+This expanded form is necessary if you want to make the feature
+conditional (see below in "Configuring the schema").
+
+
 === Downstream extensions ===
 
 QAPI schema names that are externally visible, say in the Client JSON
@@ -771,6 +799,16 @@ Example: a conditional 'bar' enum member.
   [ 'foo',
     { 'name' : 'bar', 'if': 'defined(IFCOND)' } ] }
 
+Similarly, features can be specified as a dictionary with a 'name' and
+an 'if' key.
+
+Example: a conditional 'allow-negative-numbers' feature
+
+{ 'struct': 'TestType',
+  'data': { 'number': 'int' },
+  'features': [ { 'name': 'allow-negative-numbers',
+                  'if' 'defined(IFCOND)' } ] }
+
 Please note that you are responsible to ensure that the C code will
 compile with an arbitrary combination of conditions, since the
 generators are unable to check it at this point.
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index f07869ec73..71944e2e30 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -886,12 +886,25 @@ def check_enum(expr, info):
 def check_struct(expr, info):
     name = expr['struct']
     members = expr['data']
+    features = expr.get('features')
 
     check_type(info, "'data' for struct '%s'" % name, members,
                allow_dict=True, allow_optional=True)
     check_type(info, "'base' for struct '%s'" % name, expr.get('base'),
                allow_metas=['struct'])
 
+    if features:
+        if not isinstance(features, list):
+            raise QAPISemError(info,
+                               "Struct '%s' requires an array for 'features'" %
+                               name)
+        for f in features:
+            assert isinstance(f, dict)
+            check_known_keys(info, "feature of struct %s" % (name), f,
+                             ['name'], ['if'])
+
+            check_if(f, info)
+            check_name(info, "Feature of struct %s" % (name), f['name'])
 
 def check_known_keys(info, source, keys, required, optional):
 
@@ -947,6 +960,10 @@ def normalize_members(members):
                 continue
             members[key] = {'type': arg}
 
+def normalize_features(features):
+    if isinstance(features, list):
+        features[:] = [f if isinstance(f, dict) else {'name': f}
+                       for f in features]
 
 def check_exprs(exprs):
     global all_names
@@ -986,8 +1003,10 @@ def check_exprs(exprs):
             normalize_members(expr['data'])
         elif 'struct' in expr:
             meta = 'struct'
-            check_keys(expr_elem, 'struct', ['data'], ['base', 'if'])
+            check_keys(expr_elem, 'struct', ['data'],
+                       ['base', 'if', 'features'])
             normalize_members(expr['data'])
+            normalize_features(expr.get('features'))
             struct_types[expr[meta]] = expr
         elif 'command' in expr:
             meta = 'command'
@@ -1126,10 +1145,12 @@ class QAPISchemaVisitor(object):
     def visit_array_type(self, name, info, ifcond, element_type):
         pass
 
-    def visit_object_type(self, name, info, ifcond, base, members, variants):
+    def visit_object_type(self, name, info, ifcond, base, members, variants,
+                          features):
         pass
 
-    def visit_object_type_flat(self, name, info, ifcond, members, variants):
+    def visit_object_type_flat(self, name, info, ifcond, members, variants,
+                               features):
         pass
 
     def visit_alternate_type(self, name, info, ifcond, variants):
@@ -1290,7 +1311,7 @@ class QAPISchemaArrayType(QAPISchemaType):
 
 class QAPISchemaObjectType(QAPISchemaType):
     def __init__(self, name, info, doc, ifcond,
-                 base, local_members, variants):
+                 base, local_members, variants, features):
         # struct has local_members, optional base, and no variants
         # flat union has base, variants, and no local_members
         # simple union has local_members, variants, and no base
@@ -1302,11 +1323,15 @@ class QAPISchemaObjectType(QAPISchemaType):
         if variants is not None:
             assert isinstance(variants, QAPISchemaObjectTypeVariants)
             variants.set_owner(name)
+        for f in features:
+            assert isinstance(f, QAPISchemaFeature)
+            f.set_owner(name)
         self._base_name = base
         self.base = None
         self.local_members = local_members
         self.variants = variants
         self.members = None
+        self.features = features
 
     def check(self, schema):
         QAPISchemaType.check(self, schema)
@@ -1332,6 +1357,12 @@ class QAPISchemaObjectType(QAPISchemaType):
             self.variants.check(schema, seen)
             assert self.variants.tag_member in self.members
             self.variants.check_clash(self.info, seen)
+
+        # Features are in a namespace separate from members
+        seen = OrderedDict()
+        for f in self.features:
+            f.check_clash(self.info, seen)
+
         if self.doc:
             self.doc.check()
 
@@ -1368,12 +1399,15 @@ class QAPISchemaObjectType(QAPISchemaType):
 
     def visit(self, visitor):
         visitor.visit_object_type(self.name, self.info, self.ifcond,
-                                  self.base, self.local_members, self.variants)
+                                  self.base, self.local_members, self.variants,
+                                  self.features)
         visitor.visit_object_type_flat(self.name, self.info, self.ifcond,
-                                       self.members, self.variants)
+                                       self.members, self.variants,
+                                       self.features)
 
 
 class QAPISchemaMember(object):
+    """ Represents object members, enum members and features """
     role = 'member'
 
     def __init__(self, name, ifcond=None):
@@ -1418,6 +1452,8 @@ class QAPISchemaMember(object):
     def describe(self):
         return "'%s' %s" % (self.name, self._pretty_owner())
 
+class QAPISchemaFeature(QAPISchemaMember):
+    role = 'feature'
 
 class QAPISchemaObjectTypeMember(QAPISchemaMember):
     def __init__(self, name, typ, optional, ifcond=None):
@@ -1675,7 +1711,7 @@ class QAPISchema(object):
                   ('null',   'null',    'QNull' + pointer_suffix)]:
             self._def_builtin_type(*t)
         self.the_empty_object_type = QAPISchemaObjectType(
-            'q_empty', None, None, None, None, [], None)
+            'q_empty', None, None, None, None, [], None, [])
         self._def_entity(self.the_empty_object_type)
 
         qtypes = ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist',
@@ -1685,6 +1721,9 @@ class QAPISchema(object):
         self._def_entity(QAPISchemaEnumType('QType', None, None, None,
                                             qtype_values, 'QTYPE'))
 
+    def _make_features(self, features):
+        return [QAPISchemaFeature(f['name'], f.get('if')) for f in features]
+
     def _make_enum_members(self, values):
         return [QAPISchemaMember(v['name'], v.get('if')) for v in values]
 
@@ -1721,7 +1760,7 @@ class QAPISchema(object):
             assert ifcond == typ._ifcond # pylint: disable=protected-access
         else:
             self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond,
-                                                  None, members, None))
+                                                  None, members, None, []))
         return name
 
     def _def_enum_type(self, expr, info, doc):
@@ -1752,9 +1791,11 @@ class QAPISchema(object):
         base = expr.get('base')
         data = expr['data']
         ifcond = expr.get('if')
+        features = expr.get('features', [])
         self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond, base,
                                               self._make_members(data, info),
-                                              None))
+                                              None,
+                                              self._make_features(features)))
 
     def _make_variant(self, case, typ, ifcond):
         return QAPISchemaObjectTypeVariant(case, typ, ifcond)
@@ -1795,7 +1836,7 @@ class QAPISchema(object):
             QAPISchemaObjectType(name, info, doc, ifcond, base, members,
                                  QAPISchemaObjectTypeVariants(tag_name,
                                                               tag_member,
-                                                              variants)))
+                                                              variants), []))
 
     def _def_alternate_type(self, expr, info, doc):
         name = expr['alternate']
diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
index 5c8c136899..433e9fcbfb 100755
--- a/scripts/qapi/doc.py
+++ b/scripts/qapi/doc.py
@@ -220,7 +220,8 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
                                body=texi_entity(doc, 'Values', ifcond,
                                                 member_func=texi_enum_value)))
 
-    def visit_object_type(self, name, info, ifcond, base, members, variants):
+    def visit_object_type(self, name, info, ifcond, base, members, variants,
+                          features):
         doc = self.cur_doc
         if base and base.is_implicit():
             base = None
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index f7f2ca07e4..62cbf94a85 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -188,11 +188,15 @@ const QLitObject %(c_name)s = %(c_string)s;
         self._gen_qlit('[' + element + ']', 'array', {'element-type': element},
                        ifcond)
 
-    def visit_object_type_flat(self, name, info, ifcond, members, variants):
+    def visit_object_type_flat(self, name, info, ifcond, members, variants,
+                               features):
         obj = {'members': [self._gen_member(m) for m in members]}
         if variants:
             obj.update(self._gen_variants(variants.tag_member.name,
                                           variants.variants))
+        if features:
+            obj['features'] = [ (f.name, {'if': f.ifcond}) for f in features ]
+
         self._gen_qlit(name, 'object', obj, ifcond)
 
     def visit_alternate_type(self, name, info, ifcond, variants):
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 2bd6fcd44f..3edd9374aa 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -227,7 +227,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor):
             self._genh.add(gen_array(name, element_type))
             self._gen_type_cleanup(name)
 
-    def visit_object_type(self, name, info, ifcond, base, members, variants):
+    def visit_object_type(self, name, info, ifcond, base, members, variants,
+                          features):
         # Nothing to do for the special empty builtin
         if name == 'q_empty':
             return
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 826b8066e1..c1cd675c95 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -326,7 +326,8 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor):
             self._genh.add(gen_visit_decl(name))
             self._genc.add(gen_visit_list(name, element_type))
 
-    def visit_object_type(self, name, info, ifcond, base, members, variants):
+    def visit_object_type(self, name, info, ifcond, base, members, variants,
+                          features):
         # Nothing to do for the special empty builtin
         if name == 'q_empty':
             return
diff --git a/tests/qapi-schema/double-type.err b/tests/qapi-schema/double-type.err
index 799193dba1..69457173a7 100644
--- a/tests/qapi-schema/double-type.err
+++ b/tests/qapi-schema/double-type.err
@@ -1,2 +1,2 @@
 tests/qapi-schema/double-type.json:2: Unknown key 'command' in struct 'bar'
-Valid keys are 'base', 'data', 'if', 'struct'.
+Valid keys are 'base', 'data', 'features', 'if', 'struct'.
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index d21fca01fc..f2d6815c86 100644
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -38,7 +38,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
         print('array %s %s' % (name, element_type.name))
         self._print_if(ifcond)
 
-    def visit_object_type(self, name, info, ifcond, base, members, variants):
+    def visit_object_type(self, name, info, ifcond, base, members, variants,
+                          features):
         print('object %s' % name)
         if base:
             print('    base %s' % base.name)
diff --git a/tests/qapi-schema/unknown-expr-key.err b/tests/qapi-schema/unknown-expr-key.err
index 6ff8bb99c5..4340eaf894 100644
--- a/tests/qapi-schema/unknown-expr-key.err
+++ b/tests/qapi-schema/unknown-expr-key.err
@@ -1,2 +1,2 @@
 tests/qapi-schema/unknown-expr-key.json:2: Unknown keys 'bogus', 'phony' in struct 'bar'
-Valid keys are 'base', 'data', 'if', 'struct'.
+Valid keys are 'base', 'data', 'features', 'if', 'struct'.
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 1/6] qapi: Support features for structs

[Markus Armbruster]

"Support features for structs" accidentally fits the "Do stuff"
commit message anti-pattern.  Suggest:

    qapi: Add feature flags to struct types

Kevin Wolf <kwolf@redhat.com> writes:

> Sometimes, the behaviour of QEMU changes compatibly, but without a
> change in the QMP syntax (usually by allowing values or operations that
> previously resulted in an error). QMP clients may still need to know
> whether the extension is available.
>
> This allows to add a list of features to struct definitions that will be

What's "this"?  Do you mean "This patch allows"?

> made visible to QMP clients through schema introspection.

Attentive readers might ask why only structs.  The answer is of course
that structs suffice for your immediate needs, and you chose to keep
this patch simple by limiting it to what you actually need.

Now let me try to work that into your commit message:

    qapi: Add feature flags to struct types

    Sometimes, the behaviour of QEMU changes without a change in the QMP
    syntax (usually by allowing values or operations that previously
    resulted in an error).  QMP clients may still need to know whether
    they can rely on the changed behavior.

    To let us make such changes visible in introspection, add feature
    flags to the QAPI schema language.  Looks like this:

        { 'struct': 'TestType',
          'data': { 'number': 'int' },
          'features': [ 'allow-negative-numbers' ] }

    Introspection then looks like

        { "name": "TestType", "meta-type": "object",
          "members": [
              { "name": "number", "type": "int" } ],
          "features": [ "allow-negative-numbers" ] }

    This patch implements feature flags just for struct types.  We'll
    implement them more widely as needed.

>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qapi/introspect.json                   |  8 +++-
>  docs/devel/qapi-code-gen.txt           | 38 ++++++++++++++++
>  scripts/qapi/common.py                 | 61 +++++++++++++++++++++-----
>  scripts/qapi/doc.py                    |  3 +-
>  scripts/qapi/introspect.py             |  6 ++-
>  scripts/qapi/types.py                  |  3 +-
>  scripts/qapi/visit.py                  |  3 +-
>  tests/qapi-schema/double-type.err      |  2 +-
>  tests/qapi-schema/test-qapi.py         |  3 +-
>  tests/qapi-schema/unknown-expr-key.err |  2 +-
>  10 files changed, 111 insertions(+), 18 deletions(-)
>
> diff --git a/qapi/introspect.json b/qapi/introspect.json
> index 3d22166b2b..3cb6c1aca4 100644
> --- a/qapi/introspect.json
> +++ b/qapi/introspect.json
> @@ -174,6 +174,11 @@
>  #            and may even differ from the order of the values of the
>  #            enum type of the @tag.
>  #
> +# @features: names of features that are supported by this version and build and
> +#            aren't othervise visible through schema introspection (e.g. change
> +#            of behaviour related to an object type that didn't come with a
> +#            syntactic change in the schema of the object type) (since: 4.1)
> +#

Specify "in no particular order", like we do elsewhere.

Let's wrap lines a little earlier.

I'm not sure this is the best place to explain the indented purpose of
QAPI feature flags.  Other SchemaInfoFOO will acquire @features.  I feel
each instance should carry just terse specification.  Anything beyond
that should be in just one place.  Right now, query-qmp-schema looks
like the best fit.  We could instead write an introduction to
introspection as a file comment, right under "# = QMP introspection".

>  # Values of this type are JSON object on the wire.
>  #
>  # Since: 2.5
> @@ -181,7 +186,8 @@
>  { 'struct': 'SchemaInfoObject',
>    'data': { 'members': [ 'SchemaInfoObjectMember' ],
>              '*tag': 'str',
> -            '*variants': [ 'SchemaInfoObjectVariant' ] } }
> +            '*variants': [ 'SchemaInfoObjectVariant' ],
> +            '*features': [ 'str' ] } }
>  
>  ##
>  # @SchemaInfoObjectMember:
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index b517b0cfbf..e8ec8ac1de 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -719,6 +719,34 @@ any non-empty complex type (struct, union, or alternate), and a
>  pointer to that QAPI type is passed as a single argument.
>  
>  
> +=== Features ===
> +
> +Sometimes, the behaviour of QEMU changes compatibly, but without a
> +change in the QMP syntax (usually by allowing values or operations that
> +previously resulted in an error). QMP clients may still need to know
> +whether the extension is available.
> +
> +For this purpose, a list of features can be specified for a struct type.
> +This is exposed to the client as a list of string, where each string
> +signals that this build of QEMU shows a certain behaviour.
> +
> +In the schema, features can be specified as simple strings, for example:
> +
> +{ 'struct': 'TestType',
> +  'data': { 'number': 'int' },
> +  'features': [ 'allow-negative-numbers' ] }
> +
> +Another option is to specify features as dictionaries, where the key
> +'name' specifies the feature string to be exposed to clients:
> +
> +{ 'struct': 'TestType',
> +  'data': { 'number': 'int' },
> +  'features': [ { 'name': 'allow-negative-numbers' } ] }
> +
> +This expanded form is necessary if you want to make the feature
> +conditional (see below in "Configuring the schema").
> +
> +
>  === Downstream extensions ===
>  
>  QAPI schema names that are externally visible, say in the Client JSON
> @@ -771,6 +799,16 @@ Example: a conditional 'bar' enum member.
>    [ 'foo',
>      { 'name' : 'bar', 'if': 'defined(IFCOND)' } ] }
>  
> +Similarly, features can be specified as a dictionary with a 'name' and
> +an 'if' key.
> +
> +Example: a conditional 'allow-negative-numbers' feature
> +
> +{ 'struct': 'TestType',
> +  'data': { 'number': 'int' },
> +  'features': [ { 'name': 'allow-negative-numbers',
> +                  'if' 'defined(IFCOND)' } ] }
> +
>  Please note that you are responsible to ensure that the C code will
>  compile with an arbitrary combination of conditions, since the
>  generators are unable to check it at this point.

Feels bolted on, just like conditionals.  I'm afraid we'll have to
rework this document to arrest its deterioration.

I'm not asking you to do this work.  Let's move on with this patch.

> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index f07869ec73..71944e2e30 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -886,12 +886,25 @@ def check_enum(expr, info):
>  def check_struct(expr, info):
>      name = expr['struct']
>      members = expr['data']
> +    features = expr.get('features')
>  
>      check_type(info, "'data' for struct '%s'" % name, members,
>                 allow_dict=True, allow_optional=True)
>      check_type(info, "'base' for struct '%s'" % name, expr.get('base'),
>                 allow_metas=['struct'])
>  
> +    if features:
> +        if not isinstance(features, list):
> +            raise QAPISemError(info,
> +                               "Struct '%s' requires an array for 'features'" %
> +                               name)
> +        for f in features:
> +            assert isinstance(f, dict)
> +            check_known_keys(info, "feature of struct %s" % (name), f,

Redundant parenthesis: (name)

> +                             ['name'], ['if'])
> +
> +            check_if(f, info)
> +            check_name(info, "Feature of struct %s" % (name), f['name'])

Again.

You managed to reduce the differences to check_enum() nicely since v1,
paving the way for future de-duplication.  I'm not asking you to
de-duplicate now.

pycodestyle-3 complains about whitespace here, and in a few other
places.  Please use it to check your patches for avoidable style nits.

>  
>  def check_known_keys(info, source, keys, required, optional):
>  
> @@ -947,6 +960,10 @@ def normalize_members(members):
>                  continue
>              members[key] = {'type': arg}
>  
> +def normalize_features(features):
> +    if isinstance(features, list):
> +        features[:] = [f if isinstance(f, dict) else {'name': f}
> +                       for f in features]

Duplicates the meat of normalize_enum().  I'm not asking you to
de-duplicate now.

>  
>  def check_exprs(exprs):
>      global all_names
> @@ -986,8 +1003,10 @@ def check_exprs(exprs):
>              normalize_members(expr['data'])
>          elif 'struct' in expr:
>              meta = 'struct'
> -            check_keys(expr_elem, 'struct', ['data'], ['base', 'if'])
> +            check_keys(expr_elem, 'struct', ['data'],
> +                       ['base', 'if', 'features'])
>              normalize_members(expr['data'])
> +            normalize_features(expr.get('features'))
>              struct_types[expr[meta]] = expr
>          elif 'command' in expr:
>              meta = 'command'
> @@ -1126,10 +1145,12 @@ class QAPISchemaVisitor(object):
>      def visit_array_type(self, name, info, ifcond, element_type):
>          pass
>  
> -    def visit_object_type(self, name, info, ifcond, base, members, variants):
> +    def visit_object_type(self, name, info, ifcond, base, members, variants,
> +                          features):
>          pass
>  
> -    def visit_object_type_flat(self, name, info, ifcond, members, variants):
> +    def visit_object_type_flat(self, name, info, ifcond, members, variants,
> +                               features):
>          pass
>  
>      def visit_alternate_type(self, name, info, ifcond, variants):
> @@ -1290,7 +1311,7 @@ class QAPISchemaArrayType(QAPISchemaType):
>  
>  class QAPISchemaObjectType(QAPISchemaType):
>      def __init__(self, name, info, doc, ifcond,
> -                 base, local_members, variants):
> +                 base, local_members, variants, features):
>          # struct has local_members, optional base, and no variants
>          # flat union has base, variants, and no local_members
>          # simple union has local_members, variants, and no base
> @@ -1302,11 +1323,15 @@ class QAPISchemaObjectType(QAPISchemaType):
>          if variants is not None:
>              assert isinstance(variants, QAPISchemaObjectTypeVariants)
>              variants.set_owner(name)
> +        for f in features:
> +            assert isinstance(f, QAPISchemaFeature)
> +            f.set_owner(name)
>          self._base_name = base
>          self.base = None
>          self.local_members = local_members
>          self.variants = variants
>          self.members = None
> +        self.features = features
>  
>      def check(self, schema):
>          QAPISchemaType.check(self, schema)
           if self.members is False:               # check for cycles
               raise QAPISemError(self.info,
                                  "Object %s contains itself" % self.name)
           if self.members:
               return
           self.members = False                    # mark as being checked
           seen = OrderedDict()
           if self._base_name:
               self.base = schema.lookup_type(self._base_name)
               assert isinstance(self.base, QAPISchemaObjectType)
               self.base.check(schema)
               self.base.check_clash(self.info, seen)
           for m in self.local_members:
               m.check(schema)
               m.check_clash(self.info, seen)
               if self.doc:
                   self.doc.connect_member(m)
           self.members = seen.values()
           if self.variants:
> @@ -1332,6 +1357,12 @@ class QAPISchemaObjectType(QAPISchemaType):
>              self.variants.check(schema, seen)
>              assert self.variants.tag_member in self.members
>              self.variants.check_clash(self.info, seen)
> +
> +        # Features are in a namespace separate from members

name space

> +        seen = OrderedDict()

seen = {} suffices.  It doesn't for members (above), because there, we
want seen.values() preserve order.  See commit 23a4b2c6f19.

Fun fact: dict is guaranteed to preserve order as of Python 3.7.

> +        for f in self.features:
> +            f.check_clash(self.info, seen)
> +
>          if self.doc:
>              self.doc.check()
>  
> @@ -1368,12 +1399,15 @@ class QAPISchemaObjectType(QAPISchemaType):
>  
>      def visit(self, visitor):
>          visitor.visit_object_type(self.name, self.info, self.ifcond,
> -                                  self.base, self.local_members, self.variants)
> +                                  self.base, self.local_members, self.variants,
> +                                  self.features)
>          visitor.visit_object_type_flat(self.name, self.info, self.ifcond,
> -                                       self.members, self.variants)
> +                                       self.members, self.variants,
> +                                       self.features)
>  
>  
>  class QAPISchemaMember(object):
> +    """ Represents object members, enum members and features """
>      role = 'member'
>  
>      def __init__(self, name, ifcond=None):
> @@ -1418,6 +1452,8 @@ class QAPISchemaMember(object):
>      def describe(self):
>          return "'%s' %s" % (self.name, self._pretty_owner())
>  
> +class QAPISchemaFeature(QAPISchemaMember):
> +    role = 'feature'
>  
>  class QAPISchemaObjectTypeMember(QAPISchemaMember):
>      def __init__(self, name, typ, optional, ifcond=None):
> @@ -1675,7 +1711,7 @@ class QAPISchema(object):
>                    ('null',   'null',    'QNull' + pointer_suffix)]:
>              self._def_builtin_type(*t)
>          self.the_empty_object_type = QAPISchemaObjectType(
> -            'q_empty', None, None, None, None, [], None)
> +            'q_empty', None, None, None, None, [], None, [])
>          self._def_entity(self.the_empty_object_type)
>  
>          qtypes = ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist',
> @@ -1685,6 +1721,9 @@ class QAPISchema(object):
>          self._def_entity(QAPISchemaEnumType('QType', None, None, None,
>                                              qtype_values, 'QTYPE'))
>  
> +    def _make_features(self, features):
> +        return [QAPISchemaFeature(f['name'], f.get('if')) for f in features]
> +
>      def _make_enum_members(self, values):
>          return [QAPISchemaMember(v['name'], v.get('if')) for v in values]
>  
> @@ -1721,7 +1760,7 @@ class QAPISchema(object):
>              assert ifcond == typ._ifcond # pylint: disable=protected-access
>          else:
>              self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond,
> -                                                  None, members, None))
> +                                                  None, members, None, []))
>          return name
>  
>      def _def_enum_type(self, expr, info, doc):
> @@ -1752,9 +1791,11 @@ class QAPISchema(object):
>          base = expr.get('base')
>          data = expr['data']
>          ifcond = expr.get('if')
> +        features = expr.get('features', [])
>          self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond, base,
>                                                self._make_members(data, info),
> -                                              None))
> +                                              None,
> +                                              self._make_features(features)))

I like this much better than v1, because it's so close to the IR for
similar things.

>  
>      def _make_variant(self, case, typ, ifcond):
>          return QAPISchemaObjectTypeVariant(case, typ, ifcond)
> @@ -1795,7 +1836,7 @@ class QAPISchema(object):
>              QAPISchemaObjectType(name, info, doc, ifcond, base, members,
>                                   QAPISchemaObjectTypeVariants(tag_name,
>                                                                tag_member,
> -                                                              variants)))
> +                                                              variants), []))
>  
>      def _def_alternate_type(self, expr, info, doc):
>          name = expr['alternate']
> diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
> index 5c8c136899..433e9fcbfb 100755
> --- a/scripts/qapi/doc.py
> +++ b/scripts/qapi/doc.py
> @@ -220,7 +220,8 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor):
>                                 body=texi_entity(doc, 'Values', ifcond,
>                                                  member_func=texi_enum_value)))
>  
> -    def visit_object_type(self, name, info, ifcond, base, members, variants):
> +    def visit_object_type(self, name, info, ifcond, base, members, variants,
> +                          features):
>          doc = self.cur_doc
>          if base and base.is_implicit():
>              base = None
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index f7f2ca07e4..62cbf94a85 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -188,11 +188,15 @@ const QLitObject %(c_name)s = %(c_string)s;
>          self._gen_qlit('[' + element + ']', 'array', {'element-type': element},
>                         ifcond)
>  
> -    def visit_object_type_flat(self, name, info, ifcond, members, variants):
> +    def visit_object_type_flat(self, name, info, ifcond, members, variants,
> +                               features):
>          obj = {'members': [self._gen_member(m) for m in members]}
>          if variants:
>              obj.update(self._gen_variants(variants.tag_member.name,
>                                            variants.variants))
> +        if features:
> +            obj['features'] = [ (f.name, {'if': f.ifcond}) for f in features ]
> +

Another place where features have become like enums (see
visit_enum_type()).  De-duplication seems not worthwhile here.

>          self._gen_qlit(name, 'object', obj, ifcond)
>  
>      def visit_alternate_type(self, name, info, ifcond, variants):
> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> index 2bd6fcd44f..3edd9374aa 100644
> --- a/scripts/qapi/types.py
> +++ b/scripts/qapi/types.py
> @@ -227,7 +227,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor):
>              self._genh.add(gen_array(name, element_type))
>              self._gen_type_cleanup(name)
>  
> -    def visit_object_type(self, name, info, ifcond, base, members, variants):
> +    def visit_object_type(self, name, info, ifcond, base, members, variants,
> +                          features):
>          # Nothing to do for the special empty builtin
>          if name == 'q_empty':
>              return
> diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> index 826b8066e1..c1cd675c95 100644
> --- a/scripts/qapi/visit.py
> +++ b/scripts/qapi/visit.py
> @@ -326,7 +326,8 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor):
>              self._genh.add(gen_visit_decl(name))
>              self._genc.add(gen_visit_list(name, element_type))
>  
> -    def visit_object_type(self, name, info, ifcond, base, members, variants):
> +    def visit_object_type(self, name, info, ifcond, base, members, variants,
> +                          features):
>          # Nothing to do for the special empty builtin
>          if name == 'q_empty':
>              return
> diff --git a/tests/qapi-schema/double-type.err b/tests/qapi-schema/double-type.err
> index 799193dba1..69457173a7 100644
> --- a/tests/qapi-schema/double-type.err
> +++ b/tests/qapi-schema/double-type.err
> @@ -1,2 +1,2 @@
>  tests/qapi-schema/double-type.json:2: Unknown key 'command' in struct 'bar'
> -Valid keys are 'base', 'data', 'if', 'struct'.
> +Valid keys are 'base', 'data', 'features', 'if', 'struct'.
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index d21fca01fc..f2d6815c86 100644
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -38,7 +38,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
>          print('array %s %s' % (name, element_type.name))
>          self._print_if(ifcond)
>  
> -    def visit_object_type(self, name, info, ifcond, base, members, variants):
> +    def visit_object_type(self, name, info, ifcond, base, members, variants,
> +                          features):
>          print('object %s' % name)
>          if base:
>              print('    base %s' % base.name)

Printing @features is left to PATCH 2.  Okay.

> diff --git a/tests/qapi-schema/unknown-expr-key.err b/tests/qapi-schema/unknown-expr-key.err
> index 6ff8bb99c5..4340eaf894 100644
> --- a/tests/qapi-schema/unknown-expr-key.err
> +++ b/tests/qapi-schema/unknown-expr-key.err
> @@ -1,2 +1,2 @@
>  tests/qapi-schema/unknown-expr-key.json:2: Unknown keys 'bogus', 'phony' in struct 'bar'
> -Valid keys are 'base', 'data', 'if', 'struct'.
> +Valid keys are 'base', 'data', 'features', 'if', 'struct'.

Looks pretty good now!

Re: [Qemu-devel] [PATCH v2 1/6] qapi: Support features for structs

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 921 bytes --]

On 5/24/19 8:20 AM, Markus Armbruster wrote:
> "Support features for structs" accidentally fits the "Do stuff"
> commit message anti-pattern.  Suggest:
> 
>     qapi: Add feature flags to struct types
> 

>> +# @features: names of features that are supported by this version and build and
>> +#            aren't othervise visible through schema introspection (e.g. change

s/othervise/otherwise/

>> +#            of behaviour related to an object type that didn't come with a

Do we have a strong US or UK preference on spelling in user-visible
docs? (US would use "behavior")

>> +#            syntactic change in the schema of the object type) (since: 4.1)
>> +#
> 
> Specify "in no particular order", like we do elsewhere.
> 
> Let's wrap lines a little earlier.
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PATCH v2 2/6] tests/qapi-schema: Test for good feature lists in structs

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qapi-schema/qapi-schema-test.json | 30 +++++++++++++++++++++++
 tests/qapi-schema/qapi-schema-test.out  | 32 +++++++++++++++++++++++++
 tests/qapi-schema/test-qapi.py          |  4 ++++
 3 files changed, 66 insertions(+)

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 0952c68734..35a50fbe54 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -242,3 +242,33 @@
   { 'foo': 'TestIfStruct',
     'bar': { 'type': ['TestIfEnum'], 'if': 'defined(TEST_IF_EVT_BAR)' } },
   'if': 'defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)' }
+
+# test 'features' for structs
+
+{ 'struct': 'FeatureStruct0',
+  'data': { 'foo': 'int' },
+  'features': [] }
+{ 'struct': 'FeatureStruct1',
+  'data': { 'foo': 'int' },
+  'features': [ 'feature1' ] }
+{ 'struct': 'FeatureStruct2',
+  'data': { 'foo': 'int' },
+  'features': [ { 'name': 'feature1' } ] }
+{ 'struct': 'FeatureStruct3',
+  'data': { 'foo': 'int' },
+  'features': [ 'feature1', 'feature2' ] }
+{ 'struct': 'FeatureStruct4',
+  'data': { 'namespace-test': 'int' },
+  'features': [ 'namespace-test', 'int', 'name', 'if' ] }
+
+{ 'struct': 'CondFeatureStruct1',
+  'data': { 'foo': 'int' },
+  'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'} ] }
+{ 'struct': 'CondFeatureStruct2',
+  'data': { 'foo': 'int' },
+  'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'},
+                { 'name': 'feature2', 'if': 'defined(TEST_IF_FEATURE_2)'} ] }
+{ 'struct': 'CondFeatureStruct3',
+  'data': { 'foo': 'int' },
+  'features': [ { 'name': 'feature1', 'if': [ 'defined(TEST_IF_COND_1)',
+                                              'defined(TEST_IF_COND_2)'] } ] }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 77fb1e1aa9..54255e5708 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -354,3 +354,35 @@ object q_obj_TestIfEvent-arg
 event TestIfEvent q_obj_TestIfEvent-arg
    boxed=False
     if ['defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)']
+object FeatureStruct0
+    member foo: int optional=False
+object FeatureStruct1
+    member foo: int optional=False
+    feature feature1
+object FeatureStruct2
+    member foo: int optional=False
+    feature feature1
+object FeatureStruct3
+    member foo: int optional=False
+    feature feature1
+    feature feature2
+object FeatureStruct4
+    member namespace-test: int optional=False
+    feature namespace-test
+    feature int
+    feature name
+    feature if
+object CondFeatureStruct1
+    member foo: int optional=False
+    feature feature1
+        if ['defined(TEST_IF_FEATURE_1)']
+object CondFeatureStruct2
+    member foo: int optional=False
+    feature feature1
+        if ['defined(TEST_IF_FEATURE_1)']
+    feature feature2
+        if ['defined(TEST_IF_FEATURE_2)']
+object CondFeatureStruct3
+    member foo: int optional=False
+    feature feature1
+        if ['defined(TEST_IF_COND_1)', 'defined(TEST_IF_COND_2)']
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index f2d6815c86..b0f770b9bd 100644
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -49,6 +49,10 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
             self._print_if(m.ifcond, 8)
         self._print_variants(variants)
         self._print_if(ifcond)
+        if features:
+            for f in features:
+                print('    feature %s' % f.name)
+                self._print_if(f.ifcond, 8)
 
     def visit_alternate_type(self, name, info, ifcond, variants):
         print('alternate %s' % name)
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 2/6] tests/qapi-schema: Test for good feature lists in structs

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qapi-schema/qapi-schema-test.json | 30 +++++++++++++++++++++++
>  tests/qapi-schema/qapi-schema-test.out  | 32 +++++++++++++++++++++++++
>  tests/qapi-schema/test-qapi.py          |  4 ++++
>  3 files changed, 66 insertions(+)
>
> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index 0952c68734..35a50fbe54 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -242,3 +242,33 @@
>    { 'foo': 'TestIfStruct',
>      'bar': { 'type': ['TestIfEnum'], 'if': 'defined(TEST_IF_EVT_BAR)' } },
>    'if': 'defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)' }
> +
> +# test 'features' for structs
> +
> +{ 'struct': 'FeatureStruct0',
> +  'data': { 'foo': 'int' },
> +  'features': [] }
> +{ 'struct': 'FeatureStruct1',
> +  'data': { 'foo': 'int' },
> +  'features': [ 'feature1' ] }
> +{ 'struct': 'FeatureStruct2',
> +  'data': { 'foo': 'int' },
> +  'features': [ { 'name': 'feature1' } ] }
> +{ 'struct': 'FeatureStruct3',
> +  'data': { 'foo': 'int' },
> +  'features': [ 'feature1', 'feature2' ] }
> +{ 'struct': 'FeatureStruct4',
> +  'data': { 'namespace-test': 'int' },
> +  'features': [ 'namespace-test', 'int', 'name', 'if' ] }
> +
> +{ 'struct': 'CondFeatureStruct1',
> +  'data': { 'foo': 'int' },
> +  'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'} ] }
> +{ 'struct': 'CondFeatureStruct2',
> +  'data': { 'foo': 'int' },
> +  'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'},
> +                { 'name': 'feature2', 'if': 'defined(TEST_IF_FEATURE_2)'} ] }
> +{ 'struct': 'CondFeatureStruct3',
> +  'data': { 'foo': 'int' },
> +  'features': [ { 'name': 'feature1', 'if': [ 'defined(TEST_IF_COND_1)',
> +                                              'defined(TEST_IF_COND_2)'] } ] }

Let's add

   { 'command': 'test-features',
     'data': { 'fs0': 'FeatureStruct0',
               'fs1': 'FeatureStruct1',
               'fs2': 'FeatureStruct2',
               'fs3': 'FeatureStruct3',
               'cfs1': 'CondFeatureStruct1',
               'cfs2': 'CondFeatureStruct2',
               'cfs3': 'CondFeatureStruct3' } }

because without it, the feature test cases won't generate introspection
code.

[...]

Re: [Qemu-devel] [PATCH v2 2/6] tests/qapi-schema: Test for good feature lists in structs

[Kevin Wolf]

Am 24.05.2019 um 15:29 hat Markus Armbruster geschrieben:
> Let's add
> 
>    { 'command': 'test-features',
>      'data': { 'fs0': 'FeatureStruct0',
>                'fs1': 'FeatureStruct1',
>                'fs2': 'FeatureStruct2',
>                'fs3': 'FeatureStruct3',
>                'cfs1': 'CondFeatureStruct1',
>                'cfs2': 'CondFeatureStruct2',
>                'cfs3': 'CondFeatureStruct3' } }
> 
> because without it, the feature test cases won't generate introspection
> code.

Of course, like everything else you requested, I'll just do this to get
the series off my table, but I'm still curious: Where would
introspection code ever be generated for the test cases? I saw neither
test code that generates the source files nor reference output that it
would be compared against.

Kevin

Re: [Qemu-devel] [PATCH v2 2/6] tests/qapi-schema: Test for good feature lists in structs

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 24.05.2019 um 15:29 hat Markus Armbruster geschrieben:
>> Let's add
>> 
>>    { 'command': 'test-features',
>>      'data': { 'fs0': 'FeatureStruct0',
>>                'fs1': 'FeatureStruct1',
>>                'fs2': 'FeatureStruct2',
>>                'fs3': 'FeatureStruct3',
>>                'cfs1': 'CondFeatureStruct1',
>>                'cfs2': 'CondFeatureStruct2',
>>                'cfs3': 'CondFeatureStruct3' } }
>> 
>> because without it, the feature test cases won't generate introspection
>> code.
>
> Of course, like everything else you requested, I'll just do this to get
> the series off my table, but I'm still curious: Where would
> introspection code ever be generated for the test cases? I saw neither
> test code that generates the source files nor reference output that it
> would be compared against.

Asking me to explain why I want something done when you can't see it
yourself is much, much better than blindly implementing it.

Makefile.include feeds the two positive tests qapi-schema-test.json and
doc-good.json to qapi-gen.py.

The .o for the former's .c get linked into a bunch of tests via Make
variable $(test-qapi-obj-y).  One of them is test-qobject-input-visitor.
Its test case "/visitor/input/qapi-introspect" checks the generated
QObject conforms to the schema.

qapi-schema.json gets tested end-to-end instead: qmp-cmd-tests tests
query-qmp-schema.

Both tests only check schema conformance, they don't compare to expected
output.  Perhaps they should.  But I can still diff the generated
qmp-introspect.c manually, which I routinely do when messing with the
generator.

Makes sense?

Re: [Qemu-devel] [PATCH v2 2/6] tests/qapi-schema: Test for good feature lists in structs

[Kevin Wolf]

Am 03.06.2019 um 08:35 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 24.05.2019 um 15:29 hat Markus Armbruster geschrieben:
> >> Let's add
> >> 
> >>    { 'command': 'test-features',
> >>      'data': { 'fs0': 'FeatureStruct0',
> >>                'fs1': 'FeatureStruct1',
> >>                'fs2': 'FeatureStruct2',
> >>                'fs3': 'FeatureStruct3',
> >>                'cfs1': 'CondFeatureStruct1',
> >>                'cfs2': 'CondFeatureStruct2',
> >>                'cfs3': 'CondFeatureStruct3' } }
> >> 
> >> because without it, the feature test cases won't generate introspection
> >> code.
> >
> > Of course, like everything else you requested, I'll just do this to get
> > the series off my table, but I'm still curious: Where would
> > introspection code ever be generated for the test cases? I saw neither
> > test code that generates the source files nor reference output that it
> > would be compared against.
> 
> Asking me to explain why I want something done when you can't see it
> yourself is much, much better than blindly implementing it.

Well, adding a command can't hurt anyway, so doing both in parallel
seemed like a good option. :-)

> Makefile.include feeds the two positive tests qapi-schema-test.json and
> doc-good.json to qapi-gen.py.
> 
> The .o for the former's .c get linked into a bunch of tests via Make
> variable $(test-qapi-obj-y).  One of them is test-qobject-input-visitor.
> Its test case "/visitor/input/qapi-introspect" checks the generated
> QObject conforms to the schema.
> 
> qapi-schema.json gets tested end-to-end instead: qmp-cmd-tests tests
> query-qmp-schema.
> 
> Both tests only check schema conformance, they don't compare to expected
> output.  Perhaps they should.  But I can still diff the generated
> qmp-introspect.c manually, which I routinely do when messing with the
> generator.
> 
> Makes sense?

Ah, so basically we don't fully check the correctness of the
introspection output, but just that two different generated files are
consistent with each other. Yes, makes sense.

I just didn't realise that the generated code is actually compiled and
linked to some other test cases. My expectation was the schema testing
was only in tests/qapi-schema/.

Kevin

[Qemu-devel] [PATCH v2 3/6] tests/qapi-schema: Error case tests for features in structs

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qapi-schema/features-bad-type.json       | 3 +++
 tests/qapi-schema/features-duplicate-name.json | 3 +++
 tests/qapi-schema/features-missing-name.json   | 3 +++
 tests/qapi-schema/features-name-bad-type.json  | 3 +++
 tests/qapi-schema/features-no-list.json        | 3 +++
 tests/qapi-schema/features-unknown-key.json    | 3 +++
 tests/Makefile.include                         | 6 ++++++
 tests/qapi-schema/features-bad-type.err        | 1 +
 tests/qapi-schema/features-bad-type.exit       | 1 +
 tests/qapi-schema/features-bad-type.out        | 0
 tests/qapi-schema/features-duplicate-name.err  | 1 +
 tests/qapi-schema/features-duplicate-name.exit | 1 +
 tests/qapi-schema/features-duplicate-name.out  | 0
 tests/qapi-schema/features-missing-name.err    | 1 +
 tests/qapi-schema/features-missing-name.exit   | 1 +
 tests/qapi-schema/features-missing-name.out    | 0
 tests/qapi-schema/features-name-bad-type.err   | 1 +
 tests/qapi-schema/features-name-bad-type.exit  | 1 +
 tests/qapi-schema/features-name-bad-type.out   | 0
 tests/qapi-schema/features-no-list.err         | 1 +
 tests/qapi-schema/features-no-list.exit        | 1 +
 tests/qapi-schema/features-no-list.out         | 0
 tests/qapi-schema/features-unknown-key.err     | 2 ++
 tests/qapi-schema/features-unknown-key.exit    | 1 +
 tests/qapi-schema/features-unknown-key.out     | 0
 25 files changed, 37 insertions(+)
 create mode 100644 tests/qapi-schema/features-bad-type.json
 create mode 100644 tests/qapi-schema/features-duplicate-name.json
 create mode 100644 tests/qapi-schema/features-missing-name.json
 create mode 100644 tests/qapi-schema/features-name-bad-type.json
 create mode 100644 tests/qapi-schema/features-no-list.json
 create mode 100644 tests/qapi-schema/features-unknown-key.json
 create mode 100644 tests/qapi-schema/features-bad-type.err
 create mode 100644 tests/qapi-schema/features-bad-type.exit
 create mode 100644 tests/qapi-schema/features-bad-type.out
 create mode 100644 tests/qapi-schema/features-duplicate-name.err
 create mode 100644 tests/qapi-schema/features-duplicate-name.exit
 create mode 100644 tests/qapi-schema/features-duplicate-name.out
 create mode 100644 tests/qapi-schema/features-missing-name.err
 create mode 100644 tests/qapi-schema/features-missing-name.exit
 create mode 100644 tests/qapi-schema/features-missing-name.out
 create mode 100644 tests/qapi-schema/features-name-bad-type.err
 create mode 100644 tests/qapi-schema/features-name-bad-type.exit
 create mode 100644 tests/qapi-schema/features-name-bad-type.out
 create mode 100644 tests/qapi-schema/features-no-list.err
 create mode 100644 tests/qapi-schema/features-no-list.exit
 create mode 100644 tests/qapi-schema/features-no-list.out
 create mode 100644 tests/qapi-schema/features-unknown-key.err
 create mode 100644 tests/qapi-schema/features-unknown-key.exit
 create mode 100644 tests/qapi-schema/features-unknown-key.out

diff --git a/tests/qapi-schema/features-bad-type.json b/tests/qapi-schema/features-bad-type.json
new file mode 100644
index 0000000000..57db5540e7
--- /dev/null
+++ b/tests/qapi-schema/features-bad-type.json
@@ -0,0 +1,3 @@
+{ 'struct': 'FeatureStruct0',
+  'data': { 'foo': 'int' },
+  'features': [ [ 'a feature cannot be an array' ] ] }
diff --git a/tests/qapi-schema/features-duplicate-name.json b/tests/qapi-schema/features-duplicate-name.json
new file mode 100644
index 0000000000..29358e6220
--- /dev/null
+++ b/tests/qapi-schema/features-duplicate-name.json
@@ -0,0 +1,3 @@
+{ 'struct': 'FeatureStruct0',
+  'data': { 'foo': 'int' },
+  'features': [ 'foo', 'bar', 'foo' ] }
diff --git a/tests/qapi-schema/features-missing-name.json b/tests/qapi-schema/features-missing-name.json
new file mode 100644
index 0000000000..2314f97c00
--- /dev/null
+++ b/tests/qapi-schema/features-missing-name.json
@@ -0,0 +1,3 @@
+{ 'struct': 'FeatureStruct0',
+  'data': { 'foo': 'int' },
+  'features': [ { 'if': 'defined(NAMELESS_FEATURES)' } ] }
diff --git a/tests/qapi-schema/features-name-bad-type.json b/tests/qapi-schema/features-name-bad-type.json
new file mode 100644
index 0000000000..b07139978a
--- /dev/null
+++ b/tests/qapi-schema/features-name-bad-type.json
@@ -0,0 +1,3 @@
+{ 'struct': 'FeatureStruct0',
+  'data': { 'foo': 'int' },
+  'features': [ { 'name': { 'feature-type': 'object' } } ] }
diff --git a/tests/qapi-schema/features-no-list.json b/tests/qapi-schema/features-no-list.json
new file mode 100644
index 0000000000..9484fd94fc
--- /dev/null
+++ b/tests/qapi-schema/features-no-list.json
@@ -0,0 +1,3 @@
+{ 'struct': 'FeatureStruct0',
+  'data': { 'foo': 'int' },
+  'features': 'bar' }
diff --git a/tests/qapi-schema/features-unknown-key.json b/tests/qapi-schema/features-unknown-key.json
new file mode 100644
index 0000000000..134df3b503
--- /dev/null
+++ b/tests/qapi-schema/features-unknown-key.json
@@ -0,0 +1,3 @@
+{ 'struct': 'FeatureStruct0',
+  'data': { 'foo': 'int' },
+  'features': [ { 'name': 'bar', 'colour': 'red' } ] }
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 60de085ee1..9284f82ec8 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -377,6 +377,12 @@ qapi-schema += event-boxed-empty.json
 qapi-schema += event-case.json
 qapi-schema += event-member-invalid-dict.json
 qapi-schema += event-nest-struct.json
+qapi-schema += features-bad-type.json
+qapi-schema += features-duplicate-name.json
+qapi-schema += features-missing-name.json
+qapi-schema += features-name-bad-type.json
+qapi-schema += features-no-list.json
+qapi-schema += features-unknown-key.json
 qapi-schema += flat-union-array-branch.json
 qapi-schema += flat-union-bad-base.json
 qapi-schema += flat-union-bad-discriminator.json
diff --git a/tests/qapi-schema/features-bad-type.err b/tests/qapi-schema/features-bad-type.err
new file mode 100644
index 0000000000..5fb95c2f90
--- /dev/null
+++ b/tests/qapi-schema/features-bad-type.err
@@ -0,0 +1 @@
+tests/qapi-schema/features-bad-type.json:1: Feature of struct FeatureStruct0 requires a string name
diff --git a/tests/qapi-schema/features-bad-type.exit b/tests/qapi-schema/features-bad-type.exit
new file mode 100644
index 0000000000..d00491fd7e
--- /dev/null
+++ b/tests/qapi-schema/features-bad-type.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/features-bad-type.out b/tests/qapi-schema/features-bad-type.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/features-duplicate-name.err b/tests/qapi-schema/features-duplicate-name.err
new file mode 100644
index 0000000000..c0a4cccae6
--- /dev/null
+++ b/tests/qapi-schema/features-duplicate-name.err
@@ -0,0 +1 @@
+tests/qapi-schema/features-duplicate-name.json:1: 'foo' (feature of FeatureStruct0) collides with 'foo' (feature of FeatureStruct0)
diff --git a/tests/qapi-schema/features-duplicate-name.exit b/tests/qapi-schema/features-duplicate-name.exit
new file mode 100644
index 0000000000..d00491fd7e
--- /dev/null
+++ b/tests/qapi-schema/features-duplicate-name.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/features-duplicate-name.out b/tests/qapi-schema/features-duplicate-name.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/features-missing-name.err b/tests/qapi-schema/features-missing-name.err
new file mode 100644
index 0000000000..4f1d2715aa
--- /dev/null
+++ b/tests/qapi-schema/features-missing-name.err
@@ -0,0 +1 @@
+tests/qapi-schema/features-missing-name.json:1: Key 'name' is missing from feature of struct FeatureStruct0
diff --git a/tests/qapi-schema/features-missing-name.exit b/tests/qapi-schema/features-missing-name.exit
new file mode 100644
index 0000000000..d00491fd7e
--- /dev/null
+++ b/tests/qapi-schema/features-missing-name.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/features-missing-name.out b/tests/qapi-schema/features-missing-name.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/features-name-bad-type.err b/tests/qapi-schema/features-name-bad-type.err
new file mode 100644
index 0000000000..8a3eecb972
--- /dev/null
+++ b/tests/qapi-schema/features-name-bad-type.err
@@ -0,0 +1 @@
+tests/qapi-schema/features-name-bad-type.json:1: Feature of struct FeatureStruct0 requires a string name
diff --git a/tests/qapi-schema/features-name-bad-type.exit b/tests/qapi-schema/features-name-bad-type.exit
new file mode 100644
index 0000000000..d00491fd7e
--- /dev/null
+++ b/tests/qapi-schema/features-name-bad-type.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/features-name-bad-type.out b/tests/qapi-schema/features-name-bad-type.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/features-no-list.err b/tests/qapi-schema/features-no-list.err
new file mode 100644
index 0000000000..61ed68612b
--- /dev/null
+++ b/tests/qapi-schema/features-no-list.err
@@ -0,0 +1 @@
+tests/qapi-schema/features-no-list.json:1: Struct 'FeatureStruct0' requires an array for 'features'
diff --git a/tests/qapi-schema/features-no-list.exit b/tests/qapi-schema/features-no-list.exit
new file mode 100644
index 0000000000..d00491fd7e
--- /dev/null
+++ b/tests/qapi-schema/features-no-list.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/features-no-list.out b/tests/qapi-schema/features-no-list.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/features-unknown-key.err b/tests/qapi-schema/features-unknown-key.err
new file mode 100644
index 0000000000..a1d693030d
--- /dev/null
+++ b/tests/qapi-schema/features-unknown-key.err
@@ -0,0 +1,2 @@
+tests/qapi-schema/features-unknown-key.json:1: Unknown key 'colour' in feature of struct FeatureStruct0
+Valid keys are 'if', 'name'.
diff --git a/tests/qapi-schema/features-unknown-key.exit b/tests/qapi-schema/features-unknown-key.exit
new file mode 100644
index 0000000000..d00491fd7e
--- /dev/null
+++ b/tests/qapi-schema/features-unknown-key.exit
@@ -0,0 +1 @@
+1
diff --git a/tests/qapi-schema/features-unknown-key.out b/tests/qapi-schema/features-unknown-key.out
new file mode 100644
index 0000000000..e69de29bb2
-- 
2.20.1

[Qemu-devel] [PATCH v2 4/6] qapi: Disentangle QAPIDoc code

[Kevin Wolf]

Documentation comment follow a certain structure: First, we have a text
with a general description (called QAPIDoc.body). After this,
descriptions of the arguments follow. Finally, we have part that
contains various named sections.

The code doesn't show this structure but just checks the right side
conditions so it happens to do the right set of things in the right
phase. This is hard to follow, and adding support for documentation of
features would be even harder.

This restructures the code so that the three parts are clearly
separated. The code becomes a bit longer, but easier to follow.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 scripts/qapi/common.py | 107 ++++++++++++++++++++++++++++++++---------
 1 file changed, 83 insertions(+), 24 deletions(-)

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 71944e2e30..1d0f4847db 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -120,6 +120,27 @@ class QAPIDoc(object):
         def connect(self, member):
             self.member = member
 
+    class SymbolPart:
+        """
+        Describes which part of the documentation we're parsing right now.
+
+        BODY means that we're ready to process freeform text into self.body. A
+        symbol name is only allowed if no other text was parsed yet. It is
+        interpreted as the symbol name that describes the currently documented
+        object. On getting the second symbol name, we proceed to ARGS.
+
+        ARGS means that we're parsing the arguments section. Any symbol name is
+        interpreted as an argument and an ArgSection is created for it.
+
+        VARIOUS is the final part where freeform sections may appear. This
+        includes named sections such as "Return:" as well as unnamed
+        paragraphs. No symbols are allowed any more in this part.
+        """
+        # Can't make it a subclass of Enum because of Python 2
+        BODY = 0
+        ARGS = 1
+        VARIOUS = 2
+
     def __init__(self, parser, info):
         # self._parser is used to report errors with QAPIParseError.  The
         # resulting error position depends on the state of the parser.
@@ -135,6 +156,7 @@ class QAPIDoc(object):
         self.sections = []
         # the current section
         self._section = self.body
+        self._part = QAPIDoc.SymbolPart.BODY
 
     def has_section(self, name):
         """Return True if we have a section with this name."""
@@ -154,37 +176,84 @@ class QAPIDoc(object):
             raise QAPIParseError(self._parser, "Missing space after #")
         line = line[1:]
 
+        if self._part == QAPIDoc.SymbolPart.BODY:
+            self._append_body_line(line)
+        elif self._part == QAPIDoc.SymbolPart.ARGS:
+            self._append_args_line(line)
+        elif self._part == QAPIDoc.SymbolPart.VARIOUS:
+            self._append_various_line(line)
+        else:
+            assert False
+
+
+    def end_comment(self):
+        self._end_section()
+
+    def _check_named_section(self, line, name):
+        if name in ('Returns:', 'Since:',
+                    # those are often singular or plural
+                    'Note:', 'Notes:',
+                    'Example:', 'Examples:',
+                    'TODO:'):
+            self._part = QAPIDoc.SymbolPart.VARIOUS
+            return True
+        return False
+
+    def _append_body_line(self, line):
+        name = line.split(' ', 1)[0]
         # FIXME not nice: things like '#  @foo:' and '# @foo: ' aren't
         # recognized, and get silently treated as ordinary text
-        if self.symbol:
-            self._append_symbol_line(line)
-        elif not self.body.text and line.startswith('@'):
+        if not self.symbol and not self.body.text and line.startswith('@'):
             if not line.endswith(':'):
                 raise QAPIParseError(self._parser, "Line should end with :")
             self.symbol = line[1:-1]
             # FIXME invalid names other than the empty string aren't flagged
             if not self.symbol:
                 raise QAPIParseError(self._parser, "Invalid name")
+        elif self.symbol:
+            # We already know that we document some symbol
+            if name.startswith('@') and name.endswith(':'):
+                self._part = QAPIDoc.SymbolPart.ARGS
+                self._append_args_line(line)
+            elif self.symbol and self._check_named_section(line, name):
+                self._append_various_line(line)
+            else:
+                self._append_freeform(line.strip())
         else:
-            self._append_freeform(line)
-
-    def end_comment(self):
-        self._end_section()
+            # This is free-form documentation without a symbol
+            self._append_freeform(line.strip())
 
-    def _append_symbol_line(self, line):
+    def _append_args_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])
-        elif name in ('Returns:', 'Since:',
-                      # those are often singular or plural
-                      'Note:', 'Notes:',
-                      'Example:', 'Examples:',
-                      'TODO:'):
+        elif self._check_named_section(line, name):
+            return self._append_various_line(line)
+        elif (self._section.text.endswith('\n\n')
+              and line and not line[0].isspace()):
+            self._start_section()
+            self._part = QAPIDoc.SymbolPart.VARIOUS
+            return self._append_various_line(line)
+
+        self._append_freeform(line.strip())
+
+    def _append_various_line(self, line):
+        name = line.split(' ', 1)[0]
+
+        if name.startswith('@') and name.endswith(':'):
+            raise QAPIParseError(self._parser,
+                                 "'%s' can't follow '%s' section"
+                                 % (name, self.sections[0].name))
+        elif self._check_named_section(line, name):
             line = line[len(name)+1:]
             self._start_section(name[:-1])
 
+        if (not self._section.name or
+            not self._section.name.startswith('Example')):
+            line = line.strip()
+
         self._append_freeform(line)
 
     def _start_args_section(self, name):
@@ -194,10 +263,7 @@ class QAPIDoc(object):
         if name in self.args:
             raise QAPIParseError(self._parser,
                                  "'%s' parameter name duplicated" % name)
-        if self.sections:
-            raise QAPIParseError(self._parser,
-                                 "'@%s:' can't follow '%s' section"
-                                 % (name, self.sections[0].name))
+        assert not self.sections
         self._end_section()
         self._section = QAPIDoc.ArgSection(name)
         self.args[name] = self._section
@@ -219,13 +285,6 @@ class QAPIDoc(object):
             self._section = None
 
     def _append_freeform(self, line):
-        in_arg = isinstance(self._section, QAPIDoc.ArgSection)
-        if (in_arg and self._section.text.endswith('\n\n')
-                and line and not line[0].isspace()):
-            self._start_section()
-        if (in_arg or not self._section.name
-                or not self._section.name.startswith('Example')):
-            line = line.strip()
         match = re.match(r'(@\S+:)', line)
         if match:
             raise QAPIParseError(self._parser,
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 4/6] qapi: Disentangle QAPIDoc code

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Documentation comment follow a certain structure: First, we have a text
> with a general description (called QAPIDoc.body). After this,
> descriptions of the arguments follow. Finally, we have part that
> contains various named sections.
>
> The code doesn't show this structure but just checks the right side
> conditions so it happens to do the right set of things in the right

What are "side conditions"?

> phase. This is hard to follow, and adding support for documentation of
> features would be even harder.
>
> This restructures the code so that the three parts are clearly
> separated. The code becomes a bit longer, but easier to follow.

Recommend to mention that output remains unchanged.

>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  scripts/qapi/common.py | 107 ++++++++++++++++++++++++++++++++---------
>  1 file changed, 83 insertions(+), 24 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 71944e2e30..1d0f4847db 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -120,6 +120,27 @@ class QAPIDoc(object):
>          def connect(self, member):
>              self.member = member
>  
> +    class SymbolPart:
> +        """
> +        Describes which part of the documentation we're parsing right now.

So SymbolPart is a part of the documentation.  Shouldn't it be named
DocPart then?

> +
> +        BODY means that we're ready to process freeform text into self.body. A

s/freeform/free-form/

> +        symbol name is only allowed if no other text was parsed yet. It is

Start your sentences with a capital letter.

> +        interpreted as the symbol name that describes the currently documented
> +        object. On getting the second symbol name, we proceed to ARGS.
> +
> +        ARGS means that we're parsing the arguments section. Any symbol name is
> +        interpreted as an argument and an ArgSection is created for it.
> +
> +        VARIOUS is the final part where freeform sections may appear. This
> +        includes named sections such as "Return:" as well as unnamed
> +        paragraphs. No symbols are allowed any more in this part.

s/any more/anymore/

Suggest "Symbols are not allowed anymore".

Only expression documentation blocks have these parts.  Free-form
documentation blocks don't.  Peeking ahead at the parent class's code:
we make the complete free-form doc block a BODY part, which is okay.

> +        """

Considering the dearth of doc strings in this code, criticizing yours
makes me feel bad, but here goes anyway.  This class is not about
parsing.  Pretty much all of the doc string really belongs to the parent
class, or maybe to its .append().  It may well also belong to
qapi-code-gen.txt section "Expression documentation".

Speaking of that section: it could use some love, to put it charitably.
Mind, I'm not asking you to give it your love ;)

Let me try to write a doc string that actually belongs here:

        """
        A documentation part.

        Expression documentation blocks consist of
        * a BODY part: first line naming the expression, plus an
          optional overview
        * an ARGS part: description of each argument (for commands and
          events) or member (for structs, unions and alternates),
        * a VARIOUS part: optional tagged sections.

        Free-form documentation blocks consist only of a BODY part.
        """

Could throw in a pointer to qapi-code-gen.txt.

This loses (useful!) documentation on how we interpret "symbol names".
Should move to the code that actually deals with them.

> +        # Can't make it a subclass of Enum because of Python 2

Thanks for documenting Python 2 contortions!  Let's phrase it as a TODO
comment.

> +        BODY = 0

Any particular reason for 0?

As far as I can tell, Python enum values commonly start with 1, to make
them all true.

> +        ARGS = 1
> +        VARIOUS = 2
> +
>      def __init__(self, parser, info):
>          # self._parser is used to report errors with QAPIParseError.  The
>          # resulting error position depends on the state of the parser.
> @@ -135,6 +156,7 @@ class QAPIDoc(object):
>          self.sections = []
>          # the current section
>          self._section = self.body
> +        self._part = QAPIDoc.SymbolPart.BODY

The right hand side is tiresome, but I don't have better ideas.

>  
>      def has_section(self, name):
>          """Return True if we have a section with this name."""
> @@ -154,37 +176,84 @@ class QAPIDoc(object):
       def append(self, line):
           """Parse a comment line and add it to the documentation."""
           line = line[1:]
           if not line:
               self._append_freeform(line)
               return

           if line[0] != ' ':
>              raise QAPIParseError(self._parser, "Missing space after #")
>          line = line[1:]
>  
> +        if self._part == QAPIDoc.SymbolPart.BODY:
> +            self._append_body_line(line)
> +        elif self._part == QAPIDoc.SymbolPart.ARGS:
> +            self._append_args_line(line)
> +        elif self._part == QAPIDoc.SymbolPart.VARIOUS:
> +            self._append_various_line(line)
> +        else:
> +            assert False

Hmm.  As far as I can tell, this what we use ._part for.  All other
occurences assign to it.

If you replace

    self._part = QAPIDoc.SymbolPart.BODY

by

    self._append_line = self._append_body_line

and so forth, then the whole conditional shrinks to

    self._append_line(line)

and we don't have to muck around with enums.

> +
> +

pycodestyle-3 gripes:

    scripts/qapi/common.py:196:5: E303 too many blank lines (2)

> +    def end_comment(self):
> +        self._end_section()
> +
> +    def _check_named_section(self, line, name):
> +        if name in ('Returns:', 'Since:',
> +                    # those are often singular or plural
> +                    'Note:', 'Notes:',
> +                    'Example:', 'Examples:',
> +                    'TODO:'):
> +            self._part = QAPIDoc.SymbolPart.VARIOUS

Hiding such a side effect in a _check_FOO() function isn't so nice, but
considering the code you're cleaning up, you got a fairly generous "not
so nice" allowance to spend.  No need to do anything.

> +            return True
> +        return False

@line is unused.

> +
> +    def _append_body_line(self, line):
> +        name = line.split(' ', 1)[0]
>          # FIXME not nice: things like '#  @foo:' and '# @foo: ' aren't
>          # recognized, and get silently treated as ordinary text
> -        if self.symbol:
> -            self._append_symbol_line(line)
> -        elif not self.body.text and line.startswith('@'):
> +        if not self.symbol and not self.body.text and line.startswith('@'):
>              if not line.endswith(':'):
>                  raise QAPIParseError(self._parser, "Line should end with :")
>              self.symbol = line[1:-1]
>              # FIXME invalid names other than the empty string aren't flagged
>              if not self.symbol:
>                  raise QAPIParseError(self._parser, "Invalid name")
> +        elif self.symbol:
> +            # We already know that we document some symbol
> +            if name.startswith('@') and name.endswith(':'):
> +                self._part = QAPIDoc.SymbolPart.ARGS
> +                self._append_args_line(line)
> +            elif self.symbol and self._check_named_section(line, name):
> +                self._append_various_line(line)
> +            else:
> +                self._append_freeform(line.strip())
>          else:
> -            self._append_freeform(line)
> -
> -    def end_comment(self):
> -        self._end_section()
> +            # This is free-form documentation without a symbol
> +            self._append_freeform(line.strip())
>  
> -    def _append_symbol_line(self, line):
> +    def _append_args_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])
> -        elif name in ('Returns:', 'Since:',
> -                      # those are often singular or plural
> -                      'Note:', 'Notes:',
> -                      'Example:', 'Examples:',
> -                      'TODO:'):
> +        elif self._check_named_section(line, name):
> +            return self._append_various_line(line)

Here you return something, and...

> +        elif (self._section.text.endswith('\n\n')
> +              and line and not line[0].isspace()):
> +            self._start_section()
> +            self._part = QAPIDoc.SymbolPart.VARIOUS
> +            return self._append_various_line(line)

... also here, but ...

> +
> +        self._append_freeform(line.strip())

... not here.

> +
> +    def _append_various_line(self, line):
> +        name = line.split(' ', 1)[0]
> +
> +        if name.startswith('@') and name.endswith(':'):
> +            raise QAPIParseError(self._parser,
> +                                 "'%s' can't follow '%s' section"
> +                                 % (name, self.sections[0].name))
> +        elif self._check_named_section(line, name):
>              line = line[len(name)+1:]
>              self._start_section(name[:-1])
>  
> +        if (not self._section.name or
> +            not self._section.name.startswith('Example')):

pycodestyle-3 gripes:

    scripts/qapi/common.py:283:13: E129 visually indented line with same indent as next logical line

Fix like this:

           if (not self._section.name
                   or not self._section.name.startswith('Example')):

> +            line = line.strip()
> +
>          self._append_freeform(line)
>  
>      def _start_args_section(self, name):
> @@ -194,10 +263,7 @@ class QAPIDoc(object):
>          if name in self.args:
>              raise QAPIParseError(self._parser,
>                                   "'%s' parameter name duplicated" % name)
> -        if self.sections:
> -            raise QAPIParseError(self._parser,
> -                                 "'@%s:' can't follow '%s' section"
> -                                 % (name, self.sections[0].name))
> +        assert not self.sections
>          self._end_section()
>          self._section = QAPIDoc.ArgSection(name)
>          self.args[name] = self._section
> @@ -219,13 +285,6 @@ class QAPIDoc(object):
>              self._section = None
>  
>      def _append_freeform(self, line):
> -        in_arg = isinstance(self._section, QAPIDoc.ArgSection)
> -        if (in_arg and self._section.text.endswith('\n\n')
> -                and line and not line[0].isspace()):
> -            self._start_section()
> -        if (in_arg or not self._section.name
> -                or not self._section.name.startswith('Example')):
> -            line = line.strip()
>          match = re.match(r'(@\S+:)', line)
>          if match:
>              raise QAPIParseError(self._parser,

The patch is hard to read (not your fault).  I mostly reviewed the
result instead.

Re: [Qemu-devel] [PATCH v2 4/6] qapi: Disentangle QAPIDoc code

[Kevin Wolf]

Am 24.05.2019 um 18:11 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Documentation comment follow a certain structure: First, we have a text
> > with a general description (called QAPIDoc.body). After this,
> > descriptions of the arguments follow. Finally, we have part that
> > contains various named sections.
> >
> > The code doesn't show this structure but just checks the right side
> > conditions so it happens to do the right set of things in the right
> 
> What are "side conditions"?
> 
> > phase. This is hard to follow, and adding support for documentation of
> > features would be even harder.
> >
> > This restructures the code so that the three parts are clearly
> > separated. The code becomes a bit longer, but easier to follow.
> 
> Recommend to mention that output remains unchanged.
> 
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  scripts/qapi/common.py | 107 ++++++++++++++++++++++++++++++++---------
> >  1 file changed, 83 insertions(+), 24 deletions(-)
> >
> > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> > index 71944e2e30..1d0f4847db 100644
> > --- a/scripts/qapi/common.py
> > +++ b/scripts/qapi/common.py
> > @@ -120,6 +120,27 @@ class QAPIDoc(object):
> >          def connect(self, member):
> >              self.member = member
> >  
> > +    class SymbolPart:
> > +        """
> > +        Describes which part of the documentation we're parsing right now.
> 
> So SymbolPart is a part of the documentation.  Shouldn't it be named
> DocPart then?

That's a better name. I was stuck in the old code (which was concerned
about what a symbol name means at which point) rather than thinking
about high-level concepts.

> > +
> > +        BODY means that we're ready to process freeform text into self.body. A
> 
> s/freeform/free-form/

Both are valid spellings and I generally don't expect correct spellings
to be corrected, but arguably "free-form" is more standard. I'll change
it. (If we were consistent, the method should have been named
_append_free_form rather than _append_freeform originally...)

> > +        symbol name is only allowed if no other text was parsed yet. It is
> 
> Start your sentences with a capital letter.

I would gladly correct a sentence not starting with a capital letter if
I could see any. The quoted sentence starts with a capital "A" in the
previous line.

> > +        interpreted as the symbol name that describes the currently documented
> > +        object. On getting the second symbol name, we proceed to ARGS.
> > +
> > +        ARGS means that we're parsing the arguments section. Any symbol name is
> > +        interpreted as an argument and an ArgSection is created for it.
> > +
> > +        VARIOUS is the final part where freeform sections may appear. This
> > +        includes named sections such as "Return:" as well as unnamed
> > +        paragraphs. No symbols are allowed any more in this part.
> 
> s/any more/anymore/

Again both are valid, but this time, "any more" is the more standard
spelling.

> > +        # Can't make it a subclass of Enum because of Python 2
> 
> Thanks for documenting Python 2 contortions!  Let's phrase it as a TODO
> comment.
> 
> > +        BODY = 0
> 
> Any particular reason for 0?
> 
> As far as I can tell, Python enum values commonly start with 1, to make
> them all true.

If you use enums in a boolean context, you're doing something wrong
anyway. *shrug*

I'll change it, it's consistent with real Enum classes where the values
becomes non-integer objects (which therefore evaluate as True in boolean
contexts).

> > +        ARGS = 1
> > +        VARIOUS = 2
> > +
> >      def __init__(self, parser, info):
> >          # self._parser is used to report errors with QAPIParseError.  The
> >          # resulting error position depends on the state of the parser.
> > @@ -135,6 +156,7 @@ class QAPIDoc(object):
> >          self.sections = []
> >          # the current section
> >          self._section = self.body
> > +        self._part = QAPIDoc.SymbolPart.BODY
> 
> The right hand side is tiresome, but I don't have better ideas.

This is just what Python enums look like... I could move the class
outside of QAPIDoc to save that part of the prefix, but I'd prefer not
to.

> >  
> >      def has_section(self, name):
> >          """Return True if we have a section with this name."""
> > @@ -154,37 +176,84 @@ class QAPIDoc(object):
>        def append(self, line):
>            """Parse a comment line and add it to the documentation."""
>            line = line[1:]
>            if not line:
>                self._append_freeform(line)
>                return
> 
>            if line[0] != ' ':
> >              raise QAPIParseError(self._parser, "Missing space after #")
> >          line = line[1:]
> >  
> > +        if self._part == QAPIDoc.SymbolPart.BODY:
> > +            self._append_body_line(line)
> > +        elif self._part == QAPIDoc.SymbolPart.ARGS:
> > +            self._append_args_line(line)
> > +        elif self._part == QAPIDoc.SymbolPart.VARIOUS:
> > +            self._append_various_line(line)
> > +        else:
> > +            assert False
> 
> Hmm.  As far as I can tell, this what we use ._part for.  All other
> occurences assign to it.
> 
> If you replace
> 
>     self._part = QAPIDoc.SymbolPart.BODY
> 
> by
> 
>     self._append_line = self._append_body_line
> 
> and so forth, then the whole conditional shrinks to
> 
>     self._append_line(line)
> 
> and we don't have to muck around with enums.

I could just have added a boolean that decides whether a symbol is an
argument or a feature. That would have been a minimal hack that
wouldn't involve any enums.

I intentionally decided not to do that because the whole structure of
the parser was horribly confusing to me and I felt that introducing a
clear state machine would improve its legibility a lot. I still think
that this is what it did.

If you don't like a proper state machine, I can do that bool thing. I
don't think throwing in function pointers would be very helpful for
readers, so we'd get a major code change for no gain.

Kevin

Re: [Qemu-devel] [PATCH v2 4/6] qapi: Disentangle QAPIDoc code

[Markus Armbruster]

Cc: Eric for English language advice.

Kevin Wolf <kwolf@redhat.com> writes:

> Am 24.05.2019 um 18:11 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Documentation comment follow a certain structure: First, we have a text
>> > with a general description (called QAPIDoc.body). After this,
>> > descriptions of the arguments follow. Finally, we have part that
>> > contains various named sections.
>> >
>> > The code doesn't show this structure but just checks the right side
>> > conditions so it happens to do the right set of things in the right
>> 
>> What are "side conditions"?
>> 
>> > phase. This is hard to follow, and adding support for documentation of
>> > features would be even harder.
>> >
>> > This restructures the code so that the three parts are clearly
>> > separated. The code becomes a bit longer, but easier to follow.
>> 
>> Recommend to mention that output remains unchanged.
>> 
>> >
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> > ---
>> >  scripts/qapi/common.py | 107 ++++++++++++++++++++++++++++++++---------
>> >  1 file changed, 83 insertions(+), 24 deletions(-)
>> >
>> > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>> > index 71944e2e30..1d0f4847db 100644
>> > --- a/scripts/qapi/common.py
>> > +++ b/scripts/qapi/common.py
>> > @@ -120,6 +120,27 @@ class QAPIDoc(object):
>> >          def connect(self, member):
>> >              self.member = member
>> >  
>> > +    class SymbolPart:
>> > +        """
>> > +        Describes which part of the documentation we're parsing right now.
>> 
>> So SymbolPart is a part of the documentation.  Shouldn't it be named
>> DocPart then?
>
> That's a better name. I was stuck in the old code (which was concerned
> about what a symbol name means at which point) rather than thinking
> about high-level concepts.
>
>> > +
>> > +        BODY means that we're ready to process freeform text into self.body. A
>> 
>> s/freeform/free-form/
>
> Both are valid spellings and I generally don't expect correct spellings
> to be corrected, but arguably "free-form" is more standard. I'll change
> it.

The error message and qapi-code-gen.txt consistently spell it free-form.
I prefer consistent spelling, not least for greppability.

>     (If we were consistent, the method should have been named
> _append_free_form rather than _append_freeform originally...)

Yes.

>> > +        symbol name is only allowed if no other text was parsed yet. It is
>> 
>> Start your sentences with a capital letter.
>
> I would gladly correct a sentence not starting with a capital letter if
> I could see any. The quoted sentence starts with a capital "A" in the
> previous line.

My mistake, I overlooked the "A" at the end of the line.

>> > +        interpreted as the symbol name that describes the currently documented
>> > +        object. On getting the second symbol name, we proceed to ARGS.
>> > +
>> > +        ARGS means that we're parsing the arguments section. Any symbol name is
>> > +        interpreted as an argument and an ArgSection is created for it.
>> > +
>> > +        VARIOUS is the final part where freeform sections may appear. This
>> > +        includes named sections such as "Return:" as well as unnamed
>> > +        paragraphs. No symbols are allowed any more in this part.
>> 
>> s/any more/anymore/
>
> Again both are valid, but this time, "any more" is the more standard
> spelling.

Eric, what's your take?

>> > +        # Can't make it a subclass of Enum because of Python 2
>> 
>> Thanks for documenting Python 2 contortions!  Let's phrase it as a TODO
>> comment.
>> 
>> > +        BODY = 0
>> 
>> Any particular reason for 0?
>> 
>> As far as I can tell, Python enum values commonly start with 1, to make
>> them all true.
>
> If you use enums in a boolean context, you're doing something wrong
> anyway. *shrug*

No argument.  But...

> I'll change it, it's consistent with real Enum classes where the values
> becomes non-integer objects (which therefore evaluate as True in boolean
> contexts).

... consistency with real Enum costs us nothing, so let's do it.

>> > +        ARGS = 1
>> > +        VARIOUS = 2
>> > +
>> >      def __init__(self, parser, info):
>> >          # self._parser is used to report errors with QAPIParseError.  The
>> >          # resulting error position depends on the state of the parser.
>> > @@ -135,6 +156,7 @@ class QAPIDoc(object):
>> >          self.sections = []
>> >          # the current section
>> >          self._section = self.body
>> > +        self._part = QAPIDoc.SymbolPart.BODY
>> 
>> The right hand side is tiresome, but I don't have better ideas.
>
> This is just what Python enums look like... I could move the class
> outside of QAPIDoc to save that part of the prefix, but I'd prefer not
> to.

It's okay.

>> >  
>> >      def has_section(self, name):
>> >          """Return True if we have a section with this name."""
>> > @@ -154,37 +176,84 @@ class QAPIDoc(object):
>>        def append(self, line):
>>            """Parse a comment line and add it to the documentation."""
>>            line = line[1:]
>>            if not line:
>>                self._append_freeform(line)
>>                return
>> 
>>            if line[0] != ' ':
>> >              raise QAPIParseError(self._parser, "Missing space after #")
>> >          line = line[1:]
>> >  
>> > +        if self._part == QAPIDoc.SymbolPart.BODY:
>> > +            self._append_body_line(line)
>> > +        elif self._part == QAPIDoc.SymbolPart.ARGS:
>> > +            self._append_args_line(line)
>> > +        elif self._part == QAPIDoc.SymbolPart.VARIOUS:
>> > +            self._append_various_line(line)
>> > +        else:
>> > +            assert False
>> 
>> Hmm.  As far as I can tell, this what we use ._part for.  All other
>> occurences assign to it.
>> 
>> If you replace
>> 
>>     self._part = QAPIDoc.SymbolPart.BODY
>> 
>> by
>> 
>>     self._append_line = self._append_body_line
>> 
>> and so forth, then the whole conditional shrinks to
>> 
>>     self._append_line(line)
>> 
>> and we don't have to muck around with enums.
>
> I could just have added a boolean that decides whether a symbol is an
> argument or a feature. That would have been a minimal hack that
> wouldn't involve any enums.
>
> I intentionally decided not to do that because the whole structure of
> the parser was horribly confusing to me

Not just to you.

>                                         and I felt that introducing a
> clear state machine would improve its legibility a lot. I still think
> that this is what it did.
>
> If you don't like a proper state machine, I can do that bool thing. I
> don't think throwing in function pointers would be very helpful for
> readers, so we'd get a major code change for no gain.

There's more than one way to code a state machine.  Encoding the state
as enum / switch over next state is one.  Encoding the state as pointer
/ jump to next state is another.

[Qemu-devel] [PATCH v2 5/6] qapi: Allow documentation for features

[Kevin Wolf]

Features will be documented in a new part introduced by a "Features:"
line, after arguments and before named sections.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 scripts/qapi/common.py | 43 ++++++++++++++++++++++++++++++++++++++----
 scripts/qapi/doc.py    | 11 +++++++++++
 2 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 1d0f4847db..6a1ec87d41 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -132,6 +132,9 @@ class QAPIDoc(object):
         ARGS means that we're parsing the arguments section. Any symbol name is
         interpreted as an argument and an ArgSection is created for it.
 
+        FEATURES means that we're parsing features sections. Any symbol name is
+        interpreted as a feature.
+
         VARIOUS is the final part where freeform sections may appear. This
         includes named sections such as "Return:" as well as unnamed
         paragraphs. No symbols are allowed any more in this part.
@@ -139,7 +142,8 @@ class QAPIDoc(object):
         # Can't make it a subclass of Enum because of Python 2
         BODY = 0
         ARGS = 1
-        VARIOUS = 2
+        FEATURES = 2
+        VARIOUS = 3
 
     def __init__(self, parser, info):
         # self._parser is used to report errors with QAPIParseError.  The
@@ -152,6 +156,7 @@ class QAPIDoc(object):
         self.body = QAPIDoc.Section()
         # dict mapping parameter name to ArgSection
         self.args = OrderedDict()
+        self.features = OrderedDict()
         # a list of Section
         self.sections = []
         # the current section
@@ -180,6 +185,8 @@ class QAPIDoc(object):
             self._append_body_line(line)
         elif self._part == QAPIDoc.SymbolPart.ARGS:
             self._append_args_line(line)
+        elif self._part == QAPIDoc.SymbolPart.FEATURES:
+            self._append_features_line(line)
         elif self._part == QAPIDoc.SymbolPart.VARIOUS:
             self._append_various_line(line)
         else:
@@ -215,6 +222,8 @@ class QAPIDoc(object):
             if name.startswith('@') and name.endswith(':'):
                 self._part = QAPIDoc.SymbolPart.ARGS
                 self._append_args_line(line)
+            elif line == 'Features:':
+                self._part = QAPIDoc.SymbolPart.FEATURES
             elif self.symbol and self._check_named_section(line, name):
                 self._append_various_line(line)
             else:
@@ -231,6 +240,26 @@ class QAPIDoc(object):
             self._start_args_section(name[1:-1])
         elif self._check_named_section(line, name):
             return self._append_various_line(line)
+        elif (self._section.text.endswith('\n\n')
+              and line and not line[0].isspace()):
+            if line == 'Features:':
+                self._part = QAPIDoc.SymbolPart.FEATURES
+                return
+            else:
+                self._start_section()
+                self._part = QAPIDoc.SymbolPart.VARIOUS
+                return self._append_various_line(line)
+
+        self._append_freeform(line.strip())
+
+    def _append_features_line(self, line):
+        name = line.split(' ', 1)[0]
+
+        if name.startswith('@') and name.endswith(':'):
+            line = line[len(name)+1:]
+            self._start_features_section(name[1:-1])
+        elif self._check_named_section(line, name):
+            return self._append_various_line(line)
         elif (self._section.text.endswith('\n\n')
               and line and not line[0].isspace()):
             self._start_section()
@@ -256,17 +285,23 @@ class QAPIDoc(object):
 
         self._append_freeform(line)
 
-    def _start_args_section(self, name):
+    def _start_symbol_section(self, symbols_dict, name):
         # FIXME invalid names other than the empty string aren't flagged
         if not name:
             raise QAPIParseError(self._parser, "Invalid parameter name")
-        if name in self.args:
+        if name in symbols_dict:
             raise QAPIParseError(self._parser,
                                  "'%s' parameter name duplicated" % name)
         assert not self.sections
         self._end_section()
         self._section = QAPIDoc.ArgSection(name)
-        self.args[name] = self._section
+        symbols_dict[name] = self._section
+
+    def _start_args_section(self, name):
+        self._start_symbol_section(self.args, name)
+
+    def _start_features_section(self, name):
+        self._start_symbol_section(self.features, name)
 
     def _start_section(self, name=None):
         if name in ('Returns', 'Since') and self.has_section(name):
diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
index 433e9fcbfb..8e799b9e0b 100755
--- a/scripts/qapi/doc.py
+++ b/scripts/qapi/doc.py
@@ -181,6 +181,16 @@ def texi_members(doc, what, base, variants, member_func):
         return ''
     return '\n@b{%s:}\n@table @asis\n%s@end table\n' % (what, items)
 
+def texi_features(doc):
+    """Format the table of features"""
+    items = ''
+    for section in doc.features.values():
+        desc = texi_format(section.text)
+        items += '@item @code{%s}\n%s' % (section.name, desc)
+    if not items:
+        return ''
+    return '\n@b{Features:}\n@table @asis\n%s@end table\n' % (items)
+
 
 def texi_sections(doc, ifcond):
     """Format additional sections following arguments"""
@@ -201,6 +211,7 @@ def texi_entity(doc, what, ifcond, base=None, variants=None,
                 member_func=texi_member):
     return (texi_body(doc)
             + texi_members(doc, what, base, variants, member_func)
+            + texi_features(doc)
             + texi_sections(doc, ifcond))
 
 
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 5/6] qapi: Allow documentation for features

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Features will be documented in a new part introduced by a "Features:"
> line, after arguments and before named sections.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  scripts/qapi/common.py | 43 ++++++++++++++++++++++++++++++++++++++----
>  scripts/qapi/doc.py    | 11 +++++++++++
>  2 files changed, 50 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 1d0f4847db..6a1ec87d41 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -132,6 +132,9 @@ class QAPIDoc(object):
>          ARGS means that we're parsing the arguments section. Any symbol name is
>          interpreted as an argument and an ArgSection is created for it.
>  
> +        FEATURES means that we're parsing features sections. Any symbol name is
> +        interpreted as a feature.
> +
>          VARIOUS is the final part where freeform sections may appear. This
>          includes named sections such as "Return:" as well as unnamed
>          paragraphs. No symbols are allowed any more in this part.
> @@ -139,7 +142,8 @@ class QAPIDoc(object):
>          # Can't make it a subclass of Enum because of Python 2
>          BODY = 0
>          ARGS = 1
> -        VARIOUS = 2
> +        FEATURES = 2
> +        VARIOUS = 3
>  
>      def __init__(self, parser, info):
>          # self._parser is used to report errors with QAPIParseError.  The
> @@ -152,6 +156,7 @@ class QAPIDoc(object):
>          self.body = QAPIDoc.Section()
>          # dict mapping parameter name to ArgSection
>          self.args = OrderedDict()
> +        self.features = OrderedDict()
>          # a list of Section
>          self.sections = []
>          # the current section
> @@ -180,6 +185,8 @@ class QAPIDoc(object):
>              self._append_body_line(line)
>          elif self._part == QAPIDoc.SymbolPart.ARGS:
>              self._append_args_line(line)
> +        elif self._part == QAPIDoc.SymbolPart.FEATURES:
> +            self._append_features_line(line)
>          elif self._part == QAPIDoc.SymbolPart.VARIOUS:
>              self._append_various_line(line)
>          else:
> @@ -215,6 +222,8 @@ class QAPIDoc(object):
>              if name.startswith('@') and name.endswith(':'):
>                  self._part = QAPIDoc.SymbolPart.ARGS
>                  self._append_args_line(line)
> +            elif line == 'Features:':
> +                self._part = QAPIDoc.SymbolPart.FEATURES
>              elif self.symbol and self._check_named_section(line, name):
>                  self._append_various_line(line)
>              else:
> @@ -231,6 +240,26 @@ class QAPIDoc(object):
>              self._start_args_section(name[1:-1])
>          elif self._check_named_section(line, name):
>              return self._append_various_line(line)

Here you return something...

> +        elif (self._section.text.endswith('\n\n')
> +              and line and not line[0].isspace()):
> +            if line == 'Features:':
> +                self._part = QAPIDoc.SymbolPart.FEATURES
> +                return

... and here you don't.

> +            else:

Unnecessary else after return.  Let's scratch the else.

> +                self._start_section()
> +                self._part = QAPIDoc.SymbolPart.VARIOUS
> +                return self._append_various_line(line)
> +
> +        self._append_freeform(line.strip())
> +
> +    def _append_features_line(self, line):
> +        name = line.split(' ', 1)[0]
> +
> +        if name.startswith('@') and name.endswith(':'):
> +            line = line[len(name)+1:]
> +            self._start_features_section(name[1:-1])
> +        elif self._check_named_section(line, name):
> +            return self._append_various_line(line)
>          elif (self._section.text.endswith('\n\n')
>                and line and not line[0].isspace()):
>              self._start_section()
> @@ -256,17 +285,23 @@ class QAPIDoc(object):
>  
>          self._append_freeform(line)
>  
> -    def _start_args_section(self, name):
> +    def _start_symbol_section(self, symbols_dict, name):
>          # FIXME invalid names other than the empty string aren't flagged
>          if not name:
>              raise QAPIParseError(self._parser, "Invalid parameter name")
> -        if name in self.args:
> +        if name in symbols_dict:
>              raise QAPIParseError(self._parser,
>                                   "'%s' parameter name duplicated" % name)
>          assert not self.sections
>          self._end_section()
>          self._section = QAPIDoc.ArgSection(name)
> -        self.args[name] = self._section
> +        symbols_dict[name] = self._section
> +
> +    def _start_args_section(self, name):
> +        self._start_symbol_section(self.args, name)
> +
> +    def _start_features_section(self, name):
> +        self._start_symbol_section(self.features, name)
>  
>      def _start_section(self, name=None):
>          if name in ('Returns', 'Since') and self.has_section(name):
> diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
> index 433e9fcbfb..8e799b9e0b 100755
> --- a/scripts/qapi/doc.py
> +++ b/scripts/qapi/doc.py
> @@ -181,6 +181,16 @@ def texi_members(doc, what, base, variants, member_func):
>          return ''
>      return '\n@b{%s:}\n@table @asis\n%s@end table\n' % (what, items)
>  
> +def texi_features(doc):
> +    """Format the table of features"""
> +    items = ''
> +    for section in doc.features.values():
> +        desc = texi_format(section.text)
> +        items += '@item @code{%s}\n%s' % (section.name, desc)
> +    if not items:
> +        return ''
> +    return '\n@b{Features:}\n@table @asis\n%s@end table\n' % (items)
> +
>  
>  def texi_sections(doc, ifcond):
>      """Format additional sections following arguments"""
> @@ -201,6 +211,7 @@ def texi_entity(doc, what, ifcond, base=None, variants=None,
>                  member_func=texi_member):
>      return (texi_body(doc)
>              + texi_members(doc, what, base, variants, member_func)
> +            + texi_features(doc)
>              + texi_sections(doc, ifcond))

[Qemu-devel] [PATCH v2 6/6] file-posix: Add dynamic-auto-read-only QAPI feature

[Kevin Wolf]

In commit 23dece19da4 ('file-posix: Make auto-read-only dynamic') ,
auto-read-only=on changed its behaviour in file-posix for the 4.0
release. This change cannot be detected through the usual mechanisms
like schema introspection. Add a new feature flag to the schema to
allow libvirt to detect the presence of the new behaviour.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qapi/block-core.json | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 7ccbfff9d0..e820e7dfb0 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2843,6 +2843,15 @@
 #                         file is large, do not use in production.
 #                         (default: off) (since: 3.0)
 #
+# Features:
+# @dynamic-auto-read-only: If present, enabled auto-read-only means that the
+#                          driver will open the image read-only at first,
+#                          dynamically reopen the image file read-write when
+#                          the first writer is attached to the node and reopen
+#                          read-only when the last writer is detached. This
+#                          allows to give QEMU write permissions only on demand
+#                          when an operation actually needs write access.
+#
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsFile',
@@ -2852,7 +2861,9 @@
             '*aio': 'BlockdevAioOptions',
 	    '*drop-cache': {'type': 'bool',
 	                    'if': 'defined(CONFIG_LINUX)'},
-            '*x-check-cache-dropped': 'bool' } }
+            '*x-check-cache-dropped': 'bool' },
+  'features': [ { 'name': 'dynamic-auto-read-only',
+                  'if': 'defined(CONFIG_POSIX)' } ] }
 
 ##
 # @BlockdevOptionsNull:
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 0/6] file-posix: Add dynamic-auto-read-only QAPI feature

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> This series adds optional feature lists to struct definitions in the
> QAPI schema and makes use of them to advertise the new behaviour of
> auto-read-only=on in file-posix.

Found nothing major.  Looking forward to v3!