[Qemu-devel] [PATCH 0/2] qapi: Add detection for the 'savevm' fix for blockdev

[Qemu-devel] [PATCH 0/2] 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

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                 |  8 ++++++-
 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, 65 insertions(+), 13 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH 1/2] qapi: Add feature flags to commands in qapi introspection

[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 an 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..38682daace 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 string, where each string
 signals that this build of QEMU shows a certain behaviour.

 In the schema, features can be specified as simple strings, for example:
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

Re: [Qemu-devel] [PATCH 1/2] qapi: Add feature flags to commands in qapi introspection

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 1972 bytes --]

On 9/17/19 10:49 AM, Peter Krempa wrote:
> 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 an command which may not be

s/ an / a /

> 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 ++--

May be some rebase churn needed here as Markus has been reworking that file:
https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg02959.html

>  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..38682daace 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 string, where each string

Pre-existing, but "list of strings" or "list of string entries"

>  signals that this build of QEMU shows a certain behaviour.
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

[Qemu-devel] [PATCH 2/2] 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 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 | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/qapi/misc.json b/qapi/misc.json
index 6bd11f50e6..e2b33c3f8a 100644
--- a/qapi/misc.json
+++ b/qapi/misc.json
@@ -1020,6 +1020,11 @@
 #
 # @cpu-index: The CPU to use for commands that require an implicit CPU
 #
+# Features:
+# @savevm-blockdev-monitor-nodes: If present, the savevm monitor command
+#                                 correctly handles monitor owned block nodes
+#                                 when taking a snapshot.
+#
 # Returns: the output of the command as a string
 #
 # Since: 0.14.0
@@ -1047,7 +1052,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

Re: [Qemu-devel] [PATCH 2/2] qapi: Allow introspecting fix for savevm's cooperation with blockdev

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 1882 bytes --]

On 9/17/19 10:49 AM, Peter Krempa wrote:
> 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 | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/misc.json b/qapi/misc.json
> index 6bd11f50e6..e2b33c3f8a 100644
> --- a/qapi/misc.json
> +++ b/qapi/misc.json
> @@ -1020,6 +1020,11 @@
>  #
>  # @cpu-index: The CPU to use for commands that require an implicit CPU
>  #
> +# Features:
> +# @savevm-blockdev-monitor-nodes: If present, the savevm monitor command
> +#                                 correctly handles monitor owned block nodes
> +#                                 when taking a snapshot.

Is it worth adding a '(since 4.2)' on when features are added?

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

We could, of course, actually implement a QMP 'savevm' and use _that_ as
the introspection.  But that's a bigger can of worms, so this is
reasonable enough for the 4.2 timeframe.

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


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

Re: [Qemu-devel] [PATCH 2/2] qapi: Allow introspecting fix for savevm's cooperation with blockdev

[Kevin Wolf]

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

Am 17.09.2019 um 18:33 hat Eric Blake geschrieben:
> On 9/17/19 10:49 AM, Peter Krempa wrote:
> > 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 | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qapi/misc.json b/qapi/misc.json
> > index 6bd11f50e6..e2b33c3f8a 100644
> > --- a/qapi/misc.json
> > +++ b/qapi/misc.json
> > @@ -1020,6 +1020,11 @@
> >  #
> >  # @cpu-index: The CPU to use for commands that require an implicit CPU
> >  #
> > +# Features:
> > +# @savevm-blockdev-monitor-nodes: If present, the savevm monitor command
> > +#                                 correctly handles monitor owned block nodes
> > +#                                 when taking a snapshot.
> 
> Is it worth adding a '(since 4.2)' on when features are added?

I would also rather describe the change in behaviour ("only includes
monitor owned block nodes if they have no parents") than saying that the
behaviour is now correct.

(Not the least because we know that the current way still isn't quite
correct, just hopefully correct enough: Block job BlockBackends are
currently snapshotted, which is unintended and will be changed in the
future. However, in practice people probably won't use block jobs on
non-root nodes and internal snapshots together.)

> > +#
> >  # Returns: the output of the command as a string
> >  #
> >  # Since: 0.14.0
> > @@ -1047,7 +1052,8 @@
> >  ##
> >  { 'command': 'human-monitor-command',
> >    'data': {'command-line': 'str', '*cpu-index': 'int'},
> > -  'returns': 'str' }
> > +  'returns': 'str',
> > +  'features' : [ { 'name': 'savevm-blockdev-monitor-nodes' } ] }
> 
> We could, of course, actually implement a QMP 'savevm' and use _that_ as
> the introspection.  But that's a bigger can of worms, so this is
> reasonable enough for the 4.2 timeframe.

I think a QMP savevm would sidestep the whole issue by taking an
explicit list of nodes to snapshot, and an explicit option to tell which
node to store the vmstate on.

Kevin

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

Re: [Qemu-devel] [PATCH 2/2] qapi: Allow introspecting fix for savevm's cooperation with blockdev

[Peter Krempa]

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

On Wed, Sep 18, 2019 at 10:22:13 +0200, Kevin Wolf wrote:
> Am 17.09.2019 um 18:33 hat Eric Blake geschrieben:
> > On 9/17/19 10:49 AM, Peter Krempa wrote:
> > > 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 | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/qapi/misc.json b/qapi/misc.json
> > > index 6bd11f50e6..e2b33c3f8a 100644
> > > --- a/qapi/misc.json
> > > +++ b/qapi/misc.json
> > > @@ -1020,6 +1020,11 @@
> > >  #
> > >  # @cpu-index: The CPU to use for commands that require an implicit CPU
> > >  #
> > > +# Features:
> > > +# @savevm-blockdev-monitor-nodes: If present, the savevm monitor command
> > > +#                                 correctly handles monitor owned block nodes
> > > +#                                 when taking a snapshot.
> > 
> > Is it worth adding a '(since 4.2)' on when features are added?
> 
> I would also rather describe the change in behaviour ("only includes
> monitor owned block nodes if they have no parents") than saying that the
> behaviour is now correct.

That's a good idea. I'll post it in a v2 if required.

> (Not the least because we know that the current way still isn't quite
> correct, just hopefully correct enough: Block job BlockBackends are
> currently snapshotted, which is unintended and will be changed in the
> future. However, in practice people probably won't use block jobs on
> non-root nodes and internal snapshots together.)
> 
> > > +#
> > >  # Returns: the output of the command as a string
> > >  #
> > >  # Since: 0.14.0
> > > @@ -1047,7 +1052,8 @@
> > >  ##
> > >  { 'command': 'human-monitor-command',
> > >    'data': {'command-line': 'str', '*cpu-index': 'int'},
> > > -  'returns': 'str' }
> > > +  'returns': 'str',
> > > +  'features' : [ { 'name': 'savevm-blockdev-monitor-nodes' } ] }
> > 
> > We could, of course, actually implement a QMP 'savevm' and use _that_ as
> > the introspection.  But that's a bigger can of worms, so this is
> > reasonable enough for the 4.2 timeframe.
> 
> I think a QMP savevm would sidestep the whole issue by taking an
> explicit list of nodes to snapshot, and an explicit option to tell which
> node to store the vmstate on.

A straight replacement for savevm would be quite pointless and also such
discussion took already place multiple times in the past. The result
always was that we need something better than just a qmp version of
savevm.

I'll be happy to implement the libvirt support for the "new" savevm if
it ever appears.

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