Re: [Qemu-devel] [PATCH v2 04/12] qapi: Convert delvm

[Eric Blake <eblake@redhat.com>]

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

On 04/24/2013 09:32 AM, Pavel Hrdina wrote:
> QMP command vm-snapshot-delete takes two parameters: name and id. They are
> optional, but 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. The command returns
> SnapshotInfo only if the snapshot exists, otherwise it returns an error message.
> 
> HMP command delvm now uses the new vm-snapshot-delete, but behave slightly

s/but behave/and behaves/

> different. The delvm takes one optional flag -i and one parameter name. If you
> provide only the name parameter, it will match only snapshots with that name.
> If you also provide the flag, it will match only snapshots with name as id.
> Information about successfully deleted snapshot will be printed, otherwise
> an error message will be printed.
> 
> These improves behavior of the command to be more strict on selecting snapshots
> because actual behavior is wrong. Now if you want to delete snapshot with name '2'
> but there is no snapshot with that name it could delete snapshot with id '2' and
> that isn't what you want.
> 
> There is also small hack to ensure that in each block device with different
> driver type the correct snapshot is deleted. The 'qcow2' and 'sheepdog' internally
> search at first for id but 'rbd' has only name and therefore search only for name.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  hmp-commands.hx         | 14 +++++-----
>  hmp.c                   | 33 +++++++++++++++++++++++
>  hmp.h                   |  1 +
>  include/sysemu/sysemu.h |  1 -
>  qapi-schema.json        | 17 ++++++++++++
>  qmp-commands.hx         | 38 +++++++++++++++++++++++++++
>  savevm.c                | 69 +++++++++++++++++++++++++++++++++++++++----------
>  7 files changed, 153 insertions(+), 20 deletions(-)
> 

> +void hmp_vm_snapshot_delete(Monitor *mon, const QDict *qdict)
> +{
> +    const char *name = qdict_get_try_str(qdict, "name");
> +    const bool id = qdict_get_try_bool(qdict, "id", false);

Don't know that the 'const' is really needed here, but doesn't hurt.

> +    Error *local_err = NULL;
> +    SnapshotInfo *info;
> +
> +    if (id) {
> +        info = qmp_vm_snapshot_delete(false, NULL, true, name, &local_err);
> +    } else {
> +        info = qmp_vm_snapshot_delete(true, name, false, NULL, &local_err);
> +    }
> +
> +    if (info) {
> +        char buf[256];

I know this fixed-size buffer is just a copy-and-paste from other code
that displays snapshot information, but I really hate it. On the other
hand, I can tolerate if we have it as an intermediate step between two
series that both land in the same release.

If your series goes in first, Wenchao's series that cleans up the
fixed-size buffer will need to be rebased to tweak this additional spot.
 If Wenchao's patches go in first, then you will have a bit of rebase
work to do.  Since we are already deferring this series into 1.6, I
think it would be nice to post a unified series of the best of both
authors, rather than continuing to waffle on what should go in first.
[And if I keep saying that often enough, I may end up getting my hands
dirty and becoming the person that posts such a unified patch, although
generally I don't like forcefully taking over someone else's initial work.]

> +++ b/qapi-schema.json
> @@ -3505,3 +3505,20 @@
>      '*asl_compiler_rev':  'uint32',
>      '*file':              'str',
>      '*data':              'str' }}
> +
> +##
> +# @vm-snapshot-delete:
> +#
> +# Delete a snapshot identified by name or id or both. One of the name or id
> +# is required. It will returns SnapshotInfo of successfully deleted snapshot.

s/returns/return/

> +#
> +# @name: #optional tag of an existing snapshot
> +#
> +# @id: #optional id of an existing snapshot
> +#
> +# Returns: SnapshotInfo on success
> +#
> +# Since: 1.5

1.6, now that we are deferring.

> +##
> +{ 'command': 'vm-snapshot-delete', 'data': {'*name': 'str', '*id': 'str'},
> +  'returns': 'SnapshotInfo' }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 4d65422..9b15cb4 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1423,6 +1423,44 @@ Example:
>  
>  EQMP
>      {
> +        .name       = "vm-snapshot-delete",
> +        .args_type  = "name:s?,id:s?",
> +        .params     = "[tag] [id]",
> +        .help       = "delete a VM snapshot from its tag or id",

Sounds slightly better if you do s/from/based on/

> +        .mhandler.cmd_new = qmp_marshal_input_vm_snapshot_delete
> +    },
> +
> +SQMP
> +vm-snapshot-delete
> +------
> +
> +Delete a snapshot identified by name or id or both. One of the name or id
> +is required. It will returns SnapshotInfo of successfully deleted snapshot.

s/returns/return/

> @@ -2556,31 +2557,73 @@ int load_vmstate(const char *name)
>      return 0;
>  }
>  
> -void do_delvm(Monitor *mon, const QDict *qdict)
> +SnapshotInfo *qmp_vm_snapshot_delete(const bool has_name, const char *name,
> +                                     const bool has_id, const char *id,
> +                                     Error **errp)

Actual function looks right to me.

-- 
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: 621 bytes --]