QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/2] qapi: Add detection for the 'savevm' fix for blockdev
@ 2019-09-20 14:26 Peter Krempa
  2019-09-20 14:26 ` [PATCH v2 1/2] qapi: Add feature flags to commands in qapi introspection Peter Krempa
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Peter Krempa @ 2019-09-20 14:26 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, Michael Roth

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

v2:
 - fixed typos pointed out by Eric
 - added 'since' tag
 - reworded to describe the fix this allows to detect properly (Kevin)
 - verified that this can be rebased on top of Markus' series
   automatically

Peter Krempa (2):
  qapi: Add feature flags to commands in qapi introspection
  qapi: Allow introspecting fix for savevm's cooperation with blockdev

 docs/devel/qapi-code-gen.txt   |  4 ++--
 qapi/introspect.json           |  6 ++++-
 qapi/misc.json                 |  9 +++++++-
 scripts/qapi/commands.py       |  3 ++-
 scripts/qapi/common.py         | 40 +++++++++++++++++++++++++++++-----
 scripts/qapi/doc.py            |  3 ++-
 scripts/qapi/introspect.py     |  7 +++++-
 tests/qapi-schema/test-qapi.py |  7 +++++-
 8 files changed, 66 insertions(+), 13 deletions(-)

-- 
2.21.0



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 1/2] qapi: Add feature flags to commands in qapi introspection
  2019-09-20 14:26 [PATCH v2 0/2] qapi: Add detection for the 'savevm' fix for blockdev Peter Krempa
@ 2019-09-20 14:26 ` Peter Krempa
  2019-10-01  6:40   ` Markus Armbruster
  2019-10-01 20:01   ` Markus Armbruster
  2019-09-20 14:26 ` [PATCH v2 2/2] qapi: Allow introspecting fix for savevm's cooperation with blockdev Peter Krempa
  2019-09-30 13:04 ` [PATCH v2 0/2] qapi: Add detection for the 'savevm' fix for blockdev Peter Krempa
  2 siblings, 2 replies; 15+ messages in thread
From: Peter Krempa @ 2019-09-20 14:26 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, Michael Roth

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   |  4 ++--
 qapi/introspect.json           |  6 ++++-
 scripts/qapi/commands.py       |  3 ++-
 scripts/qapi/common.py         | 40 +++++++++++++++++++++++++++++-----
 scripts/qapi/doc.py            |  3 ++-
 scripts/qapi/introspect.py     |  7 +++++-
 tests/qapi-schema/test-qapi.py |  7 +++++-
 7 files changed, 58 insertions(+), 12 deletions(-)

diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index e8ec8ac1de..d465cc6330 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -726,8 +726,8 @@ 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
+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.

 In the schema, features can be specified as simple strings, for example:
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 b929e07be4..6cfe6cdd9e 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -276,7 +276,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/common.py b/scripts/qapi/common.py
index d61bfdc526..1502820f46 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -838,6 +838,7 @@ def check_type(info, source, value, allow_array=False,
 def check_command(expr, info):
     name = expr['command']
     boxed = expr.get('boxed', False)
+    features = expr.get('features')

     args_meta = ['struct']
     if boxed:
@@ -852,6 +853,19 @@ def check_command(expr, info):
                expr.get('returns'), allow_array=True,
                allow_optional=True, allow_metas=returns_meta)

+    if features:
+        if not isinstance(features, list):
+            raise QAPISemError(info,
+                               "Command '%s' requires an array for 'features'" %
+                               name)
+        for f in features:
+            assert isinstance(f, dict)
+            check_known_keys(info, "feature of command %s" % name, f,
+                             ['name'], ['if'])
+
+            check_if(f, info)
+            check_name(info, "Feature of command %s" % name, f['name'])
+

 def check_event(expr, info):
     name = expr['event']
@@ -1138,8 +1152,10 @@ def check_exprs(exprs):
             meta = 'command'
             check_keys(expr_elem, 'command', [],
                        ['data', 'returns', 'gen', 'success-response',
-                        'boxed', 'allow-oob', 'allow-preconfig', 'if'])
+                        'boxed', 'allow-oob', 'allow-preconfig', 'if',
+                        'features'])
             normalize_members(expr.get('data'))
+            normalize_features(expr.get('features'))
         elif 'event' in expr:
             meta = 'event'
             check_keys(expr_elem, 'event', [], ['data', 'boxed', 'if'])
@@ -1283,7 +1299,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):
@@ -1697,10 +1714,14 @@ class QAPISchemaAlternateType(QAPISchemaType):

 class QAPISchemaCommand(QAPISchemaEntity):
     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_owner(name)
         self._arg_type_name = arg_type
         self.arg_type = None
         self._ret_type_name = ret_type
@@ -1710,6 +1731,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)
@@ -1731,12 +1753,18 @@ class QAPISchemaCommand(QAPISchemaEntity):
             self.ret_type = schema.lookup_type(self._ret_type_name)
             assert isinstance(self.ret_type, QAPISchemaType)

+        # 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):
         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):
@@ -1989,6 +2017,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))
@@ -1997,7 +2026,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)))

     def _def_event(self, expr, info, doc):
         name = expr['event']
diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
index 5fc0fc7e06..aa4c557244 100755
--- a/scripts/qapi/doc.py
+++ b/scripts/qapi/doc.py
@@ -249,7 +249,8 @@ class QAPISchemaGenDocVisitor(qapi.common.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/introspect.py b/scripts/qapi/introspect.py
index f62cf0a2e1..36a5b195e5 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -206,13 +206,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/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index b0f770b9bd..def34aa489 100644
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -60,13 +60,18 @@ 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))
         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))
-- 
2.21.0



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2 2/2] qapi: Allow introspecting fix for savevm's cooperation with blockdev
  2019-09-20 14:26 [PATCH v2 0/2] qapi: Add detection for the 'savevm' fix for blockdev Peter Krempa
  2019-09-20 14:26 ` [PATCH v2 1/2] qapi: Add feature flags to commands in qapi introspection Peter Krempa
@ 2019-09-20 14:26 ` Peter Krempa
  2019-10-01 19:34   ` Markus Armbruster
  2019-09-30 13:04 ` [PATCH v2 0/2] qapi: Add detection for the 'savevm' fix for blockdev Peter Krempa
  2 siblings, 1 reply; 15+ messages in thread
From: Peter Krempa @ 2019-09-20 14:26 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, Michael Roth

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 nodes monitor owned and thus considered for
snapshot. This was fixed but 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..673445dfa9 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 savevm monitor command 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' : [ { 'name': 'savevm-blockdev-monitor-nodes' } ] }

 ##
 # @change:
-- 
2.21.0



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 0/2] qapi: Add detection for the 'savevm' fix for blockdev
  2019-09-20 14:26 [PATCH v2 0/2] qapi: Add detection for the 'savevm' fix for blockdev Peter Krempa
  2019-09-20 14:26 ` [PATCH v2 1/2] qapi: Add feature flags to commands in qapi introspection Peter Krempa
  2019-09-20 14:26 ` [PATCH v2 2/2] qapi: Allow introspecting fix for savevm's cooperation with blockdev Peter Krempa
@ 2019-09-30 13:04 ` Peter Krempa
  2 siblings, 0 replies; 15+ messages in thread
From: Peter Krempa @ 2019-09-30 13:04 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Markus Armbruster, Michael Roth

On Fri, Sep 20, 2019 at 16:26:43 +0200, Peter Krempa wrote:
> 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
> 
> v2:
>  - fixed typos pointed out by Eric
>  - added 'since' tag
>  - reworded to describe the fix this allows to detect properly (Kevin)
>  - verified that this can be rebased on top of Markus' series
>    automatically

Ping


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/2] qapi: Add feature flags to commands in qapi introspection
  2019-09-20 14:26 ` [PATCH v2 1/2] qapi: Add feature flags to commands in qapi introspection Peter Krempa
@ 2019-10-01  6:40   ` Markus Armbruster
  2019-10-01 14:17     ` Peter Krempa
  2019-10-01 20:01   ` Markus Armbruster
  1 sibling, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2019-10-01  6:40 UTC (permalink / raw)
  To: Peter Krempa; +Cc: Kevin Wolf, qemu-devel, Michael Roth

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>

+1 on adding features to commands.

Patch conflicts with the technical debt payback work I posted before and
after this series, although not nearly as badly as I expected.  It'll
conflict some more with the parts I haven't flushed.  The funny bit: I
went on that rampage in preparation of QAPI language extensions
including "features everywhere, not just structs".

I'll look into how to best fit your work into mine.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/2] qapi: Add feature flags to commands in qapi introspection
  2019-10-01  6:40   ` Markus Armbruster
@ 2019-10-01 14:17     ` Peter Krempa
  0 siblings, 0 replies; 15+ messages in thread
From: Peter Krempa @ 2019-10-01 14:17 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Kevin Wolf, qemu-devel, Michael Roth

On Tue, Oct 01, 2019 at 08:40:21 +0200, Markus Armbruster wrote:
> 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>
> 
> +1 on adding features to commands.
> 
> Patch conflicts with the technical debt payback work I posted before and
> after this series, although not nearly as badly as I expected.  It'll
> conflict some more with the parts I haven't flushed.  The funny bit: I
> went on that rampage in preparation of QAPI language extensions
> including "features everywhere, not just structs".
> 
> I'll look into how to best fit your work into mine.

Thanks. I have patches ready which depend on this to enable blockdev so
please let me know if the design changes somehow.


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/2] qapi: Allow introspecting fix for savevm's cooperation with blockdev
  2019-09-20 14:26 ` [PATCH v2 2/2] qapi: Allow introspecting fix for savevm's cooperation with blockdev Peter Krempa
@ 2019-10-01 19:34   ` Markus Armbruster
  2019-10-01 21:07     ` Eric Blake
  0 siblings, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2019-10-01 19:34 UTC (permalink / raw)
  To: Peter Krempa; +Cc: Kevin Wolf, qemu-devel, Michael Roth

Peter Krempa <pkrempa@redhat.com> writes:

> savevm was buggy as it considered all monitor owned block device nodes

Recommend "monitor-owned block device nodes" or "block device nodes
owned by a monitor"

> for snapshot. With introduction of -blockdev the common usage made all
> nodes including protocol nodes monitor owned and thus considered for
> snapshot.

What exactly is / was the problem?

>           This was fixed but clients need to be able to detect whether
> this fix is present.

Fixed where?  Commit hash, if possible.

>
> Since savevm does not have an QMP alternative add the feature for the

Comma after alternative.

> 'human-monitor-command' backdoor which is used to call this command in
> modern use.

Eww.  I don't have better ideas short of "design and implement a sane
QMP interface to internal snapshots", which is too much work.

> 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..673445dfa9 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 savevm monitor command only

Maybe s/the savevm monitor command/HMP command savevm/

> +#                                 snapshots monitor owned nodes if they have no

monitor-owned or owner by a monitor again.

> +#                                 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' : [ { 'name': 'savevm-blockdev-monitor-nodes' } ] }

No space before ':'.

Short form would do:

     'features': [ 'savevm-blockdev-monitor-nodes' ]

>
>  ##
>  # @change:


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/2] qapi: Add feature flags to commands in qapi introspection
  2019-09-20 14:26 ` [PATCH v2 1/2] qapi: Add feature flags to commands in qapi introspection Peter Krempa
  2019-10-01  6:40   ` Markus Armbruster
@ 2019-10-01 20:01   ` Markus Armbruster
  2019-10-02  6:15     ` Markus Armbruster
  1 sibling, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2019-10-01 20:01 UTC (permalink / raw)
  To: Peter Krempa; +Cc: Kevin Wolf, qemu-devel, Michael Roth

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   |  4 ++--
>  qapi/introspect.json           |  6 ++++-
>  scripts/qapi/commands.py       |  3 ++-
>  scripts/qapi/common.py         | 40 +++++++++++++++++++++++++++++-----
>  scripts/qapi/doc.py            |  3 ++-
>  scripts/qapi/introspect.py     |  7 +++++-
>  tests/qapi-schema/test-qapi.py |  7 +++++-
>  7 files changed, 58 insertions(+), 12 deletions(-)
>
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index e8ec8ac1de..d465cc6330 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -726,8 +726,8 @@ 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
> +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.

Limit line length to 70 characters, and double space after
sentence-ending period, please.

>
>  In the schema, features can be specified as simple strings, for example:
> 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 b929e07be4..6cfe6cdd9e 100644
> --- a/scripts/qapi/commands.py
> +++ b/scripts/qapi/commands.py
> @@ -276,7 +276,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/common.py b/scripts/qapi/common.py
> index d61bfdc526..1502820f46 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py

Conflicts with my "[PATCH 6/7] qapi: Split up scripts/qapi/common.py"
and also with work that has already landed in master.  Untested rebase
appended for your convenience.

> @@ -838,6 +838,7 @@ def check_type(info, source, value, allow_array=False,
>  def check_command(expr, info):
>      name = expr['command']
>      boxed = expr.get('boxed', False)
> +    features = expr.get('features')
>
>      args_meta = ['struct']
>      if boxed:
> @@ -852,6 +853,19 @@ def check_command(expr, info):
>                 expr.get('returns'), allow_array=True,
>                 allow_optional=True, allow_metas=returns_meta)
>
> +    if features:
> +        if not isinstance(features, list):
> +            raise QAPISemError(info,
> +                               "Command '%s' requires an array for 'features'" %
> +                               name)
> +        for f in features:
> +            assert isinstance(f, dict)
> +            check_known_keys(info, "feature of command %s" % name, f,
> +                             ['name'], ['if'])
> +
> +            check_if(f, info)
> +            check_name(info, "Feature of command %s" % name, f['name'])
> +

Copied from check_struct() with minor edits.

Conflicts semantically with commit eeb57c85da "qapi: Avoid redundant
definition references in error messages", which changes the code you
copy in a way that makes editing it superfluous.  Please factor it into
a helper function check_features(features, info).

>
>  def check_event(expr, info):
>      name = expr['event']
> @@ -1138,8 +1152,10 @@ def check_exprs(exprs):
>              meta = 'command'
>              check_keys(expr_elem, 'command', [],
>                         ['data', 'returns', 'gen', 'success-response',
> -                        'boxed', 'allow-oob', 'allow-preconfig', 'if'])
> +                        'boxed', 'allow-oob', 'allow-preconfig', 'if',
> +                        'features'])
>              normalize_members(expr.get('data'))
> +            normalize_features(expr.get('features'))
>          elif 'event' in expr:
>              meta = 'event'
>              check_keys(expr_elem, 'event', [], ['data', 'boxed', 'if'])
> @@ -1283,7 +1299,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):
> @@ -1697,10 +1714,14 @@ class QAPISchemaAlternateType(QAPISchemaType):
>
>  class QAPISchemaCommand(QAPISchemaEntity):
>      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_owner(name)

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

>          self._arg_type_name = arg_type
>          self.arg_type = None
>          self._ret_type_name = ret_type
> @@ -1710,6 +1731,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)
> @@ -1731,12 +1753,18 @@ class QAPISchemaCommand(QAPISchemaEntity):
>              self.ret_type = schema.lookup_type(self._ret_type_name)
>              assert isinstance(self.ret_type, QAPISchemaType)
>
> +        # 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):
>          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):
> @@ -1989,6 +2017,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))
> @@ -1997,7 +2026,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)))
>
>      def _def_event(self, expr, info, doc):
>          name = expr['event']
> diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
> index 5fc0fc7e06..aa4c557244 100755
> --- a/scripts/qapi/doc.py
> +++ b/scripts/qapi/doc.py
> @@ -249,7 +249,8 @@ class QAPISchemaGenDocVisitor(qapi.common.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/introspect.py b/scripts/qapi/introspect.py
> index f62cf0a2e1..36a5b195e5 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -206,13 +206,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/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index b0f770b9bd..def34aa489 100644
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -60,13 +60,18 @@ 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))
>          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))

Copied from visit_object_type().  Would you mind factoring it into a
@staticmethod _print_features()?

No tests.  Please add at least a positive test case to
qapi-schema-test.json.


From c55b056497b79e922d0489cf2dfe4d2a5edb62c8 Mon Sep 17 00:00:00 2001
From: Markus Armbruster <armbru@redhat.com>
Date: Tue, 1 Oct 2019 20:47:04 +0200
Subject: [PATCH 1/2] qapi: Add feature flags to commands in qapi

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 |  7 ++++++-
 8 files changed, 59 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..5c98d31941 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,
+                               "Command '%s' requires an array for 'features'" %
+                               name)
+        for f in features:
+            assert isinstance(f, dict)
+            check_known_keys(info, "feature of command %s" % name, f,
+                             ['name'], ['if'])
+
+            check_if(f, info)
+            check_name(info, "Feature of command %s" % name, f['name'])
+
 
 def check_event(expr, info):
     args = expr.get('data')
@@ -357,7 +371,7 @@ 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'))
@@ -366,6 +380,7 @@ def check_exprs(exprs):
             check_keys(expr, info, meta,
                        ['event'], ['data', 'boxed', 'if'])
             normalize_members(expr.get('data'))
+            normalize_features(expr.get('features'))
             check_event(expr, info)
         else:
             assert False, 'unexpected meta type'
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..dd0fdc5ced 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_owner(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..fc41d24bb9 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -72,13 +72,18 @@ 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))
         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))
-- 
2.21.0



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/2] qapi: Allow introspecting fix for savevm's cooperation with blockdev
  2019-10-01 19:34   ` Markus Armbruster
@ 2019-10-01 21:07     ` Eric Blake
  2019-10-02 11:57       ` Markus Armbruster
  0 siblings, 1 reply; 15+ messages in thread
From: Eric Blake @ 2019-10-01 21:07 UTC (permalink / raw)
  To: Markus Armbruster, Peter Krempa; +Cc: Kevin Wolf, qemu-devel, Michael Roth

On 10/1/19 2:34 PM, Markus Armbruster wrote:
> Peter Krempa <pkrempa@redhat.com> writes:
> 
>> savevm was buggy as it considered all monitor owned block device nodes
> 
> Recommend "monitor-owned block device nodes" or "block device nodes
> owned by a monitor"
> 
>> for snapshot. With introduction of -blockdev the common usage made all
>> nodes including protocol nodes monitor owned and thus considered for
>> snapshot.
> 
> What exactly is / was the problem?


Old way: using QMP add_device, you create a drive backend with two BDS 
(format and protocol) assigned to it; the drive backend has your given 
name, and both BDS have a generated name (beginning with '#').  The two 
BDS are not monitor-owned, rather, the drive is.

New way: using QMP blockdev_add, you create the two BDS manually with 
names of your choice, then plug that blockdev into an unnamed 
blockbackend (the drive no longer needs a name, because you can get at 
everything through the BDS name).  You _could_ do this in one step (the 
QAPI allows self-recursion where you can define both the format and 
protocol in one step), but it is easier to do in two steps (define the 
protocol BDS first, then define the format BDS using a "string" name of 
the protocol BDS instead of a { "driver":..., args... } object of the 
protocol layer.  But by making two calls,  now both BDS are monitor-owned.

At snapshot-time, the code currently looks for all monitor-owned nodes 
when deciding what to snapshot.  In the old way, this finds the named 
drive, picks up its associated top-most node, and snapshots the format 
layer.  In the new way, the drive is unnamed so it is skipped, while 
there are two named BDS, but we don't want a snapshot of the protocol layer.


> 
>>            This was fixed but clients need to be able to detect whether
>> this fix is present.
> 
> Fixed where?  Commit hash, if possible.

Pull request: 
https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04773.html
(assuming it doesn't need a respin before landing, 8ec72832)

> 
>>
>> Since savevm does not have an QMP alternative add the feature for the
> 
> Comma after alternative.
> 
>> 'human-monitor-command' backdoor which is used to call this command in
>> modern use.
> 
> Eww.  I don't have better ideas short of "design and implement a sane
> QMP interface to internal snapshots", which is too much work.
> 
>> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>> ---
>>   qapi/misc.json | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 1/2] qapi: Add feature flags to commands in qapi introspection
  2019-10-01 20:01   ` Markus Armbruster
@ 2019-10-02  6:15     ` Markus Armbruster
  0 siblings, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2019-10-02  6:15 UTC (permalink / raw)
  To: Peter Krempa; +Cc: Kevin Wolf, qemu-devel, Michael Roth

Markus Armbruster <armbru@redhat.com> writes:

> 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>
[...]
>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
>> index d61bfdc526..1502820f46 100644
>> --- a/scripts/qapi/common.py
>> +++ b/scripts/qapi/common.py
>
> Conflicts with my "[PATCH 6/7] qapi: Split up scripts/qapi/common.py"
> and also with work that has already landed in master.  Untested rebase
> appended for your convenience.

I appended a stale version, sorry.  Have a fresh one.

[...]
> No tests.  Please add at least a positive test case to
> qapi-schema-test.json.

Adding a feature to some existing command should suffice.



From 4c216a45764be91b660f26c866352e5cab7b20ee Mon Sep 17 00:00:00 2001
From: Markus Armbruster <armbru@redhat.com>
Date: Tue, 1 Oct 2019 20:47:04 +0200
Subject: [PATCH 1/2] qapi: Add feature flags to commands in qapi

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 |  7 ++++++-
 8 files changed, 59 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..3e1a49e9af 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,7 +371,7 @@ 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'))
@@ -366,6 +380,7 @@ def check_exprs(exprs):
             check_keys(expr, info, meta,
                        ['event'], ['data', 'boxed', 'if'])
             normalize_members(expr.get('data'))
+            normalize_features(expr.get('features'))
             check_event(expr, info)
         else:
             assert False, 'unexpected meta type'
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..fc41d24bb9 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -72,13 +72,18 @@ 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))
         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))
-- 
2.21.0



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/2] qapi: Allow introspecting fix for savevm's cooperation with blockdev
  2019-10-01 21:07     ` Eric Blake
@ 2019-10-02 11:57       ` Markus Armbruster
  2019-10-10 15:07         ` Kevin Wolf
  0 siblings, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2019-10-02 11:57 UTC (permalink / raw)
  To: Eric Blake; +Cc: Kevin Wolf, Peter Krempa, qemu-devel, Michael Roth

Eric Blake <eblake@redhat.com> writes:

> On 10/1/19 2:34 PM, Markus Armbruster wrote:
>> Peter Krempa <pkrempa@redhat.com> writes:
>>
>>> savevm was buggy as it considered all monitor owned block device nodes
>>
>> Recommend "monitor-owned block device nodes" or "block device nodes
>> owned by a monitor"
>>
>>> for snapshot. With introduction of -blockdev the common usage made all
>>> nodes including protocol nodes monitor owned and thus considered for
>>> snapshot.
>>
>> What exactly is / was the problem?
>
>
> Old way: using QMP add_device, you create a drive backend with two BDS
> (format and protocol) assigned to it; the drive backend has your given
> name, and both BDS have a generated name (beginning with '#').  The
> two BDS are not monitor-owned, rather, the drive is.
>
> New way: using QMP blockdev_add, you create the two BDS manually with
> names of your choice, then plug that blockdev into an unnamed
> blockbackend (the drive no longer needs a name, because you can get at
> everything through the BDS name).  You _could_ do this in one step
> (the QAPI allows self-recursion where you can define both the format
> and protocol in one step), but it is easier to do in two steps (define
> the protocol BDS first, then define the format BDS using a "string"
> name of the protocol BDS instead of a { "driver":..., args... } object
> of the protocol layer.  But by making two calls,  now both BDS are
> monitor-owned.
>
> At snapshot-time, the code currently looks for all monitor-owned nodes
> when deciding what to snapshot.  In the old way, this finds the named
> drive, picks up its associated top-most node, and snapshots the format
> layer.  In the new way, the drive is unnamed so it is skipped, while
> there are two named BDS, but we don't want a snapshot of the protocol
> layer.

So the problem is certain (common & sane) -blockdev use makes savevm
create additional, unwanted snapshots.

Your explanation should be worked into the commit message along with ...

>>>            This was fixed but clients need to be able to detect whether
>>> this fix is present.
>>
>> Fixed where?  Commit hash, if possible.
>
> Pull request:
> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04773.html
> (assuming it doesn't need a respin before landing, 8ec72832)

... a pointer to this fix.

Thanks!

[...]


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/2] qapi: Allow introspecting fix for savevm's cooperation with blockdev
  2019-10-02 11:57       ` Markus Armbruster
@ 2019-10-10 15:07         ` Kevin Wolf
  2019-10-11  6:08           ` Markus Armbruster
  0 siblings, 1 reply; 15+ messages in thread
From: Kevin Wolf @ 2019-10-10 15:07 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Peter Krempa, qemu-devel, Michael Roth

Am 02.10.2019 um 13:57 hat Markus Armbruster geschrieben:
> Eric Blake <eblake@redhat.com> writes:
> 
> > On 10/1/19 2:34 PM, Markus Armbruster wrote:
> >> Peter Krempa <pkrempa@redhat.com> writes:
> >>
> >>> savevm was buggy as it considered all monitor owned block device nodes
> >>
> >> Recommend "monitor-owned block device nodes" or "block device nodes
> >> owned by a monitor"
> >>
> >>> for snapshot. With introduction of -blockdev the common usage made all
> >>> nodes including protocol nodes monitor owned and thus considered for
> >>> snapshot.
> >>
> >> What exactly is / was the problem?
> >
> >
> > Old way: using QMP add_device, you create a drive backend with two BDS
> > (format and protocol) assigned to it; the drive backend has your given
> > name, and both BDS have a generated name (beginning with '#').  The
> > two BDS are not monitor-owned, rather, the drive is.
> >
> > New way: using QMP blockdev_add, you create the two BDS manually with
> > names of your choice, then plug that blockdev into an unnamed
> > blockbackend (the drive no longer needs a name, because you can get at
> > everything through the BDS name).  You _could_ do this in one step
> > (the QAPI allows self-recursion where you can define both the format
> > and protocol in one step), but it is easier to do in two steps (define
> > the protocol BDS first, then define the format BDS using a "string"
> > name of the protocol BDS instead of a { "driver":..., args... } object
> > of the protocol layer.  But by making two calls,  now both BDS are
> > monitor-owned.
> >
> > At snapshot-time, the code currently looks for all monitor-owned nodes
> > when deciding what to snapshot.  In the old way, this finds the named
> > drive, picks up its associated top-most node, and snapshots the format
> > layer.  In the new way, the drive is unnamed so it is skipped, while
> > there are two named BDS, but we don't want a snapshot of the protocol
> > layer.
> 
> So the problem is certain (common & sane) -blockdev use makes savevm
> create additional, unwanted snapshots.

Actually, the most common protocol driver is file-posix, which doesn't
support snapshots, so usually the result was that savevm just fails
because it can't snapshot something that it (incorrectly) thinks it
should snapshot.

Kevin

> Your explanation should be worked into the commit message along with ...
> 
> >>>            This was fixed but clients need to be able to detect whether
> >>> this fix is present.
> >>
> >> Fixed where?  Commit hash, if possible.
> >
> > Pull request:
> > https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04773.html
> > (assuming it doesn't need a respin before landing, 8ec72832)
> 
> ... a pointer to this fix.
> 
> Thanks!
> 
> [...]


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/2] qapi: Allow introspecting fix for savevm's cooperation with blockdev
  2019-10-10 15:07         ` Kevin Wolf
@ 2019-10-11  6:08           ` Markus Armbruster
  2019-10-11  9:00             ` Kevin Wolf
  0 siblings, 1 reply; 15+ messages in thread
From: Markus Armbruster @ 2019-10-11  6:08 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: Peter Krempa, qemu-devel, Michael Roth

Kevin Wolf <kwolf@redhat.com> writes:

> Am 02.10.2019 um 13:57 hat Markus Armbruster geschrieben:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>> > On 10/1/19 2:34 PM, Markus Armbruster wrote:
>> >> Peter Krempa <pkrempa@redhat.com> writes:
>> >>
>> >>> savevm was buggy as it considered all monitor owned block device nodes
>> >>
>> >> Recommend "monitor-owned block device nodes" or "block device nodes
>> >> owned by a monitor"
>> >>
>> >>> for snapshot. With introduction of -blockdev the common usage made all
>> >>> nodes including protocol nodes monitor owned and thus considered for
>> >>> snapshot.
>> >>
>> >> What exactly is / was the problem?
>> >
>> >
>> > Old way: using QMP add_device, you create a drive backend with two BDS
>> > (format and protocol) assigned to it; the drive backend has your given
>> > name, and both BDS have a generated name (beginning with '#').  The
>> > two BDS are not monitor-owned, rather, the drive is.
>> >
>> > New way: using QMP blockdev_add, you create the two BDS manually with
>> > names of your choice, then plug that blockdev into an unnamed
>> > blockbackend (the drive no longer needs a name, because you can get at
>> > everything through the BDS name).  You _could_ do this in one step
>> > (the QAPI allows self-recursion where you can define both the format
>> > and protocol in one step), but it is easier to do in two steps (define
>> > the protocol BDS first, then define the format BDS using a "string"
>> > name of the protocol BDS instead of a { "driver":..., args... } object
>> > of the protocol layer.  But by making two calls,  now both BDS are
>> > monitor-owned.
>> >
>> > At snapshot-time, the code currently looks for all monitor-owned nodes
>> > when deciding what to snapshot.  In the old way, this finds the named
>> > drive, picks up its associated top-most node, and snapshots the format
>> > layer.  In the new way, the drive is unnamed so it is skipped, while
>> > there are two named BDS, but we don't want a snapshot of the protocol
>> > layer.
>> 
>> So the problem is certain (common & sane) -blockdev use makes savevm
>> create additional, unwanted snapshots.
>
> Actually, the most common protocol driver is file-posix, which doesn't
> support snapshots, so usually the result was that savevm just fails
> because it can't snapshot something that it (incorrectly) thinks it
> should snapshot.

v3's commit message:

    qapi: Allow introspecting fix for savevm's cooperation with blockdev
    
    '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>
 
Kevin, is this explanation sufficiently correct & complete?


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/2] qapi: Allow introspecting fix for savevm's cooperation with blockdev
  2019-10-11  6:08           ` Markus Armbruster
@ 2019-10-11  9:00             ` Kevin Wolf
  2019-10-11 11:10               ` Markus Armbruster
  0 siblings, 1 reply; 15+ messages in thread
From: Kevin Wolf @ 2019-10-11  9:00 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Peter Krempa, qemu-devel, Michael Roth

Am 11.10.2019 um 08:08 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 02.10.2019 um 13:57 hat Markus Armbruster geschrieben:
> >> Eric Blake <eblake@redhat.com> writes:
> >> 
> >> > On 10/1/19 2:34 PM, Markus Armbruster wrote:
> >> >> Peter Krempa <pkrempa@redhat.com> writes:
> >> >>
> >> >>> savevm was buggy as it considered all monitor owned block device nodes
> >> >>
> >> >> Recommend "monitor-owned block device nodes" or "block device nodes
> >> >> owned by a monitor"
> >> >>
> >> >>> for snapshot. With introduction of -blockdev the common usage made all
> >> >>> nodes including protocol nodes monitor owned and thus considered for
> >> >>> snapshot.
> >> >>
> >> >> What exactly is / was the problem?
> >> >
> >> >
> >> > Old way: using QMP add_device, you create a drive backend with two BDS
> >> > (format and protocol) assigned to it; the drive backend has your given
> >> > name, and both BDS have a generated name (beginning with '#').  The
> >> > two BDS are not monitor-owned, rather, the drive is.
> >> >
> >> > New way: using QMP blockdev_add, you create the two BDS manually with
> >> > names of your choice, then plug that blockdev into an unnamed
> >> > blockbackend (the drive no longer needs a name, because you can get at
> >> > everything through the BDS name).  You _could_ do this in one step
> >> > (the QAPI allows self-recursion where you can define both the format
> >> > and protocol in one step), but it is easier to do in two steps (define
> >> > the protocol BDS first, then define the format BDS using a "string"
> >> > name of the protocol BDS instead of a { "driver":..., args... } object
> >> > of the protocol layer.  But by making two calls,  now both BDS are
> >> > monitor-owned.
> >> >
> >> > At snapshot-time, the code currently looks for all monitor-owned nodes
> >> > when deciding what to snapshot.  In the old way, this finds the named
> >> > drive, picks up its associated top-most node, and snapshots the format
> >> > layer.  In the new way, the drive is unnamed so it is skipped, while
> >> > there are two named BDS, but we don't want a snapshot of the protocol
> >> > layer.
> >> 
> >> So the problem is certain (common & sane) -blockdev use makes savevm
> >> create additional, unwanted snapshots.
> >
> > Actually, the most common protocol driver is file-posix, which doesn't
> > support snapshots, so usually the result was that savevm just fails
> > because it can't snapshot something that it (incorrectly) thinks it
> > should snapshot.
> 
> v3's commit message:
> 
>     qapi: Allow introspecting fix for savevm's cooperation with blockdev
>     
>     '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.

Something is missing in this sentence. I think you lost the "but" from
the original message.

>     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>
>  
> Kevin, is this explanation sufficiently correct & complete?

Looks good to me otherwise.

Kevin


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2 2/2] qapi: Allow introspecting fix for savevm's cooperation with blockdev
  2019-10-11  9:00             ` Kevin Wolf
@ 2019-10-11 11:10               ` Markus Armbruster
  0 siblings, 0 replies; 15+ messages in thread
From: Markus Armbruster @ 2019-10-11 11:10 UTC (permalink / raw)
  To: Kevin Wolf; +Cc: Peter Krempa, qemu-devel, Michael Roth

Kevin Wolf <kwolf@redhat.com> writes:

> Am 11.10.2019 um 08:08 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Am 02.10.2019 um 13:57 hat Markus Armbruster geschrieben:
[...]
>> >> So the problem is certain (common & sane) -blockdev use makes savevm
>> >> create additional, unwanted snapshots.
>> >
>> > Actually, the most common protocol driver is file-posix, which doesn't
>> > support snapshots, so usually the result was that savevm just fails
>> > because it can't snapshot something that it (incorrectly) thinks it
>> > should snapshot.
>> 
>> v3's commit message:
>> 
>>     qapi: Allow introspecting fix for savevm's cooperation with blockdev
>>     
>>     '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.
>
> Something is missing in this sentence. I think you lost the "but" from
> the original message.

I fixed this in v4 by inserting a period.  I wasn't aware we had lost a
"but".

>>     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>
>>  
>> Kevin, is this explanation sufficiently correct & complete?
>
> Looks good to me otherwise.

Thanks!


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-20 14:26 [PATCH v2 0/2] qapi: Add detection for the 'savevm' fix for blockdev Peter Krempa
2019-09-20 14:26 ` [PATCH v2 1/2] qapi: Add feature flags to commands in qapi introspection Peter Krempa
2019-10-01  6:40   ` Markus Armbruster
2019-10-01 14:17     ` Peter Krempa
2019-10-01 20:01   ` Markus Armbruster
2019-10-02  6:15     ` Markus Armbruster
2019-09-20 14:26 ` [PATCH v2 2/2] qapi: Allow introspecting fix for savevm's cooperation with blockdev Peter Krempa
2019-10-01 19:34   ` Markus Armbruster
2019-10-01 21:07     ` Eric Blake
2019-10-02 11:57       ` Markus Armbruster
2019-10-10 15:07         ` Kevin Wolf
2019-10-11  6:08           ` Markus Armbruster
2019-10-11  9:00             ` Kevin Wolf
2019-10-11 11:10               ` Markus Armbruster
2019-09-30 13:04 ` [PATCH v2 0/2] qapi: Add detection for the 'savevm' fix for blockdev Peter Krempa

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org qemu-devel@archiver.kernel.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox