Re: [Qemu-devel] [PATCH 13/18] qapi: Convert savevm

[Eric Blake <eblake@redhat.com>]

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

On 08/15/2012 01:41 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---

I'm focusing my review more on the public interface (since that's what
affects libvirt), and therefore glanced through 1 through 12 but did not
pay close attention to them.

>  hmp-commands.hx  |  2 +-
>  hmp.c            | 10 ++++++++++
>  hmp.h            |  1 +
>  qapi-schema.json | 19 +++++++++++++++++++
>  qmp-commands.hx  | 29 +++++++++++++++++++++++++++++
>  savevm.c         | 25 +++++++++----------------
>  sysemu.h         |  1 -
>  7 files changed, 69 insertions(+), 18 deletions(-)
> 
> +
> +void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)
> +{
> +    const char *name = qdict_get_try_str(qdict, "name");

In the cover letter, you said "and for QMP you have to always provide
name parameter" - but this says 'name' is optional and can still be
empty (id only).

> +++ b/qapi-schema.json
> @@ -2356,3 +2356,22 @@
>  # Since: 1.2.0
>  ##
>  { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'] }
> +
> +##
> +# @vm-snapshot-save:
> +#
> +# Create a snapshot of the whole virtual machine. If 'tag' is provided,

'tag' is a misnomer, since you list @name as the parameter name.

> +# it is used as human readable identifier. If there is already a snapshot
> +# with the same tag or ID, it is replaced.
> +#
> +# The VM is automatically stopped and resumed and saving a snapshot can take
> +# a long time.
> +#
> +# @name: tag or id of new or existing snapshot

Here, you document @name as required,

> +#
> +# Returns: Nothing on success
> +#          If an error occurs, GenericError with error message
> +#
> +# Since: 1.2

We missed 1.2 hard freeze.  This better be 1.3.

> +##
> +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }

but here, you listed it as '*name' meaning optional.  I'm okay with
leaving name optional, provided that it means that you end up allocating
the next unique id.  But if that's the case, then returning nothing is
wrong; this command needs to return { 'id':'int', '*name':'str' }, so
that the user knows what snapshot just got allocated.

> +SQMP
> +vm-snapshot-save
> +------
> +
> +Create a snapshot of the whole virtual machine. If 'tag' is provided,
> +it is used as human readable identifier. If there is already a snapshot
> +with the same tag or ID, it is replaced.
> +
> +The VM is automatically stopped and resumed and saving a snapshot can take
> +a long time.

I don't like that this command can take a long time.  Just today on
#virt IRC, someone was complaining that 'savevm' HMP cannot be canceled.
 If we're going to create a new interface, it would be nicer to create a
command that starts the save process and returns immediately, as well as
commands to track progress, allow an early abort, and send an event on
completion.  The HMP 'savevm' interface can issue multiple QMP commands
under the hood to continue with it's blocking behavior, but as long as
we are fixing things, I think the QMP interface should be more powerful.

> +
> +Arguments:
> +
> +- "name": tag or id of new or existing snapshot
> +
> +Example:
> +
> +-> { "execute": "vm-snapshot-save", "arguments": { "name": "my_snapshot" } }
> +<- { "return": {} }

Again, you need to return the id of the just-created snapshot.

> @@ -2176,21 +2174,20 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>      }
>  
>      /* Delete old snapshots of the same name */
> -    if (name && del_existing_snapshots(name, NULL) < 0) {
> +    if (has_name && del_existing_snapshots(name, errp) < 0) {

Here's hoping later in the series updates this to make saner decisions.
 (Maybe I should peruse the entire series before commenting on
individual patches?)

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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