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

[Eric Blake]

On 04/16/2013 10:05 AM, Pavel Hrdina wrote:
> QMP command vm-snapshot-delete no takes two parameters name and id. They are

s/no//
s/parameters/parameters:/

> 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, if snapshot doesn't exist it returns
> nothing. If something goes wrong, it returns an error message.

I would prefer returning an error message if the snapshot doesn't exist.
 The QMP code generator is not set up for a top-level optional return.
I guess I'll see what you implemented below...

> 
> HMP command delvm now uses the new vm-snapshot-delete, but behave slightly
> 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, if there is 
> no
> snapshot with that name or id, the appropriate message will be printed. If
> something goes wrong, 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.

If I understand correctly, your goal is that 'delvm 2' will never delete
id '2' after this patch (unless id '2' happened to also be tag '2'); to
get the old behavior of being able to delete the (possibly anonymous) id
2, you have to use 'delvm -i 2'.

Yes, I can live with this explicit change in semantics.

> 
> Signed-off-by: Pavel Hrdina <address@hidden>
> ---
>  hmp-commands.hx         | 14 ++++++++------
>  hmp.c                   | 35 ++++++++++++++++++++++++++++++++++
>  hmp.h                   |  1 +
>  include/sysemu/sysemu.h |  1 -
>  qapi-schema.json        | 17 +++++++++++++++++
>  qmp-commands.hx         | 50 
> ++++++++++++++++++++++++++++++++++++++++++++++++-
>  savevm.c                | 49 +++++++++++++++++++++++++++++++++++-------------
>  7 files changed, 146 insertions(+), 21 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index df44906..d1701ce 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -339,16 +339,18 @@ ETEXI
>  
>      {
>          .name       = "delvm",
> -        .args_type  = "name:s",
> -        .params     = "tag|id",
> -        .help       = "delete a VM snapshot from its tag or id",
> -        .mhandler.cmd = do_delvm,
> +        .args_type  = "id:-i,name:s",
> +        .params     = "[-i] tag|[id]",

The 'tag|[id]' looks odd.  Would it be any better written as merely:
 [-i] name

> +        .help       = "delete a VM snapshot from its tag or id if -i flag is 
> provided",

Maybe:
 delete a VM snapshot by tag name, or with -i by its id
but I'm not sure if my attempt was any better.

> +        .mhandler.cmd = hmp_vm_snapshot_delete,
>      },
>  
>  STEXI
> address@hidden delvm @var{tag}|@var{id}
> address@hidden delvm [-i] @var{tag}|address@hidden

This line should match whatever .params string we settle on above.

>  @findex delvm
> -Delete the snapshot identified by @var{tag} or @var{id}.
> +Delete a snapshot identified by @var{tag}. If flag -i is provided, delete
> +a snapshot indentified by @var{id}.

s/indentified/identified/

> +
>  ETEXI
>  
>      {
> diff --git a/hmp.c b/hmp.c
> index 4fb76ec..2c754b3 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1425,3 +1425,38 @@ void hmp_chardev_remove(Monitor *mon, const QDict 
> *qdict)
>      qmp_chardev_remove(qdict_get_str(qdict, "id"), &local_err);
>      hmp_handle_error(mon, &local_err);
>  }
> +
> +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);
> +    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];
> +        QEMUSnapshotInfo sn = {
> +            .vm_state_size = info->vm_state_size,
> +            .date_sec = info->date_sec,
> +            .date_nsec = info->date_nsec,
> +            .vm_clock_nsec = info->vm_clock_sec * 1000000000 +
> +                             info->vm_clock_nsec,
> +        };
> +        pstrcpy(sn.id_str, sizeof(sn.id_str), info->id);
> +        pstrcpy(sn.name, sizeof(sn.name), info->name);
> +        monitor_printf(mon, "Deleted snapshot info:\n");
> +        monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), 
> NULL));
> +        monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), 
> &sn));

I like the idea of printing details about what just got deleted.

> +    } else if (!error_is_set(&local_err)) {

If info is NULL, but error_is_set returns false, we have a problem.
This code should not be reachable, since QMP doesn't have a way to
return optional information.  That is, QMP should have already reported
a lookup error into local_err, and it should be sufficient to do:

if (info) {
...
} else {
    monitor_printf(mon, "%s\n", error_get_pretty(local_err));
    error_free(local_err);
}

> +        monitor_printf(mon, "Snapshot '%s' not found.\n", name);
> +    }

Hmm.  If I have:

id 1 name 2
id 2 name 3
id 4 no name

Existing behavior says 'delvm 1' deletes 'id 1 name 2'; 'delvm 2' is
ambiguous whether it deletes 'id 1 name 2' or 'id 2 name 3' (and if I
recall correctly, the behavior was different between qemu-img vs. HMP),
and 'delvm 4' is the only way to delete the no-name snapshot 'id 4'.

Your approach says 'delvm 1' is an error, 'delvm 2' deletes 'id 1 name
2', and 'delvm -i 2' deletes 'id 2 name 3'; and the only way to delete
the no-name snapshot is to use 'delvm -i 4'.

My idea of trying id as a fallback if name lookup fails would mean
'delvm 1' deletes 'id 1 name 2', 'delvm 2' deletes 'id 1 name 2', and
'delvm -i 2' deletes 'id 2 name 3'; and that deleting the no-name
snapshot can be done with the simpler 'delvm 4'.

I guess either approach works, as long as we document it.  My comments
on 0/11 about having a fallback to id when name lookup fails may not be
necessary after all, if we go with your approach.  My approach is more
like existing code, in that it is possible to delete no-name snapshots
without having to pass any extra options; but your way is a bit nicer in
that deleting by id is generally not what you want to do without being
explicit.

Anyone else have an opinion on which semantic we should use if name
lookup fails but -i was not specified?

> +++ 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.
> +#
> +# @name: tag of an existing snapshot
> +#
> +# @id: id of an existing snapshot

Mark these with #optional.

> +#
> +# Returns: SnapshotInfo on success

This implies that failure to find a matching consistent snapshot to
delete must be a failure (since there is no way to mark an optional return).

> +#
> +# Since: 1.5
> +##
> +{ 'command': 'vm-snapshot-delete', 'data': {'*name': 'str', '*id': 'str'},
> +  'returns': 'SnapshotInfo' }

Or, if you really want to make an optional return, the current
convention is that you have to wrap it in a type that has an optional
member.  Something like

{ 'type': 'SnapshotReturn', 'data': { '*snapshot' : 'SnapshotInfo' } }
{ 'command': 'vm-snapshot-delte',
  'data': { '*name': 'str', '*id': 'str' },
  'returns': 'SnapshotReturn' }

where the result of a successful delete would be:

{ 'snapshot' : { 'name':'...', ... } }

and the result of a failed lookup but non-error (that sounds weird, just
typing it) would be:

{ }

See ChardevReturn/chardev-add for an example of optional return value;
but for that function, an optional return really made sense.  But if you
agree that an optional return doesn't make sense here, and that we only
succeed if we return something, then you get the nicer:

{ 'name':'...', ... }

with no extra layer to unwrap as your return value.

Oh, that points out another thing - since the qcow2 file supports
snapshots with no tag, you probably need a (separate) patch to
qapi-schema.json to change type SnapshotInfo to have '*name', with a
note that name is absent on anonymous snapshots (but that it will be
present on any snapshot created via QMP).

> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 4d65422..86f399d 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1419,7 +1419,55 @@ Example:
>  
>  -> { "execute": "add_client", "arguments": { "protocol": "vnc",
>                                               "fdname": "myclient" } }
> -<- { "return": {} }
> +<- {
> +      "return": {
> +         "id": "1",
> +         "name": "my_snapshot",
> +         "date-sec": 1364480534,
> +         "date-nsec": 978215000,
> +         "vm-clock-sec": 5,
> +         "vm-clock-nsec": 153620449,
> +        "vm-state-size": 5709953
> +      }
> +   }

Huh?  Why are you munging the 'add_client' example?

> +++ b/savevm.c

>  
> -    bs1 = NULL;
> -    while ((bs1 = bdrv_next(bs1))) {
> -        if (bdrv_can_snapshot(bs1)) {
> -            bdrv_snapshot_delete(bs1, name, &local_err);
> -            if (error_is_set(&local_err)) {
> -                monitor_printf(mon, "%s\n", error_get_pretty(local_err));
> -                error_free(local_err);
> +    if (!bdrv_snapshot_find(bs, &sn, name, id, false)) {
> +        /* no need to set an error if snapshot doesn't exist */
> +        return NULL;

Won't work.  My understanding is that you cannot return NULL without
setting an error.  While it may do what you want for HMP, it doesn't
jive with the way that QMP works, which is that a NULL return means that
it will assume the error is set, and try to deference it to build up the
error return, since QMP must always return something.  If you are going
to succeed with no data, you still have to return {}.

Maybe I'm not familiar enough with QMP, and you can get away with this
after all.  On the other hand, I'd much rather have error reporting here
anyways, since libvirt will be using QMP, and hiding the error to only
occur in HMP means that libvirt, and any other QMP client, would have to
reinvent the error reporting themselves, if they get an empty return.

> +    }
> +
> +    info = g_malloc0(sizeof(SnapshotInfo));
> +    info->id = g_strdup(sn.id_str);
> +    info->name = g_strdup(sn.name);
> +    info->date_nsec = sn.date_nsec;
> +    info->date_sec = sn.date_sec;
> +    info->vm_state_size = sn.vm_state_size;
> +    info->vm_clock_nsec = sn.vm_clock_nsec % 1000000000;
> +    info->vm_clock_sec = sn.vm_clock_nsec / 1000000000;
> +
> +    bs = NULL;
> +    while ((bs = bdrv_next(bs))) {
> +        if (bdrv_can_snapshot(bs)
> +                && bdrv_snapshot_find(bs, &sn, name, id, false)) {

Style - most of qemu puts && at the end of one line break, and starts
the next line flush with the first character after ( of the line above,
as in:

    if (bdrv_can_snapshot(bs) &&
        bdrv_snapshot_find(bs...)) {

> +            bdrv_snapshot_delete(bs, sn.name, errp);
> +            if (error_is_set(errp)) {
> +                return NULL;
>              }

Yuck - this means that a failure to delete partway through leaks info,
as well as leaving a partial snapshot in the remaining files not
visited.  Fix the memory leak no matter what.  And maybe we should
reconsider what happens if we have a partial failure during delete - is
it best to plow on as far as possible?  How should we indicate it back
to the caller that we found the snapshot they requested, but didn't
finish deleting it?

>          }
>      }
> +
> +    return info;
>  }
>  
>  void do_info_snapshots(Monitor *mon, const QDict *qdict)
> 

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

signature.asc
Description: OpenPGP digital signature