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

[Kevin Wolf <kwolf@redhat.com>]

[-- 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 --]