Re: [Qemu-devel] [PATCH 00/11] covert savevm, loadvm and delvm into qapi

[Eric Blake]

On 04/16/2013 10:05 AM, Pavel Hrdina wrote:

s/covert/convert/ in the subject, if you send a v2 (but doesn't affect
git, so depending on how review goes, you may get lucky :)

> I'm sending patches for all commands in one patch series because the
> savevm command depends on delvm command.
> 
> This patch series introduces new design of these commands:
> 
> * QMP vm-snapshot-save:
>     - { 'command': 'vm-snapshot-save',
>         'data': { 'name': 'str' },
>         'returns': 'SnapshotInfo' }
>     - vm-snapshot-save returns an error if there is an existing snapshot with
>       the same name
>     - you cannot provide an id for a new snapshot
>     - all information about created snapshot will be returned

Looks good.

> 
> * QMP vm-snapshot-load
>     - { 'command': 'vm-snapshot-load',
>         'data': { '*name': 'str', '*id': 'str' },
>         'returns': 'SnapshotInfo' }
>     - one of the name or id must be provided
>     - if both are provided they will match only the snapshot with the same 
> name
>       and id
>     - returns SnapshotInfo only if the snapshot exists.

A return value is not strictly necessary, if we guarantee an error on
all cases where nothing was loaded.  On the other hand, by returning a
value, the caller can learn which 'name' or 'id' was populated if the
user omitted an input parameter, in case the caller plans to do some
sanity checking on what actually got loaded.  I don't care either way
(libvirt will just be looking for non-error return, rather than parsing
the return value).

> 
> * QMP vm-snapshot-delete:
>     - { 'command': 'vm-snapshot-delete',
>         'data': { '*name': 'str', '*id': 'str' },
>         'returns': 'SnapshotInfo' }
>     - same rules as vm-snapshot-load

Same comment about 'returns':{} being sufficient (libvirt will just be
looking for non-error return).

> 
> * HMP savevm:
>     - args_type = "force:-f,name:s?",
>     - if the name is not provided the HMP command will generates new one for 
> QMP
>       command
>     - if there is already a snapshot with provided or generated name it will
>       fails
>     - there will be an optional -f parameter to force saving requested 
> snapshot
>       and it will internally use vm-snapshot-delete and then vm-snapshot-save
>     - all information about created snapshot will be printed

Looks good.

> 
> * HMP loadvm:
>     - args_type = "id:-i,name:s",
>     - follow the same behavior as the QMP command

Not quite.  You are only providing one name, so what you are really
doing is: if -i is present, supply that name as the QMP id argument.  If
-i is not present, first try the name as the QMP name argument, and on
failure try again with the same name as the QMP id argument.

>     - it load snapshot that match the provided name
>     - if an id flag is provided, it load snapshot that match the name 
> parameter
>       as an id of snapshot

The args_type looks reasonable.

> 
> * HMP delvm:
>     - args_type = "id:-i,name:s"
>     - same rules as loadvm

Again, looks reasonable.

> 
> Pavel Hrdina (11):
>   qemu-img: introduce qemu_img_handle_error()
>   block: update error reporting for bdrv_snapshot_delete() and related
>     functions
>   savevm: update bdrv_snapshot_find() to find snapshot by id or name
>   qapi: Convert delvm
>   block: update error reporting for bdrv_snapshot_goto() and related
>     functions
>   savevm: update error reporting for qemu_loadvm_state()
>   qapi: Convert loadvm
>   block: update error reporting for bdrv_snapshot_create() and related
>     functions
>   savevm: update error reporting off qemu_savevm_state() and related
>     functions
>   qapi: Convert savevm
>   savevm: remove backward compatibility from bdrv_snapshot_find()
> 
>  block.c                   | 100 +++++++------
>  block/qcow2-snapshot.c    |  71 ++++++---
>  block/qcow2.h             |  16 +-
>  block/rbd.c               |  50 ++++---
>  block/sheepdog.c          |  61 ++++----
>  hmp-commands.hx           |  48 +++---
>  hmp.c                     | 119 +++++++++++++++
>  hmp.h                     |   3 +
>  include/block/block.h     |  17 ++-
>  include/block/block_int.h |  17 ++-
>  include/sysemu/sysemu.h   |  12 +-
>  migration.c               |  15 +-
>  monitor.c                 |  12 --
>  qapi-schema.json          |  54 +++++++
>  qemu-img.c                |  54 ++++---
>  qmp-commands.hx           | 127 +++++++++++++++-
>  savevm.c                  | 363 
> +++++++++++++++++++++++++---------------------
>  vl.c                      |   7 +-
>  18 files changed, 782 insertions(+), 364 deletions(-)
> 

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

signature.asc
Description: OpenPGP digital signature