Patchwork [04/11] qapi: Convert delvm

login
register
mail settings
Submitter Pavel Hrdina
Date April 16, 2013, 4:05 p.m.
Message ID <35a7188b135544116c0c8e954c8aa20bd88b54bb.1366127809.git.phrdina@redhat.com>
Download mbox | patch
Permalink /patch/237019/
State New
Headers show

Comments

Pavel Hrdina - April 16, 2013, 4:05 p.m.
QMP command vm-snapshot-delete no 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, if snapshot doesn't exist it returns
nothing. If something goes wrong, it returns an error message.

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.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 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(-)
Eric Blake - April 16, 2013, 7:39 p.m.
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 <phrdina@redhat.com>
> ---
>  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
> -@item delvm @var{tag}|@var{id}
> +@item delvm [-i] @var{tag}|[@var{id}]

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)
>
Kevin Wolf - April 18, 2013, 1:28 p.m.
Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben:
> QMP command vm-snapshot-delete no 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, if snapshot doesn't exist it returns
> nothing. If something goes wrong, it returns an error message.
> 
> 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.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  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]",

This is a weird syntax description. I think I'd write it like this:
"tag|-i id"

> +        .help       = "delete a VM snapshot from its tag or id if -i flag is provided",
> +        .mhandler.cmd = hmp_vm_snapshot_delete,
>      },
>  
>  STEXI
> -@item delvm @var{tag}|@var{id}
> +@item delvm [-i] @var{tag}|[@var{id}]
>  @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}.
> +
>  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));
> +    } else if (!error_is_set(&local_err)) {
> +        monitor_printf(mon, "Snapshot '%s' not found.\n", name);
> +    }

Why isn't an error set when trying to delete a snapshot that doesn't
exist?

> +
> +    qapi_free_SnapshotInfo(info);
> +    hmp_handle_error(mon, &local_err);
> +}
> diff --git a/hmp.h b/hmp.h
> index 95fe76e..b0667b3 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -85,5 +85,6 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict);
>  void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
>  void hmp_chardev_add(Monitor *mon, const QDict *qdict);
>  void hmp_chardev_remove(Monitor *mon, const QDict *qdict);
> +void hmp_vm_snapshot_delete(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 6578782..f46f9d2 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -67,7 +67,6 @@ void qemu_add_machine_init_done_notifier(Notifier *notify);
>  
>  void do_savevm(Monitor *mon, const QDict *qdict);
>  int load_vmstate(const char *name);
> -void do_delvm(Monitor *mon, const QDict *qdict);
>  void do_info_snapshots(Monitor *mon, const QDict *qdict);
>  
>  void qemu_announce_self(void);
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 751d3c2..641f3ac 100644
> --- a/qapi-schema.json
> +++ 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
> +#
> +# Returns: SnapshotInfo on success
> +#
> +# Since: 1.5
> +##
> +{ 'command': 'vm-snapshot-delete', 'data': {'*name': 'str', '*id': 'str'},
> +  'returns': 'SnapshotInfo' }
> 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
> +      }
> +   }
> +
> +EQMP
> +    {
> +        .name       = "vm-snapshot-delete",
> +        .args_type  = "name:s?,id:s?",
> +        .params     = "[tag] [id]",
> +        .help       = "delete a VM snapshot from its tag or id",
> +        .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.
> +
> +Arguments:
> +
> +- "name": tag of an existing snapshot (json-string, optional)
> +
> +- "id": id of an existing snapshot (json-string, optional)
> +
> +Example:
> +
> +-> { "execute": "vm-snapshot-delete", "arguments": { "name": "my_snapshot" } }
> +<- {
> +      "return": {
> +         "id": "1",
> +         "name": "my_snapshot",
> +         "date-sec": 1364480534,
> +         "date-nsec": 978215000,
> +         "vm-clock-sec": 5,
> +         "vm-clock-nsec": 153620449,
> +         "vm-state-size": 5709953
> +      }
> +   }
> +
>  
>  EQMP
>      {
> diff --git a/savevm.c b/savevm.c
> index 96a2340..4af7d2d 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2473,28 +2473,51 @@ 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)
>  {
> -    BlockDriverState *bs, *bs1;
> -    Error *local_err = NULL;
> -    const char *name = qdict_get_str(qdict, "name");
> +    BlockDriverState *bs;
> +    SnapshotInfo *info = NULL;
> +    QEMUSnapshotInfo sn;
> +
> +    if (!has_name && !has_id) {
> +        error_setg(errp, "name or id must be provided");
> +        return NULL;
> +    }
>  
>      bs = bdrv_snapshots();
>      if (!bs) {
> -        monitor_printf(mon, "No block device supports snapshots\n");
> -        return;
> +        error_setg(errp, "no block device supports snapshots");

Please leave a capital N at the start of the message.

> +        return NULL;
>      }
>  
> -    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;
> +    }

Is it even allowed in QMP that you return NULL without setting an error?
What would the JSON response look like?

> +
> +    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)) {
> +            bdrv_snapshot_delete(bs, sn.name, errp);
> +            if (error_is_set(errp)) {

You need to use a local_err, the caller could have passed a NULL errp,
and then you wouldn't detect the error here.

> +                return NULL;
>              }
>          }
>      }
> +
> +    return info;
>  }

Kevin

Patch

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]",
+        .help       = "delete a VM snapshot from its tag or id if -i flag is provided",
+        .mhandler.cmd = hmp_vm_snapshot_delete,
     },
 
 STEXI
-@item delvm @var{tag}|@var{id}
+@item delvm [-i] @var{tag}|[@var{id}]
 @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}.
+
 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));
+    } else if (!error_is_set(&local_err)) {
+        monitor_printf(mon, "Snapshot '%s' not found.\n", name);
+    }
+
+    qapi_free_SnapshotInfo(info);
+    hmp_handle_error(mon, &local_err);
+}
diff --git a/hmp.h b/hmp.h
index 95fe76e..b0667b3 100644
--- a/hmp.h
+++ b/hmp.h
@@ -85,5 +85,6 @@  void hmp_nbd_server_add(Monitor *mon, const QDict *qdict);
 void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
 void hmp_chardev_add(Monitor *mon, const QDict *qdict);
 void hmp_chardev_remove(Monitor *mon, const QDict *qdict);
+void hmp_vm_snapshot_delete(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 6578782..f46f9d2 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -67,7 +67,6 @@  void qemu_add_machine_init_done_notifier(Notifier *notify);
 
 void do_savevm(Monitor *mon, const QDict *qdict);
 int load_vmstate(const char *name);
-void do_delvm(Monitor *mon, const QDict *qdict);
 void do_info_snapshots(Monitor *mon, const QDict *qdict);
 
 void qemu_announce_self(void);
diff --git a/qapi-schema.json b/qapi-schema.json
index 751d3c2..641f3ac 100644
--- a/qapi-schema.json
+++ 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
+#
+# Returns: SnapshotInfo on success
+#
+# Since: 1.5
+##
+{ 'command': 'vm-snapshot-delete', 'data': {'*name': 'str', '*id': 'str'},
+  'returns': 'SnapshotInfo' }
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
+      }
+   }
+
+EQMP
+    {
+        .name       = "vm-snapshot-delete",
+        .args_type  = "name:s?,id:s?",
+        .params     = "[tag] [id]",
+        .help       = "delete a VM snapshot from its tag or id",
+        .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.
+
+Arguments:
+
+- "name": tag of an existing snapshot (json-string, optional)
+
+- "id": id of an existing snapshot (json-string, optional)
+
+Example:
+
+-> { "execute": "vm-snapshot-delete", "arguments": { "name": "my_snapshot" } }
+<- {
+      "return": {
+         "id": "1",
+         "name": "my_snapshot",
+         "date-sec": 1364480534,
+         "date-nsec": 978215000,
+         "vm-clock-sec": 5,
+         "vm-clock-nsec": 153620449,
+         "vm-state-size": 5709953
+      }
+   }
+
 
 EQMP
     {
diff --git a/savevm.c b/savevm.c
index 96a2340..4af7d2d 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2473,28 +2473,51 @@  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)
 {
-    BlockDriverState *bs, *bs1;
-    Error *local_err = NULL;
-    const char *name = qdict_get_str(qdict, "name");
+    BlockDriverState *bs;
+    SnapshotInfo *info = NULL;
+    QEMUSnapshotInfo sn;
+
+    if (!has_name && !has_id) {
+        error_setg(errp, "name or id must be provided");
+        return NULL;
+    }
 
     bs = bdrv_snapshots();
     if (!bs) {
-        monitor_printf(mon, "No block device supports snapshots\n");
-        return;
+        error_setg(errp, "no block device supports snapshots");
+        return NULL;
     }
 
-    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;
+    }
+
+    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)) {
+            bdrv_snapshot_delete(bs, sn.name, errp);
+            if (error_is_set(errp)) {
+                return NULL;
             }
         }
     }
+
+    return info;
 }
 
 void do_info_snapshots(Monitor *mon, const QDict *qdict)