Re: [Qemu-devel] [PATCH 2/8] qmp: create qmp_savevm command

[Denis V. Lunev]

On 01/18/2016 06:58 PM, Markus Armbruster wrote:
> "Denis V. Lunev" <address@hidden> writes:
>
> > 'name' attribute is made mandatory in distinction with HMP command.
> >
> > The patch also moves hmp_savevm implementation into hmp.c. This function
> > is just a simple wrapper now and does not have knowledge about
> > migration internals.
> >
> > Signed-off-by: Denis V. Lunev <address@hidden>
> > CC: Juan Quintela <address@hidden>
> > CC: Amit Shah <address@hidden>
> > CC: Markus Armbruster <address@hidden>
> > CC: Eric Blake <address@hidden>
> > ---
> >   hmp.c                         |  3 +--
> >   include/migration/migration.h |  2 --
> >   migration/savevm.c            |  2 +-
> >   qapi-schema.json              | 13 +++++++++++++
> >   qmp-commands.hx               | 25 +++++++++++++++++++++++++
> >   5 files changed, 40 insertions(+), 5 deletions(-)
> >
> > diff --git a/hmp.c b/hmp.c
> > index e7bab75..8d6eda6 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -33,7 +33,6 @@
> >   #include "ui/console.h"
> >   #include "block/qapi.h"
> >   #include "qemu-io.h"
> > -#include "migration/migration.h"
> >   
> >   #ifdef CONFIG_SPICE
> >   #include <spice/enums.h>
> > @@ -2406,7 +2405,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
> >           name = name_buf;
> >       }
> >   
> > -    migrate_savevm(name, &local_err);
> > +    qmp_savevm(name, &local_err);
> >   
> >       if (local_err != NULL) {
> >           error_report_err(local_err);
> > diff --git a/include/migration/migration.h b/include/migration/migration.h
> > index 73c8bb1..58c04a9 100644
> > --- a/include/migration/migration.h
> > +++ b/include/migration/migration.h
> > @@ -277,8 +277,6 @@ int migrate_compress_threads(void);
> >   int migrate_decompress_threads(void);
> >   bool migrate_use_events(void);
> >   
> > -void migrate_savevm(const char *name, Error **errp);
> > -
> >   /* Sending on the return path - generic and then for each message type */
> >   void migrate_send_rp_message(MigrationIncomingState *mis,
> >                                enum mig_rp_message_type message_type,
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index 308302a..0dbb15f 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -1905,7 +1905,7 @@ int qemu_loadvm_state(QEMUFile *f)
> >       return ret;
> >   }
> >   
> > -void migrate_savevm(const char *name, Error **errp)
> > +void qmp_savevm(const char *name, Error **errp)
> >   {
> >       BlockDriverState *bs, *bs1;
> >       QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 2e31733..09d1a1a 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -4054,3 +4054,16 @@
> >   ##
> >   { 'enum': 'ReplayMode',
> >     'data': [ 'none', 'record', 'play' ] }
> > +
> > +##
> > +# @savevm
> > +#
> > +# Save a VM snapshot. Old snapshot with the same name will be deleted if 
> > exists.
> > +#
> > +# @name: identifier of a snapshot to be created
> > +#
> > +# Returns: Nothing on success
> > +#
> > +# Since 2.6
> > +##
> > +{ 'command': 'savevm', 'data': {'name': 'str'} }
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index db072a6..b7851e1 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -4795,3 +4795,28 @@ Example:
> >                    {"type": 0, "out-pport": 0, "pport": 0, "vlan-id": 3840,
> >                     "pop-vlan": 1, "id": 251658240}
> >      ]}
> > +
> > +EQMP
> > +
> > +SQMP
> > +savevm
> > +------
> > +
> > +Save a VM snapshot. Old snapshot with the same name will be deleted if exists.
> > +
> > +Arguments:
> > +
> > +- "name": snapshot name
> > +
> > +Example:
> > +
> > +-> { "execute": "savevm", "arguments": { "name": "snapshot1" } }
> > +<- { "return": {} }
> > +
> > +EQMP
> > +
> > +    {
> > +        .name       = "savevm",
> > +        .args_type  = "name:s",
> > +        .mhandler.cmd_new = qmp_marshal_savevm,
> > +    },
> A snapshot has a tag (QEMUSnapshotInfo member name) and an ID
> (QEMUSnapshotInfo member id_str).
>
> HMP's name arguments are overloaded: they're matched both against tag
> and ID.  Unwisely chosen tags can create ambiguity.  Example:
>
>      (qemu) savevm 2
>      (qemu) savevm
>      (qemu) info snapshots
>      ID        TAG                 VM SIZE                DATE       VM CLOCK
>      1         2                      1.7M 2016-01-18 16:56:31   00:00:00.000
>      2         vm-20160118165641      1.7M 2016-01-18 16:56:41   00:00:00.000
>
> Care to guess which one we get when we ask for "2"?
>
> I think we want separate, unoverloaded arguments for QMP.
I think there is no need to. Name is now absolutely mandatory.
Thus for new snapshots we will have 'name' specified and we
will be bound to name only.

'id' will be used for old VMs and this is convenience
layer to make old 'id' only snaphosts accessible
through new interface in the same way as old.