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

[Markus Armbruster <armbru@redhat.com>]

"Denis V. Lunev" <den@openvz.org> 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 <den@openvz.org>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Amit Shah <amit.shah@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> ---
>  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.