[PATCH v3 0/3] qapi: Add detection for the 'savevm' fix for blockdev

[PATCH v3 0/3] qapi: Add detection for the 'savevm' fix for blockdev

[Peter Krempa]

Add 'features' field in the schema for commands and add a feature flag
to advertise that the fix for savevm [1] is present.

[1] https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg03487.html

v3:
 - added tests
 - mentioned commit fixing savevm in the patch adding the new feature
 - mentioned actual problem in the patch adding the new feature
 - rebased on top of the last batch of Markus' cleanups

Peter Krempa (3):
  qapi: Add feature flags to commands in qapi
  tests: qapi: Test 'features' of commands
  qapi: Allow introspecting fix for savevm's cooperation with blockdev

 docs/devel/qapi-code-gen.txt            |  7 +++---
 qapi/introspect.json                    |  6 ++++-
 qapi/misc.json                          |  9 +++++++-
 scripts/qapi/commands.py                |  3 ++-
 scripts/qapi/doc.py                     |  3 ++-
 scripts/qapi/expr.py                    | 17 ++++++++++++++-
 scripts/qapi/introspect.py              |  7 +++++-
 scripts/qapi/schema.py                  | 22 +++++++++++++++----
 tests/qapi-schema/qapi-schema-test.json | 26 ++++++++++++++++++++++
 tests/qapi-schema/qapi-schema-test.out  | 29 +++++++++++++++++++++++++
 tests/qapi-schema/test-qapi.py          |  7 +++++-
 tests/test-qmp-cmds.c                   | 28 ++++++++++++++++++++++++
 12 files changed, 150 insertions(+), 14 deletions(-)

-- 
2.21.0

[PATCH v3 1/3] qapi: Add feature flags to commands in qapi

[Peter Krempa]

Similarly to features for struct types introduce the feature flags also
for commands. This will allow notifying management layers of fixes and
compatible changes in the behaviour of a command which may not be
detectable any other way.

The changes were heavily inspired by commit 6a8c0b51025.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 docs/devel/qapi-code-gen.txt   |  7 ++++---
 qapi/introspect.json           |  6 +++++-
 scripts/qapi/commands.py       |  3 ++-
 scripts/qapi/doc.py            |  3 ++-
 scripts/qapi/expr.py           | 17 ++++++++++++++++-
 scripts/qapi/introspect.py     |  7 ++++++-
 scripts/qapi/schema.py         | 22 ++++++++++++++++++----
 tests/qapi-schema/test-qapi.py |  3 ++-
 8 files changed, 55 insertions(+), 13 deletions(-)

diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 64d9e4c6a9..637fa49977 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -640,9 +640,10 @@ 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.
+For this purpose, a list of features can be specified for a command or
+struct type.  This is exposed to the client as a list of strings,
+where each string signals that this build of QEMU shows a certain
+behaviour.

 Each member of the 'features' array defines a feature.  It can either
 be { 'name': STRING, '*if': COND }, or STRING, which is shorthand for
diff --git a/qapi/introspect.json b/qapi/introspect.json
index 1843c1cb17..031a954fa9 100644
--- a/qapi/introspect.json
+++ b/qapi/introspect.json
@@ -266,13 +266,17 @@
 # @allow-oob: whether the command allows out-of-band execution,
 #             defaults to false (Since: 2.12)
 #
+# @features: names of features associated with the command, in no particular
+#            order. (since 4.2)
+#
 # TODO: @success-response (currently irrelevant, because it's QGA, not QMP)
 #
 # Since: 2.5
 ##
 { 'struct': 'SchemaInfoCommand',
   'data': { 'arg-type': 'str', 'ret-type': 'str',
-            '*allow-oob': 'bool' } }
+            '*allow-oob': 'bool',
+            '*features': [ 'str' ] } }

 ##
 # @SchemaInfoEvent:
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 898516b086..ab98e504f3 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -277,7 +277,8 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
         genc.add(gen_registry(self._regy.get_content(), self._prefix))

     def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
-                      success_response, boxed, allow_oob, allow_preconfig):
+                      success_response, boxed, allow_oob, allow_preconfig,
+                      features):
         if not gen:
             return
         # FIXME: If T is a user-defined type, the user is responsible
diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
index dc8919bab7..8278ccbc43 100644
--- a/scripts/qapi/doc.py
+++ b/scripts/qapi/doc.py
@@ -249,7 +249,8 @@ class QAPISchemaGenDocVisitor(QAPISchemaVisitor):
                                body=texi_entity(doc, 'Members', ifcond)))

     def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
-                      success_response, boxed, allow_oob, allow_preconfig):
+                      success_response, boxed, allow_oob, allow_preconfig,
+                      features):
         doc = self.cur_doc
         if boxed:
             body = texi_body(doc)
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index da23063f57..129a4075f0 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -277,12 +277,26 @@ def check_command(expr, info):
     args = expr.get('data')
     rets = expr.get('returns')
     boxed = expr.get('boxed', False)
+    features = expr.get('features')

     if boxed and args is None:
         raise QAPISemError(info, "'boxed': true requires 'data'")
     check_type(args, info, "'data'", allow_dict=not boxed)
     check_type(rets, info, "'returns'", allow_array=True)

+    if features:
+        if not isinstance(features, list):
+            raise QAPISemError(info, "'features' must be an array")
+        for f in features:
+            source = "'features' member"
+            assert isinstance(f, dict)
+            check_keys(f, info, source, ['name'], ['if'])
+            check_name_is_str(f['name'], info, source)
+            source = "%s '%s'" % (source, f['name'])
+            check_name_str(f['name'], info, source)
+            check_if(f, info, source)
+            normalize_if(f)
+

 def check_event(expr, info):
     args = expr.get('data')
@@ -357,10 +371,11 @@ def check_exprs(exprs):
         elif meta == 'command':
             check_keys(expr, info, meta,
                        ['command'],
-                       ['data', 'returns', 'boxed', 'if',
+                       ['data', 'returns', 'boxed', 'if', 'features',
                         'gen', 'success-response', 'allow-oob',
                         'allow-preconfig'])
             normalize_members(expr.get('data'))
+            normalize_features(expr.get('features'))
             check_command(expr, info)
         elif meta == 'event':
             check_keys(expr, info, meta,
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index d1c1ad346d..739b35ae8f 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -209,13 +209,18 @@ const QLitObject %(c_name)s = %(c_string)s;
                            for m in variants.variants]}, ifcond)

     def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
-                      success_response, boxed, allow_oob, allow_preconfig):
+                      success_response, boxed, allow_oob, allow_preconfig,
+                      features):
         arg_type = arg_type or self._schema.the_empty_object_type
         ret_type = ret_type or self._schema.the_empty_object_type
         obj = {'arg-type': self._use_type(arg_type),
                'ret-type': self._use_type(ret_type)}
         if allow_oob:
             obj['allow-oob'] = allow_oob
+
+        if features:
+            obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
+
         self._gen_qlit(name, 'command', obj, ifcond)

     def visit_event(self, name, info, ifcond, arg_type, boxed):
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 38041098bd..8a48231766 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -109,7 +109,8 @@ class QAPISchemaVisitor(object):
         pass

     def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
-                      success_response, boxed, allow_oob, allow_preconfig):
+                      success_response, boxed, allow_oob, allow_preconfig,
+                      features):
         pass

     def visit_event(self, name, info, ifcond, arg_type, boxed):
@@ -658,10 +659,14 @@ class QAPISchemaCommand(QAPISchemaEntity):
     meta = 'command'

     def __init__(self, name, info, doc, ifcond, arg_type, ret_type,
-                 gen, success_response, boxed, allow_oob, allow_preconfig):
+                 gen, success_response, boxed, allow_oob, allow_preconfig,
+                 features):
         QAPISchemaEntity.__init__(self, name, info, doc, ifcond)
         assert not arg_type or isinstance(arg_type, str)
         assert not ret_type or isinstance(ret_type, str)
+        for f in features:
+            assert isinstance(f, QAPISchemaFeature)
+            f.set_defined_in(name)
         self._arg_type_name = arg_type
         self.arg_type = None
         self._ret_type_name = ret_type
@@ -671,6 +676,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
         self.boxed = boxed
         self.allow_oob = allow_oob
         self.allow_preconfig = allow_preconfig
+        self.features = features

     def check(self, schema):
         QAPISchemaEntity.check(self, schema)
@@ -700,13 +706,19 @@ class QAPISchemaCommand(QAPISchemaEntity):
                         "command's 'returns' cannot take %s"
                         % self.ret_type.describe())

+        # Features are in a name space separate from members
+        seen = {}
+        for f in self.features:
+            f.check_clash(self.info, seen)
+
     def visit(self, visitor):
         QAPISchemaEntity.visit(self, visitor)
         visitor.visit_command(self.name, self.info, self.ifcond,
                               self.arg_type, self.ret_type,
                               self.gen, self.success_response,
                               self.boxed, self.allow_oob,
-                              self.allow_preconfig)
+                              self.allow_preconfig,
+                              self.features)


 class QAPISchemaEvent(QAPISchemaEntity):
@@ -983,6 +995,7 @@ class QAPISchema(object):
         allow_oob = expr.get('allow-oob', False)
         allow_preconfig = expr.get('allow-preconfig', False)
         ifcond = expr.get('if')
+        features = expr.get('features', [])
         if isinstance(data, OrderedDict):
             data = self._make_implicit_object_type(
                 name, info, doc, ifcond, 'arg', self._make_members(data, info))
@@ -991,7 +1004,8 @@ class QAPISchema(object):
             rets = self._make_array_type(rets[0], info)
         self._def_entity(QAPISchemaCommand(name, info, doc, ifcond, data, rets,
                                            gen, success_response,
-                                           boxed, allow_oob, allow_preconfig))
+                                           boxed, allow_oob, allow_preconfig,
+                                           self._make_features(features, info)))

     def _def_event(self, expr, info, doc):
         name = expr['event']
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index 664254618a..e13c2e8671 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -72,7 +72,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
         self._print_if(ifcond)

     def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
-                      success_response, boxed, allow_oob, allow_preconfig):
+                      success_response, boxed, allow_oob, allow_preconfig,
+                      features):
         print('command %s %s -> %s'
               % (name, arg_type and arg_type.name,
                  ret_type and ret_type.name))
-- 
2.21.0

[PATCH v3 2/3] tests: qapi: Test 'features' of commands

[Peter Krempa]

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 tests/qapi-schema/qapi-schema-test.json | 26 ++++++++++++++++++++++
 tests/qapi-schema/qapi-schema-test.out  | 29 +++++++++++++++++++++++++
 tests/qapi-schema/test-qapi.py          |  4 ++++
 tests/test-qmp-cmds.c                   | 28 ++++++++++++++++++++++++
 4 files changed, 87 insertions(+)

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 75c42eb0e3..220859d4c9 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -290,3 +290,29 @@
             'cfs1': 'CondFeatureStruct1',
             'cfs2': 'CondFeatureStruct2',
             'cfs3': 'CondFeatureStruct3' } }
+
+# test 'features' for command
+
+{ 'command': 'test-command-features1',
+  'features': [] }
+
+{ 'command': 'test-command-features2',
+  'features': [ 'feature1' ] }
+
+{ 'command': 'test-command-features3',
+  'features': [ 'feature1', 'feature2' ] }
+
+{ 'command': 'test-command-features4',
+  'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'} ] }
+
+{ 'command': 'test-command-features5',
+  'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'},
+                { 'name': 'feature2', 'if': 'defined(TEST_IF_FEATURE_2)'} ] }
+
+{ 'command': 'test-command-features6',
+  'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'},
+                { 'name': 'feature2', 'if': 'defined(TEST_IF_FEATURE_2)'} ] }
+
+{ 'command': 'test-command-features7',
+  '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 98031da96f..a38e348d54 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -412,3 +412,32 @@ object q_obj_test-features-arg
     member cfs3: CondFeatureStruct3 optional=False
 command test-features q_obj_test-features-arg -> None
    gen=True success_response=True boxed=False oob=False preconfig=False
+command test-command-features1 None -> None
+   gen=True success_response=True boxed=False oob=False preconfig=False
+command test-command-features2 None -> None
+   gen=True success_response=True boxed=False oob=False preconfig=False
+   feature feature1
+command test-command-features3 None -> None
+   gen=True success_response=True boxed=False oob=False preconfig=False
+   feature feature1
+   feature feature2
+command test-command-features4 None -> None
+   gen=True success_response=True boxed=False oob=False preconfig=False
+   feature feature1
+        if ['defined(TEST_IF_FEATURE_1)']
+command test-command-features5 None -> None
+   gen=True success_response=True boxed=False oob=False preconfig=False
+   feature feature1
+        if ['defined(TEST_IF_FEATURE_1)']
+   feature feature2
+        if ['defined(TEST_IF_FEATURE_2)']
+command test-command-features6 None -> None
+   gen=True success_response=True boxed=False oob=False preconfig=False
+   feature feature1
+        if ['defined(TEST_IF_FEATURE_1)']
+   feature feature2
+        if ['defined(TEST_IF_FEATURE_2)']
+command test-command-features7 None -> None
+   gen=True success_response=True boxed=False oob=False preconfig=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 e13c2e8671..62e65b6a7d 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -80,6 +80,10 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
         print('   gen=%s success_response=%s boxed=%s oob=%s preconfig=%s'
               % (gen, success_response, boxed, allow_oob, allow_preconfig))
         self._print_if(ifcond)
+        if features:
+            for f in features:
+                print('   feature %s' % f.name)
+                self._print_if(f.ifcond, 8)

     def visit_event(self, name, info, ifcond, arg_type, boxed):
         print('event %s %s' % (name, arg_type and arg_type.name))
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index 36fdf5b115..19f6e06ba7 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -51,6 +51,34 @@ void qmp_test_features(FeatureStruct0 *fs0, FeatureStruct1 *fs1,
 {
 }

+void qmp_test_command_features1(Error **errp)
+{
+}
+
+void qmp_test_command_features2(Error **errp)
+{
+}
+
+void qmp_test_command_features3(Error **errp)
+{
+}
+
+void qmp_test_command_features4(Error **errp)
+{
+}
+
+void qmp_test_command_features5(Error **errp)
+{
+}
+
+void qmp_test_command_features6(Error **errp)
+{
+}
+
+void qmp_test_command_features7(Error **errp)
+{
+}
+
 UserDefTwo *qmp_user_def_cmd2(UserDefOne *ud1a,
                               bool has_udb1, UserDefOne *ud1b,
                               Error **errp)
-- 
2.21.0

[PATCH v3 3/3] qapi: Allow introspecting fix for savevm's cooperation with blockdev

[Peter Krempa]

'savevm' was buggy as it considered all monitor-owned block device nodes
for snapshot. With introduction of -blockdev the common usage made all
nodes including protocol and backing file nodes monitor-owned and thus
considered for snapshot.

This is a problem since the 'file' protocol nodes can't have internal
snapshots and it does not make sense to take snapshot of nodes
representing backing files.

This was fixed by commit 05f4aced658a02b02 clients need to be able to
detect whether this fix is present.

Since savevm does not have an QMP alternative, add the feature for the
'human-monitor-command' backdoor which is used to call this command in
modern use.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 qapi/misc.json | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/qapi/misc.json b/qapi/misc.json
index 6bd11f50e6..5d0070aa43 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -1020,6 +1020,12 @@
 #
 # @cpu-index: The CPU to use for commands that require an implicit CPU
 #
+# Features:
+# @savevm-blockdev-monitor-nodes: If present, the HMP command savevm only
+#                                 snapshots monitor-owned nodes if they have no
+#                                 parents. This allows to use 'savevm' with
+#                                 -blockdev. (since 4.2)
+#
 # Returns: the output of the command as a string
 #
 # Since: 0.14.0
@@ -1047,7 +1053,8 @@
 ##
 { 'command': 'human-monitor-command',
   'data': {'command-line': 'str', '*cpu-index': 'int'},
-  'returns': 'str' }
+  'returns': 'str',
+  'features': [ 'savevm-blockdev-monitor-nodes' ] }

 ##
 # @change:
-- 
2.21.0

Re: [PATCH v3 3/3] qapi: Allow introspecting fix for savevm's cooperation with blockdev

[Markus Armbruster]

Peter Krempa <pkrempa@redhat.com> writes:

> 'savevm' was buggy as it considered all monitor-owned block device nodes
> for snapshot. With introduction of -blockdev the common usage made all
> nodes including protocol and backing file nodes monitor-owned and thus
> considered for snapshot.
>
> This is a problem since the 'file' protocol nodes can't have internal
> snapshots and it does not make sense to take snapshot of nodes
> representing backing files.
>
> This was fixed by commit 05f4aced658a02b02 clients need to be able to
> detect whether this fix is present.

05f4aced658a02b02.  Clients

>
> Since savevm does not have an QMP alternative, add the feature for the
> 'human-monitor-command' backdoor which is used to call this command in
> modern use.
>
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>

Preferrably with the commit message tweak:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v3 2/3] tests: qapi: Test 'features' of commands

[Markus Armbruster]

Peter Krempa <pkrempa@redhat.com> writes:

> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  tests/qapi-schema/qapi-schema-test.json | 26 ++++++++++++++++++++++
>  tests/qapi-schema/qapi-schema-test.out  | 29 +++++++++++++++++++++++++
>  tests/qapi-schema/test-qapi.py          |  4 ++++
>  tests/test-qmp-cmds.c                   | 28 ++++++++++++++++++++++++
>  4 files changed, 87 insertions(+)

More thorough than I asked for.  I'm not complaining :)

>
> diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> index 75c42eb0e3..220859d4c9 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -290,3 +290,29 @@
>              'cfs1': 'CondFeatureStruct1',
>              'cfs2': 'CondFeatureStruct2',
>              'cfs3': 'CondFeatureStruct3' } }
> +
> +# test 'features' for command
> +
> +{ 'command': 'test-command-features1',
> +  'features': [] }
> +
> +{ 'command': 'test-command-features2',
> +  'features': [ 'feature1' ] }
> +
> +{ 'command': 'test-command-features3',
> +  'features': [ 'feature1', 'feature2' ] }
> +
> +{ 'command': 'test-command-features4',
> +  'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'} ] }
> +
> +{ 'command': 'test-command-features5',
> +  'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'},
> +                { 'name': 'feature2', 'if': 'defined(TEST_IF_FEATURE_2)'} ] }
> +
> +{ 'command': 'test-command-features6',
> +  'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'},
> +                { 'name': 'feature2', 'if': 'defined(TEST_IF_FEATURE_2)'} ] }

Do you need both test-command-features5 and 6?  They look the same to me...

> +
> +{ 'command': 'test-command-features7',
> +  '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 98031da96f..a38e348d54 100644
> --- a/tests/qapi-schema/qapi-schema-test.out
> +++ b/tests/qapi-schema/qapi-schema-test.out
> @@ -412,3 +412,32 @@ object q_obj_test-features-arg
>      member cfs3: CondFeatureStruct3 optional=False
>  command test-features q_obj_test-features-arg -> None
>     gen=True success_response=True boxed=False oob=False preconfig=False
> +command test-command-features1 None -> None
> +   gen=True success_response=True boxed=False oob=False preconfig=False
> +command test-command-features2 None -> None
> +   gen=True success_response=True boxed=False oob=False preconfig=False
> +   feature feature1
> +command test-command-features3 None -> None
> +   gen=True success_response=True boxed=False oob=False preconfig=False
> +   feature feature1
> +   feature feature2
> +command test-command-features4 None -> None
> +   gen=True success_response=True boxed=False oob=False preconfig=False
> +   feature feature1
> +        if ['defined(TEST_IF_FEATURE_1)']
> +command test-command-features5 None -> None
> +   gen=True success_response=True boxed=False oob=False preconfig=False
> +   feature feature1
> +        if ['defined(TEST_IF_FEATURE_1)']
> +   feature feature2
> +        if ['defined(TEST_IF_FEATURE_2)']
> +command test-command-features6 None -> None
> +   gen=True success_response=True boxed=False oob=False preconfig=False
> +   feature feature1
> +        if ['defined(TEST_IF_FEATURE_1)']
> +   feature feature2
> +        if ['defined(TEST_IF_FEATURE_2)']
> +command test-command-features7 None -> None
> +   gen=True success_response=True boxed=False oob=False preconfig=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 e13c2e8671..62e65b6a7d 100755
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -80,6 +80,10 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
>          print('   gen=%s success_response=%s boxed=%s oob=%s preconfig=%s'
>                % (gen, success_response, boxed, allow_oob, allow_preconfig))
>          self._print_if(ifcond)
> +        if features:
> +            for f in features:
> +                print('   feature %s' % f.name)
> +                self._print_if(f.ifcond, 8)

Copied from visit_object_type().  Let's factor it into a @staticmethod
_print_features().

>
>      def visit_event(self, name, info, ifcond, arg_type, boxed):
>          print('event %s %s' % (name, arg_type and arg_type.name))
> diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
> index 36fdf5b115..19f6e06ba7 100644
> --- a/tests/test-qmp-cmds.c
> +++ b/tests/test-qmp-cmds.c
> @@ -51,6 +51,34 @@ void qmp_test_features(FeatureStruct0 *fs0, FeatureStruct1 *fs1,
>  {
>  }
>
> +void qmp_test_command_features1(Error **errp)
> +{
> +}
> +
> +void qmp_test_command_features2(Error **errp)
> +{
> +}
> +
> +void qmp_test_command_features3(Error **errp)
> +{
> +}
> +
> +void qmp_test_command_features4(Error **errp)
> +{
> +}
> +
> +void qmp_test_command_features5(Error **errp)
> +{
> +}
> +
> +void qmp_test_command_features6(Error **errp)
> +{
> +}
> +
> +void qmp_test_command_features7(Error **errp)
> +{
> +}
> +
>  UserDefTwo *qmp_user_def_cmd2(UserDefOne *ud1a,
>                                bool has_udb1, UserDefOne *ud1b,
>                                Error **errp)

Any particular reason why we shouldn't squash this into PATCH 1?

Re: [PATCH v3 1/3] qapi: Add feature flags to commands in qapi

[Markus Armbruster]

Peter Krempa <pkrempa@redhat.com> writes:

> Similarly to features for struct types introduce the feature flags also
> for commands. This will allow notifying management layers of fixes and
> compatible changes in the behaviour of a command which may not be
> detectable any other way.
>
> The changes were heavily inspired by commit 6a8c0b51025.
>
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  docs/devel/qapi-code-gen.txt   |  7 ++++---
>  qapi/introspect.json           |  6 +++++-
>  scripts/qapi/commands.py       |  3 ++-
>  scripts/qapi/doc.py            |  3 ++-
>  scripts/qapi/expr.py           | 17 ++++++++++++++++-
>  scripts/qapi/introspect.py     |  7 ++++++-
>  scripts/qapi/schema.py         | 22 ++++++++++++++++++----
>  tests/qapi-schema/test-qapi.py |  3 ++-
>  8 files changed, 55 insertions(+), 13 deletions(-)
>
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index 64d9e4c6a9..637fa49977 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -640,9 +640,10 @@ 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.
> +For this purpose, a list of features can be specified for a command or
> +struct type.  This is exposed to the client as a list of strings,
> +where each string signals that this build of QEMU shows a certain
> +behaviour.
>
>  Each member of the 'features' array defines a feature.  It can either
>  be { 'name': STRING, '*if': COND }, or STRING, which is shorthand for
> diff --git a/qapi/introspect.json b/qapi/introspect.json
> index 1843c1cb17..031a954fa9 100644
> --- a/qapi/introspect.json
> +++ b/qapi/introspect.json
> @@ -266,13 +266,17 @@
>  # @allow-oob: whether the command allows out-of-band execution,
>  #             defaults to false (Since: 2.12)
>  #
> +# @features: names of features associated with the command, in no particular
> +#            order. (since 4.2)
> +#
>  # TODO: @success-response (currently irrelevant, because it's QGA, not QMP)
>  #
>  # Since: 2.5
>  ##
>  { 'struct': 'SchemaInfoCommand',
>    'data': { 'arg-type': 'str', 'ret-type': 'str',
> -            '*allow-oob': 'bool' } }
> +            '*allow-oob': 'bool',
> +            '*features': [ 'str' ] } }
>
>  ##
>  # @SchemaInfoEvent:
> diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> index 898516b086..ab98e504f3 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -277,7 +277,8 @@ void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
>          genc.add(gen_registry(self._regy.get_content(), self._prefix))
>
>      def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
> -                      success_response, boxed, allow_oob, allow_preconfig):
> +                      success_response, boxed, allow_oob, allow_preconfig,
> +                      features):
>          if not gen:
>              return
>          # FIXME: If T is a user-defined type, the user is responsible
> diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
> index dc8919bab7..8278ccbc43 100644
> --- a/scripts/qapi/doc.py
> +++ b/scripts/qapi/doc.py
> @@ -249,7 +249,8 @@ class QAPISchemaGenDocVisitor(QAPISchemaVisitor):
>                                 body=texi_entity(doc, 'Members', ifcond)))
>
>      def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
> -                      success_response, boxed, allow_oob, allow_preconfig):
> +                      success_response, boxed, allow_oob, allow_preconfig,
> +                      features):
>          doc = self.cur_doc
>          if boxed:
>              body = texi_body(doc)
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
> index da23063f57..129a4075f0 100644
> --- a/scripts/qapi/expr.py
> +++ b/scripts/qapi/expr.py
> @@ -277,12 +277,26 @@ def check_command(expr, info):
>      args = expr.get('data')
>      rets = expr.get('returns')
>      boxed = expr.get('boxed', False)
> +    features = expr.get('features')
>
>      if boxed and args is None:
>          raise QAPISemError(info, "'boxed': true requires 'data'")
>      check_type(args, info, "'data'", allow_dict=not boxed)
>      check_type(rets, info, "'returns'", allow_array=True)
>
> +    if features:
> +        if not isinstance(features, list):
> +            raise QAPISemError(info, "'features' must be an array")
> +        for f in features:
> +            source = "'features' member"
> +            assert isinstance(f, dict)
> +            check_keys(f, info, source, ['name'], ['if'])
> +            check_name_is_str(f['name'], info, source)
> +            source = "%s '%s'" % (source, f['name'])
> +            check_name_str(f['name'], info, source)
> +            check_if(f, info, source)
> +            normalize_if(f)
> +

Copied from check_struct().  Let's factor it into a helper function
check_features(features, info).

>
>  def check_event(expr, info):
>      args = expr.get('data')
> @@ -357,10 +371,11 @@ def check_exprs(exprs):
>          elif meta == 'command':
>              check_keys(expr, info, meta,
>                         ['command'],
> -                       ['data', 'returns', 'boxed', 'if',
> +                       ['data', 'returns', 'boxed', 'if', 'features',
>                          'gen', 'success-response', 'allow-oob',
>                          'allow-preconfig'])
>              normalize_members(expr.get('data'))
> +            normalize_features(expr.get('features'))
>              check_command(expr, info)
>          elif meta == 'event':
>              check_keys(expr, info, meta,
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index d1c1ad346d..739b35ae8f 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -209,13 +209,18 @@ const QLitObject %(c_name)s = %(c_string)s;
>                             for m in variants.variants]}, ifcond)
>
>      def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
> -                      success_response, boxed, allow_oob, allow_preconfig):
> +                      success_response, boxed, allow_oob, allow_preconfig,
> +                      features):
>          arg_type = arg_type or self._schema.the_empty_object_type
>          ret_type = ret_type or self._schema.the_empty_object_type
>          obj = {'arg-type': self._use_type(arg_type),
>                 'ret-type': self._use_type(ret_type)}
>          if allow_oob:
>              obj['allow-oob'] = allow_oob
> +
> +        if features:
> +            obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
> +

Copied from visit_object_type_flat().  Acceptable for now.

>          self._gen_qlit(name, 'command', obj, ifcond)
>
>      def visit_event(self, name, info, ifcond, arg_type, boxed):
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 38041098bd..8a48231766 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -109,7 +109,8 @@ class QAPISchemaVisitor(object):
>          pass
>
>      def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
> -                      success_response, boxed, allow_oob, allow_preconfig):
> +                      success_response, boxed, allow_oob, allow_preconfig,
> +                      features):
>          pass
>
>      def visit_event(self, name, info, ifcond, arg_type, boxed):
> @@ -658,10 +659,14 @@ class QAPISchemaCommand(QAPISchemaEntity):
>      meta = 'command'
>
>      def __init__(self, name, info, doc, ifcond, arg_type, ret_type,
> -                 gen, success_response, boxed, allow_oob, allow_preconfig):
> +                 gen, success_response, boxed, allow_oob, allow_preconfig,
> +                 features):
>          QAPISchemaEntity.__init__(self, name, info, doc, ifcond)
>          assert not arg_type or isinstance(arg_type, str)
>          assert not ret_type or isinstance(ret_type, str)
> +        for f in features:
> +            assert isinstance(f, QAPISchemaFeature)
> +            f.set_defined_in(name)

Copied from QAPISchemaObjectType.__init__().  Acceptable for now.

>          self._arg_type_name = arg_type
>          self.arg_type = None
>          self._ret_type_name = ret_type
> @@ -671,6 +676,7 @@ class QAPISchemaCommand(QAPISchemaEntity):
>          self.boxed = boxed
>          self.allow_oob = allow_oob
>          self.allow_preconfig = allow_preconfig
> +        self.features = features
>
>      def check(self, schema):
>          QAPISchemaEntity.check(self, schema)
> @@ -700,13 +706,19 @@ class QAPISchemaCommand(QAPISchemaEntity):
>                          "command's 'returns' cannot take %s"
>                          % self.ret_type.describe())
>
> +        # Features are in a name space separate from members
> +        seen = {}
> +        for f in self.features:
> +            f.check_clash(self.info, seen)
> +

Copied from QAPISchemaObjectType.check().  Acceptable for now.

>      def visit(self, visitor):
>          QAPISchemaEntity.visit(self, visitor)
>          visitor.visit_command(self.name, self.info, self.ifcond,
>                                self.arg_type, self.ret_type,
>                                self.gen, self.success_response,
>                                self.boxed, self.allow_oob,
> -                              self.allow_preconfig)
> +                              self.allow_preconfig,
> +                              self.features)
>
>
>  class QAPISchemaEvent(QAPISchemaEntity):
> @@ -983,6 +995,7 @@ class QAPISchema(object):
>          allow_oob = expr.get('allow-oob', False)
>          allow_preconfig = expr.get('allow-preconfig', False)
>          ifcond = expr.get('if')
> +        features = expr.get('features', [])
>          if isinstance(data, OrderedDict):
>              data = self._make_implicit_object_type(
>                  name, info, doc, ifcond, 'arg', self._make_members(data, info))
> @@ -991,7 +1004,8 @@ class QAPISchema(object):
>              rets = self._make_array_type(rets[0], info)
>          self._def_entity(QAPISchemaCommand(name, info, doc, ifcond, data, rets,
>                                             gen, success_response,
> -                                           boxed, allow_oob, allow_preconfig))
> +                                           boxed, allow_oob, allow_preconfig,
> +                                           self._make_features(features, info)))
>
>      def _def_event(self, expr, info, doc):
>          name = expr['event']
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index 664254618a..e13c2e8671 100755
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -72,7 +72,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
>          self._print_if(ifcond)
>
>      def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
> -                      success_response, boxed, allow_oob, allow_preconfig):
> +                      success_response, boxed, allow_oob, allow_preconfig,
> +                      features):
>          print('command %s %s -> %s'
>                % (name, arg_type and arg_type.name,
>                   ret_type and ret_type.name))

v2 had:

           print('   gen=%s success_response=%s boxed=%s oob=%s preconfig=%s'
                 % (gen, success_response, boxed, allow_oob, allow_preconfig))
           self._print_if(ifcond)
  +        if features:
  +            for f in features:
  +                print('    feature %s' % f.name)
  +                self._print_if(f.ifcond, 8)

       def visit_event(self, name, info, ifcond, arg_type, boxed):
           print('event %s %s' % (name, arg_type and arg_type.name))

I see this is now in PATCH 2.  Okay.

Re: [PATCH v3 0/3] qapi: Add detection for the 'savevm' fix for blockdev

[Markus Armbruster]

I have two simple question on PATCH 2, two straightforward refactorings
to reduce code duplication, and one commit message tweak.

Answer my questions, and then we can decide whether we want a respin or
touch-ups in my tree.

Thanks!

Re: [PATCH v3 3/3] qapi: Allow introspecting fix for savevm's cooperation with blockdev

[Eric Blake]

On 10/10/19 6:05 AM, Peter Krempa wrote:

In addition to Markus' review,

> 'savevm' was buggy as it considered all monitor-owned block device nodes
> for snapshot. With introduction of -blockdev the common usage made all

With the introduction of -blockdev, the common usage

> nodes including protocol and backing file nodes monitor-owned and thus

nodes be monitor-owned

> considered for snapshot.
> 
> This is a problem since the 'file' protocol nodes can't have internal
> snapshots and it does not make sense to take snapshot of nodes
> representing backing files.
> 
> This was fixed by commit 05f4aced658a02b02 clients need to be able to
> detect whether this fix is present.
> 
> Since savevm does not have an QMP alternative, add the feature for the
> 'human-monitor-command' backdoor which is used to call this command in
> modern use.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>   qapi/misc.json | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 6bd11f50e6..5d0070aa43 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -1020,6 +1020,12 @@
>   #
>   # @cpu-index: The CPU to use for commands that require an implicit CPU
>   #
> +# Features:
> +# @savevm-blockdev-monitor-nodes: If present, the HMP command savevm only
> +#                                 snapshots monitor-owned nodes if they have no
> +#                                 parents. This allows to use 'savevm' with
> +#                                 -blockdev. (since 4.2)

s/to use/the use of/

> +#
>   # Returns: the output of the command as a string
>   #
>   # Since: 0.14.0
> @@ -1047,7 +1053,8 @@
>   ##
>   { 'command': 'human-monitor-command',
>     'data': {'command-line': 'str', '*cpu-index': 'int'},
> -  'returns': 'str' }
> +  'returns': 'str',
> +  'features': [ 'savevm-blockdev-monitor-nodes' ] }
> 
>   ##
>   # @change:
> 

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

Re: [PATCH v3 2/3] tests: qapi: Test 'features' of commands

[Peter Krempa]

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

On Thu, Oct 10, 2019 at 15:53:30 +0200, Markus Armbruster wrote:
> Peter Krempa <pkrempa@redhat.com> writes:
> 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >  tests/qapi-schema/qapi-schema-test.json | 26 ++++++++++++++++++++++
> >  tests/qapi-schema/qapi-schema-test.out  | 29 +++++++++++++++++++++++++
> >  tests/qapi-schema/test-qapi.py          |  4 ++++
> >  tests/test-qmp-cmds.c                   | 28 ++++++++++++++++++++++++
> >  4 files changed, 87 insertions(+)
> 
> More thorough than I asked for.  I'm not complaining :)
> 
> >
> > diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
> > index 75c42eb0e3..220859d4c9 100644
> > --- a/tests/qapi-schema/qapi-schema-test.json
> > +++ b/tests/qapi-schema/qapi-schema-test.json
> > @@ -290,3 +290,29 @@
> >              'cfs1': 'CondFeatureStruct1',
> >              'cfs2': 'CondFeatureStruct2',
> >              'cfs3': 'CondFeatureStruct3' } }
> > +
> > +# test 'features' for command
> > +
> > +{ 'command': 'test-command-features1',
> > +  'features': [] }
> > +
> > +{ 'command': 'test-command-features2',
> > +  'features': [ 'feature1' ] }
> > +
> > +{ 'command': 'test-command-features3',
> > +  'features': [ 'feature1', 'feature2' ] }
> > +
> > +{ 'command': 'test-command-features4',
> > +  'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'} ] }
> > +
> > +{ 'command': 'test-command-features5',
> > +  'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'},
> > +                { 'name': 'feature2', 'if': 'defined(TEST_IF_FEATURE_2)'} ] }
> > +
> > +{ 'command': 'test-command-features6',
> > +  'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'},
> > +                { 'name': 'feature2', 'if': 'defined(TEST_IF_FEATURE_2)'} ] }
> 
> Do you need both test-command-features5 and 6?  They look the same to me...

No. It can be dropped. Looks like I mistakenly appropriated
'CondFeatureStruct2' test twice :/

> > +{ 'command': 'test-command-features7',
> > +  '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 98031da96f..a38e348d54 100644
> > --- a/tests/qapi-schema/qapi-schema-test.out
> > +++ b/tests/qapi-schema/qapi-schema-test.out
> > @@ -412,3 +412,32 @@ object q_obj_test-features-arg
> >      member cfs3: CondFeatureStruct3 optional=False
> >  command test-features q_obj_test-features-arg -> None
> >     gen=True success_response=True boxed=False oob=False preconfig=False
> > +command test-command-features1 None -> None
> > +   gen=True success_response=True boxed=False oob=False preconfig=False
> > +command test-command-features2 None -> None
> > +   gen=True success_response=True boxed=False oob=False preconfig=False
> > +   feature feature1
> > +command test-command-features3 None -> None
> > +   gen=True success_response=True boxed=False oob=False preconfig=False
> > +   feature feature1
> > +   feature feature2
> > +command test-command-features4 None -> None
> > +   gen=True success_response=True boxed=False oob=False preconfig=False
> > +   feature feature1
> > +        if ['defined(TEST_IF_FEATURE_1)']
> > +command test-command-features5 None -> None
> > +   gen=True success_response=True boxed=False oob=False preconfig=False
> > +   feature feature1
> > +        if ['defined(TEST_IF_FEATURE_1)']
> > +   feature feature2
> > +        if ['defined(TEST_IF_FEATURE_2)']
> > +command test-command-features6 None -> None
> > +   gen=True success_response=True boxed=False oob=False preconfig=False
> > +   feature feature1
> > +        if ['defined(TEST_IF_FEATURE_1)']
> > +   feature feature2
> > +        if ['defined(TEST_IF_FEATURE_2)']
> > +command test-command-features7 None -> None
> > +   gen=True success_response=True boxed=False oob=False preconfig=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 e13c2e8671..62e65b6a7d 100755
> > --- a/tests/qapi-schema/test-qapi.py
> > +++ b/tests/qapi-schema/test-qapi.py
> > @@ -80,6 +80,10 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
> >          print('   gen=%s success_response=%s boxed=%s oob=%s preconfig=%s'
> >                % (gen, success_response, boxed, allow_oob, allow_preconfig))
> >          self._print_if(ifcond)
> > +        if features:
> > +            for f in features:
> > +                print('   feature %s' % f.name)
> > +                self._print_if(f.ifcond, 8)
> 
> Copied from visit_object_type().  Let's factor it into a @staticmethod
> _print_features().

I'm not sure if that's intentional but the 'struct' and 'command'
feature printers differ in indentation level by one space. I went for
aligning it with the 'gen' line above. I thought it's for visual
separation with other possible lines.

> >      def visit_event(self, name, info, ifcond, arg_type, boxed):
> >          print('event %s %s' % (name, arg_type and arg_type.name))
> > diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
> > index 36fdf5b115..19f6e06ba7 100644
> > --- a/tests/test-qmp-cmds.c
> > +++ b/tests/test-qmp-cmds.c
> > @@ -51,6 +51,34 @@ void qmp_test_features(FeatureStruct0 *fs0, FeatureStruct1 *fs1,
> >  {
> >  }
> >
> > +void qmp_test_command_features1(Error **errp)
> > +{
> > +}
> > +
> > +void qmp_test_command_features2(Error **errp)
> > +{
> > +}
> > +
> > +void qmp_test_command_features3(Error **errp)
> > +{
> > +}
> > +
> > +void qmp_test_command_features4(Error **errp)
> > +{
> > +}
> > +
> > +void qmp_test_command_features5(Error **errp)
> > +{
> > +}
> > +
> > +void qmp_test_command_features6(Error **errp)
> > +{
> > +}
> > +
> > +void qmp_test_command_features7(Error **errp)
> > +{
> > +}
> > +
> >  UserDefTwo *qmp_user_def_cmd2(UserDefOne *ud1a,
> >                                bool has_udb1, UserDefOne *ud1b,
> >                                Error **errp)
> 
> Any particular reason why we shouldn't squash this into PATCH 1?

Not really. I tend to prefer tests added separately and it was also done
so in case of features for 'structs' so I went with that approach. Said
that I'm perfectly fine with squashing them.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 2/3] tests: qapi: Test 'features' of commands

[Markus Armbruster]

Peter Krempa <pkrempa@redhat.com> writes:

> On Thu, Oct 10, 2019 at 15:53:30 +0200, Markus Armbruster wrote:
>> Peter Krempa <pkrempa@redhat.com> writes:
>> 
>> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>> > ---
>> >  tests/qapi-schema/qapi-schema-test.json | 26 ++++++++++++++++++++++
>> >  tests/qapi-schema/qapi-schema-test.out  | 29 +++++++++++++++++++++++++
>> >  tests/qapi-schema/test-qapi.py          |  4 ++++
>> >  tests/test-qmp-cmds.c                   | 28 ++++++++++++++++++++++++
>> >  4 files changed, 87 insertions(+)
>> 
>> More thorough than I asked for.  I'm not complaining :)
>> 
>> >
>> > diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
>> > index 75c42eb0e3..220859d4c9 100644
>> > --- a/tests/qapi-schema/qapi-schema-test.json
>> > +++ b/tests/qapi-schema/qapi-schema-test.json
>> > @@ -290,3 +290,29 @@
>> >              'cfs1': 'CondFeatureStruct1',
>> >              'cfs2': 'CondFeatureStruct2',
>> >              'cfs3': 'CondFeatureStruct3' } }
>> > +
>> > +# test 'features' for command
>> > +
>> > +{ 'command': 'test-command-features1',
>> > +  'features': [] }
>> > +
>> > +{ 'command': 'test-command-features2',
>> > +  'features': [ 'feature1' ] }
>> > +
>> > +{ 'command': 'test-command-features3',
>> > +  'features': [ 'feature1', 'feature2' ] }
>> > +
>> > +{ 'command': 'test-command-features4',
>> > +  'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'} ] }
>> > +
>> > +{ 'command': 'test-command-features5',
>> > +  'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'},
>> > +                { 'name': 'feature2', 'if': 'defined(TEST_IF_FEATURE_2)'} ] }
>> > +
>> > +{ 'command': 'test-command-features6',
>> > +  'features': [ { 'name': 'feature1', 'if': 'defined(TEST_IF_FEATURE_1)'},
>> > +                { 'name': 'feature2', 'if': 'defined(TEST_IF_FEATURE_2)'} ] }
>> 
>> Do you need both test-command-features5 and 6?  They look the same to me...
>
> No. It can be dropped. Looks like I mistakenly appropriated
> 'CondFeatureStruct2' test twice :/

Easy enough to fix :)

>> > +{ 'command': 'test-command-features7',
>> > +  '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 98031da96f..a38e348d54 100644
>> > --- a/tests/qapi-schema/qapi-schema-test.out
>> > +++ b/tests/qapi-schema/qapi-schema-test.out
>> > @@ -412,3 +412,32 @@ object q_obj_test-features-arg
>> >      member cfs3: CondFeatureStruct3 optional=False
>> >  command test-features q_obj_test-features-arg -> None
>> >     gen=True success_response=True boxed=False oob=False preconfig=False
>> > +command test-command-features1 None -> None
>> > +   gen=True success_response=True boxed=False oob=False preconfig=False
>> > +command test-command-features2 None -> None
>> > +   gen=True success_response=True boxed=False oob=False preconfig=False
>> > +   feature feature1
>> > +command test-command-features3 None -> None
>> > +   gen=True success_response=True boxed=False oob=False preconfig=False
>> > +   feature feature1
>> > +   feature feature2
>> > +command test-command-features4 None -> None
>> > +   gen=True success_response=True boxed=False oob=False preconfig=False
>> > +   feature feature1
>> > +        if ['defined(TEST_IF_FEATURE_1)']
>> > +command test-command-features5 None -> None
>> > +   gen=True success_response=True boxed=False oob=False preconfig=False
>> > +   feature feature1
>> > +        if ['defined(TEST_IF_FEATURE_1)']
>> > +   feature feature2
>> > +        if ['defined(TEST_IF_FEATURE_2)']
>> > +command test-command-features6 None -> None
>> > +   gen=True success_response=True boxed=False oob=False preconfig=False
>> > +   feature feature1
>> > +        if ['defined(TEST_IF_FEATURE_1)']
>> > +   feature feature2
>> > +        if ['defined(TEST_IF_FEATURE_2)']
>> > +command test-command-features7 None -> None
>> > +   gen=True success_response=True boxed=False oob=False preconfig=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 e13c2e8671..62e65b6a7d 100755
>> > --- a/tests/qapi-schema/test-qapi.py
>> > +++ b/tests/qapi-schema/test-qapi.py
>> > @@ -80,6 +80,10 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
>> >          print('   gen=%s success_response=%s boxed=%s oob=%s preconfig=%s'
>> >                % (gen, success_response, boxed, allow_oob, allow_preconfig))
>> >          self._print_if(ifcond)
>> > +        if features:
>> > +            for f in features:
>> > +                print('   feature %s' % f.name)
>> > +                self._print_if(f.ifcond, 8)
>> 
>> Copied from visit_object_type().  Let's factor it into a @staticmethod
>> _print_features().
>
> I'm not sure if that's intentional but the 'struct' and 'command'
> feature printers differ in indentation level by one space. I went for
> aligning it with the 'gen' line above. I thought it's for visual
> separation with other possible lines.

You're right.  I think I simply messed up the spacing in commit
156402e5042.  I'd like to clean it up.

I think it's easiest if I do the touch-ups here and in PATCH 1 (they're
all straightforward).  I'll post them as v4 so you can give them a quick
eye-over.

>> >      def visit_event(self, name, info, ifcond, arg_type, boxed):
>> >          print('event %s %s' % (name, arg_type and arg_type.name))
>> > diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
>> > index 36fdf5b115..19f6e06ba7 100644
>> > --- a/tests/test-qmp-cmds.c
>> > +++ b/tests/test-qmp-cmds.c
>> > @@ -51,6 +51,34 @@ void qmp_test_features(FeatureStruct0 *fs0, FeatureStruct1 *fs1,
>> >  {
>> >  }
>> >
>> > +void qmp_test_command_features1(Error **errp)
>> > +{
>> > +}
>> > +
>> > +void qmp_test_command_features2(Error **errp)
>> > +{
>> > +}
>> > +
>> > +void qmp_test_command_features3(Error **errp)
>> > +{
>> > +}
>> > +
>> > +void qmp_test_command_features4(Error **errp)
>> > +{
>> > +}
>> > +
>> > +void qmp_test_command_features5(Error **errp)
>> > +{
>> > +}
>> > +
>> > +void qmp_test_command_features6(Error **errp)
>> > +{
>> > +}
>> > +
>> > +void qmp_test_command_features7(Error **errp)
>> > +{
>> > +}
>> > +
>> >  UserDefTwo *qmp_user_def_cmd2(UserDefOne *ud1a,
>> >                                bool has_udb1, UserDefOne *ud1b,
>> >                                Error **errp)
>> 
>> Any particular reason why we shouldn't squash this into PATCH 1?
>
> Not really. I tend to prefer tests added separately and it was also done
> so in case of features for 'structs' so I went with that approach. Said
> that I'm perfectly fine with squashing them.

Okay.