diff mbox

[v2,04/12] qapi: Convert delvm

Message ID 2d4c5e75a5fa710d73fa4ffae03f4c143ba66519.1366817130.git.phrdina@redhat.com
State New
Headers show

Commit Message

Pavel Hrdina April 24, 2013, 3:32 p.m. UTC
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
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(-)

Comments

Eric Blake April 24, 2013, 10:54 p.m. UTC | #1
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.
Wayne Xia April 25, 2013, 6:58 a.m. UTC | #2
>> +        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.
   That would be a very long serial, taking time to rebase for any code
change in it, that is why I haven't consider it before.

> [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
   Pls don't, I guess it would not be a good experience working in a
long serial which may need modification later.

> generally I don't like forcefully taking over someone else's initial work.]
>
   My serial serves mainly for block image's info querying, different
with Pavel, one serial fixing all is not easy to make.
   Instead, I'll send out small serial change the common part:
1 better bdrv_snapshot_find().
2 hmp/qemu-img dumping info code().
   Then we rebase on it, as two serial, do you think it is OK?

>> +++ b/qapi-schema.json

>
Eric Blake April 25, 2013, 12:21 p.m. UTC | #3
On 04/25/2013 12:58 AM, Wenchao Xia wrote:
> 
>>> +        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.
>   That would be a very long serial, taking time to rebase for any code
> change in it, that is why I haven't consider it before.

s/serial/series/

But it would at least have the end goal in mind, instead of trying to
debate what the end goal is between two different series.

> 
>> [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
>   Pls don't, I guess it would not be a good experience working in a
> long serial which may need modification later.

Sometimes, letting an additional author join in on attempting to post
patches can be productive.  But I certainly don't want to make it feel
like a hostile takeover - we've got plenty of time before 1.6, so I
don't mind letting you work through a few more revisions of the series.

>   My serial serves mainly for block image's info querying, different
> with Pavel, one serial fixing all is not easy to make.
>   Instead, I'll send out small serial change the common part:
> 1 better bdrv_snapshot_find().
> 2 hmp/qemu-img dumping info code().
>   Then we rebase on it, as two serial, do you think it is OK?

Splitting into pieces is also okay, as long as the pieces make sense.  I
see several piecemeal changes being attempted between the two series
with several conflicts if we don't factor out common parts, such as
moving snapshot-related code into a new file, making snapshot lookup
cleaner, removing hard-coded length limits on HMP snapshot display.
Yes, getting the common parts clean as one series, then doing two more
relatively-independent series of 1. better query output, 2. QMP
counterpart to snapshot manipulations, is probably workable.
Wayne Xia April 26, 2013, 2:39 a.m. UTC | #4
于 2013-4-25 20:21, Eric Blake 写道:
> On 04/25/2013 12:58 AM, Wenchao Xia wrote:
>>
>>>> +        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.
>>    That would be a very long serial, taking time to rebase for any code
>> change in it, that is why I haven't consider it before.
>
> s/serial/series/
>
> But it would at least have the end goal in mind, instead of trying to
> debate what the end goal is between two different series.
>
>>
>>> [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
>>    Pls don't, I guess it would not be a good experience working in a
>> long serial which may need modification later.
>
> Sometimes, letting an additional author join in on attempting to post
> patches can be productive.  But I certainly don't want to make it feel
> like a hostile takeover - we've got plenty of time before 1.6, so I
> don't mind letting you work through a few more revisions of the series.
>
>>    My serial serves mainly for block image's info querying, different
>> with Pavel, one serial fixing all is not easy to make.
>>    Instead, I'll send out small serial change the common part:
>> 1 better bdrv_snapshot_find().
>> 2 hmp/qemu-img dumping info code().
>>    Then we rebase on it, as two serial, do you think it is OK?
>
> Splitting into pieces is also okay, as long as the pieces make sense.  I
> see several piecemeal changes being attempted between the two series
> with several conflicts if we don't factor out common parts, such as
> moving snapshot-related code into a new file, making snapshot lookup
> cleaner, removing hard-coded length limits on HMP snapshot display.
> Yes, getting the common parts clean as one series, then doing two more
> relatively-independent series of 1. better query output, 2. QMP
> counterpart to snapshot manipulations, is probably workable.
>
   OK, I'll send out 2 serial clean the common part. Pavel, please
wait for mine serial before v3.
Kevin Wolf May 3, 2013, 10:50 a.m. UTC | #5
Am 24.04.2013 um 17:32 hat Pavel Hrdina geschrieben:
> 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
> 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>

One general comment: I'm not sure how much sense it really makes to
delete snapshots based on ID on multiple images. The user doesn't have
any influence on the ID, I think, so a VM-wide snapshot can only be
identified by name across multiple images.

> --- a/savevm.c
> +++ b/savevm.c
> @@ -40,6 +40,7 @@
>  #include "trace.h"
>  #include "qemu/bitops.h"
>  #include "qemu/iov.h"
> +#include "block/block_int.h"
>  
>  #define SELF_ANNOUNCE_ROUNDS 5
>  
> @@ -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)
>  {
> -    BlockDriverState *bs, *bs1;
> +    BlockDriverState *bs;
> +    SnapshotInfo *info = NULL;
> +    QEMUSnapshotInfo sn;
>      Error *local_err = NULL;
> -    const char *name = qdict_get_str(qdict, "name");
> +
> +    if (!has_name && !has_id) {
> +        error_setg(errp, "Name or id must be provided");
> +        return NULL;
> +    }
> +
> +    if (!has_name) {
> +        name = NULL;
> +    }
> +    if (!has_id) {
> +        id = 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 (!bdrv_snapshot_find(bs, &sn, name, id, &local_err, false)) {
> +        error_propagate(errp, local_err);
> +        return NULL;
> +    }

Why is this necessary? The check didn't exist before.

> +
> +    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, NULL, false)) {

Aha, this is the reason: The command is underspecified and you change
some behaviour that isn't mentioned in the documentation. Before, the
command would return an error if a device that supports snapshots
doesn't have the specific snapshot. After the patch, it would silently
ignore the snapshot - except in bdrv_snapshots(), which is more or less
randomly selected.

Why is this an improvement?

Independent of what we decide here, the result should be added to the
QMP documentation.

> +            /* Small hack to ensure that proper snapshot is deleted for every
> +             * block driver. This will be fixed soon. */
> +            if (!strcmp(bs->drv->format_name, "rbd")) {
> +                bdrv_snapshot_delete(bs, sn.name, &local_err);
> +            } else {
> +                bdrv_snapshot_delete(bs, sn.id_str, &local_err);
> +            }
>              if (error_is_set(&local_err)) {
> -                qerror_report(ERROR_CLASS_GENERIC_ERROR, "Failed to delete "
> -                              "old snapshot on device '%s': %s",
> -                              bdrv_get_device_name(bs),
> -                              error_get_pretty(local_err));
> +                error_setg(errp, "Failed to delete  old snapshot on "
> +                           "device '%s': %s", bdrv_get_device_name(bs),
> +                           error_get_pretty(local_err));
>                  error_free(local_err);
>              }

You can't use error_setg() multiple times on the same errp, the second
call would trigger an assertion failure. Should we immediately break
after an error?

>          }
>      }
> +
> +    if (error_is_set(errp)) {
> +        g_free(info);
> +        return NULL;
> +    }
> +
> +    return info;
>  }

Kevin
diff mbox

Patch

diff --git a/hmp-commands.hx b/hmp-commands.hx
index df44906..32f79d9 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] name",
+        .help       = "delete a VM snapshot by name as tag or with -i flag by name as id",
+        .mhandler.cmd = hmp_vm_snapshot_delete,
     },
 
 STEXI
-@item delvm @var{tag}|@var{id}
+@item delvm [-i] @var{name}
 @findex delvm
-Delete the snapshot identified by @var{tag} or @var{id}.
+Delete a snapshot identified by @var{name} as tag. If flag -i is provided,
+delete a snapshot identified by @var{name} as id.
+
 ETEXI
 
     {
diff --git a/hmp.c b/hmp.c
index 4fb76ec..fea1cee 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1425,3 +1425,36 @@  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));
+    }
+
+    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..0bd61b8 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: #optional tag of an existing snapshot
+#
+# @id: #optional 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..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",
+        .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
+    {
         .name       = "qmp_capabilities",
         .args_type  = "",
         .params     = "",
diff --git a/savevm.c b/savevm.c
index 1622c55..1b4aea8 100644
--- a/savevm.c
+++ b/savevm.c
@@ -40,6 +40,7 @@ 
 #include "trace.h"
 #include "qemu/bitops.h"
 #include "qemu/iov.h"
+#include "block/block_int.h"
 
 #define SELF_ANNOUNCE_ROUNDS 5
 
@@ -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)
 {
-    BlockDriverState *bs, *bs1;
+    BlockDriverState *bs;
+    SnapshotInfo *info = NULL;
+    QEMUSnapshotInfo sn;
     Error *local_err = NULL;
-    const char *name = qdict_get_str(qdict, "name");
+
+    if (!has_name && !has_id) {
+        error_setg(errp, "Name or id must be provided");
+        return NULL;
+    }
+
+    if (!has_name) {
+        name = NULL;
+    }
+    if (!has_id) {
+        id = 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 (!bdrv_snapshot_find(bs, &sn, name, id, &local_err, false)) {
+        error_propagate(errp, local_err);
+        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, NULL, false)) {
+            /* Small hack to ensure that proper snapshot is deleted for every
+             * block driver. This will be fixed soon. */
+            if (!strcmp(bs->drv->format_name, "rbd")) {
+                bdrv_snapshot_delete(bs, sn.name, &local_err);
+            } else {
+                bdrv_snapshot_delete(bs, sn.id_str, &local_err);
+            }
             if (error_is_set(&local_err)) {
-                qerror_report(ERROR_CLASS_GENERIC_ERROR, "Failed to delete "
-                              "old snapshot on device '%s': %s",
-                              bdrv_get_device_name(bs),
-                              error_get_pretty(local_err));
+                error_setg(errp, "Failed to delete  old snapshot on "
+                           "device '%s': %s", bdrv_get_device_name(bs),
+                           error_get_pretty(local_err));
                 error_free(local_err);
             }
         }
     }
+
+    if (error_is_set(errp)) {
+        g_free(info);
+        return NULL;
+    }
+
+    return info;
 }
 
 void do_info_snapshots(Monitor *mon, const QDict *qdict)