[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 13/18] qapi: Convert savevm
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH 13/18] qapi: Convert savevm |
Date: |
Wed, 15 Aug 2012 13:49:22 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120717 Thunderbird/14.0 |
On 08/15/2012 01:41 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <address@hidden>
> ---
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 address@hidden +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH 03/18] block: add error parameter to bdrv_snapshot_goto() and related functions, (continued)
- [Qemu-devel] [PATCH 03/18] block: add error parameter to bdrv_snapshot_goto() and related functions, Pavel Hrdina, 2012/08/15
- [Qemu-devel] [PATCH 08/18] savevm: add error parameter to qemu_savevm_state_begin(), Pavel Hrdina, 2012/08/15
- [Qemu-devel] [PATCH 12/18] savevm: add error parameter to qemu_loadvm_state(), Pavel Hrdina, 2012/08/15
- [Qemu-devel] [PATCH 11/18] savevm: add error parameter to qemu_savevm_state(), Pavel Hrdina, 2012/08/15
- [Qemu-devel] [PATCH 09/18] savevm: add error parameter to qemu_savevm_state_iterate(), Pavel Hrdina, 2012/08/15
- [Qemu-devel] [PATCH 10/18] savevm: add error parameter to qemu_savevm_state_complete(), Pavel Hrdina, 2012/08/15
- [Qemu-devel] [PATCH 14/18] qapi: Convert loadvm, Pavel Hrdina, 2012/08/15
- [Qemu-devel] [PATCH 13/18] qapi: Convert savevm, Pavel Hrdina, 2012/08/15
- Re: [Qemu-devel] [PATCH 13/18] qapi: Convert savevm,
Eric Blake <=
- [Qemu-devel] [PATCH 16/18] qapi: Convert info snapshots, Pavel Hrdina, 2012/08/15
- [Qemu-devel] [PATCH 15/18] qapi: Convert delvm, Pavel Hrdina, 2012/08/15
- [Qemu-devel] [PATCH 17/18] hmp: allow "bool" parameter to be optional, Pavel Hrdina, 2012/08/15
- [Qemu-devel] [PATCH 18/18] vm-snapshot-save: add force parameter, Pavel Hrdina, 2012/08/15