Patchwork [RFC,3/4] qapi: Convert delvm

login
register
mail settings
Submitter Pavel Hrdina
Date July 12, 2012, 4:55 p.m.
Message ID <049abd55f3f85bac5d299ca34f17b9abd39fedc4.1342092497.git.phrdina@redhat.com>
Download mbox | patch
Permalink /patch/170733/
State New
Headers show

Comments

Pavel Hrdina - July 12, 2012, 4:55 p.m.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 hmp-commands.hx  |    2 +-
 hmp.c            |   10 ++++++++++
 hmp.h            |    1 +
 qapi-schema.json |   17 +++++++++++++++++
 qmp-commands.hx  |   24 ++++++++++++++++++++++++
 savevm.c         |   21 +++++++++++----------
 sysemu.h         |    1 -
 7 files changed, 64 insertions(+), 12 deletions(-)
Eric Blake - July 12, 2012, 5:59 p.m.
On 07/12/2012 10:55 AM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  hmp-commands.hx  |    2 +-
>  hmp.c            |   10 ++++++++++
>  hmp.h            |    1 +
>  qapi-schema.json |   17 +++++++++++++++++
>  qmp-commands.hx  |   24 ++++++++++++++++++++++++
>  savevm.c         |   21 +++++++++++----------
>  sysemu.h         |    1 -
>  7 files changed, 64 insertions(+), 12 deletions(-)

> +
> +##
> +# @delvm:

This name is worse than 'loadvm' or 'savevm', in that we are NOT
deleting the entire vm, but a named snapshot stored within the vm.
Would naming it 'delete-vm-snapshot' make more sense (in which case the
others might make more sense as 'save-vm-snapshot' and 'load-vm-snapshot')?
Pavel Hrdina - July 16, 2012, 8:12 a.m.
On 07/12/2012 07:59 PM, Eric Blake wrote:
> On 07/12/2012 10:55 AM, Pavel Hrdina wrote:
>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>
>> ---
>>   hmp-commands.hx  |    2 +-
>>   hmp.c            |   10 ++++++++++
>>   hmp.h            |    1 +
>>   qapi-schema.json |   17 +++++++++++++++++
>>   qmp-commands.hx  |   24 ++++++++++++++++++++++++
>>   savevm.c         |   21 +++++++++++----------
>>   sysemu.h         |    1 -
>>   7 files changed, 64 insertions(+), 12 deletions(-)
>> +
>> +##
>> +# @delvm:
> This name is worse than 'loadvm' or 'savevm', in that we are NOT
> deleting the entire vm, but a named snapshot stored within the vm.
> Would naming it 'delete-vm-snapshot' make more sense (in which case the
> others might make more sense as 'save-vm-snapshot' and 'load-vm-snapshot')?
>
This naming looks nice. I definitely agree that it could be 
save-vm-snapshot, load-vm-snapshot, delete-vm-snapshot and 
query-vm-snapshots.

Pavel
Eric Blake - July 16, 2012, 4:37 p.m.
On 07/16/2012 02:12 AM, Pavel Hrdina wrote:
> On 07/12/2012 07:59 PM, Eric Blake wrote:
>> On 07/12/2012 10:55 AM, Pavel Hrdina wrote:
>>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>
>>> ---
>>>   hmp-commands.hx  |    2 +-
>>>   hmp.c            |   10 ++++++++++
>>>   hmp.h            |    1 +
>>>   qapi-schema.json |   17 +++++++++++++++++
>>>   qmp-commands.hx  |   24 ++++++++++++++++++++++++
>>>   savevm.c         |   21 +++++++++++----------
>>>   sysemu.h         |    1 -
>>>   7 files changed, 64 insertions(+), 12 deletions(-)
>>> +
>>> +##
>>> +# @delvm:
>> This name is worse than 'loadvm' or 'savevm', in that we are NOT
>> deleting the entire vm, but a named snapshot stored within the vm.
>> Would naming it 'delete-vm-snapshot' make more sense (in which case the
>> others might make more sense as 'save-vm-snapshot' and
>> 'load-vm-snapshot')?
>>
> This naming looks nice. I definitely agree that it could be
> save-vm-snapshot, load-vm-snapshot, delete-vm-snapshot and
> query-vm-snapshots.

On seeing that spelled out, I wonder if the '-vm' is just noise; where
we could use 'query-snapshots' instead of 'query-vm-snapshots'.  Then
again, we already have 'blockdev-snapshot-sync', which is an entirely
different snapshot (just a block device, rather than the entire VM), so
dropping -vm is probably a bad idea.  Anyone else want to chime in on
the bikeshed painting discussion of the best QMP name?
Kevin Wolf - July 17, 2012, 10:55 a.m.
Am 16.07.2012 18:37, schrieb Eric Blake:
> On 07/16/2012 02:12 AM, Pavel Hrdina wrote:
>> On 07/12/2012 07:59 PM, Eric Blake wrote:
>>> On 07/12/2012 10:55 AM, Pavel Hrdina wrote:
>>>> Signed-off-by: Pavel Hrdina<phrdina@redhat.com>
>>>> ---
>>>>   hmp-commands.hx  |    2 +-
>>>>   hmp.c            |   10 ++++++++++
>>>>   hmp.h            |    1 +
>>>>   qapi-schema.json |   17 +++++++++++++++++
>>>>   qmp-commands.hx  |   24 ++++++++++++++++++++++++
>>>>   savevm.c         |   21 +++++++++++----------
>>>>   sysemu.h         |    1 -
>>>>   7 files changed, 64 insertions(+), 12 deletions(-)
>>>> +
>>>> +##
>>>> +# @delvm:
>>> This name is worse than 'loadvm' or 'savevm', in that we are NOT
>>> deleting the entire vm, but a named snapshot stored within the vm.
>>> Would naming it 'delete-vm-snapshot' make more sense (in which case the
>>> others might make more sense as 'save-vm-snapshot' and
>>> 'load-vm-snapshot')?
>>>
>> This naming looks nice. I definitely agree that it could be
>> save-vm-snapshot, load-vm-snapshot, delete-vm-snapshot and
>> query-vm-snapshots.
> 
> On seeing that spelled out, I wonder if the '-vm' is just noise; where
> we could use 'query-snapshots' instead of 'query-vm-snapshots'.  Then
> again, we already have 'blockdev-snapshot-sync', which is an entirely
> different snapshot (just a block device, rather than the entire VM), so
> dropping -vm is probably a bad idea.  Anyone else want to chime in on
> the bikeshed painting discussion of the best QMP name?

Sure. ;-)

If there is a blockdev-snapshot-sync, then the counterparts for the
whole VM shouldn't be called load/save/del-vm-snapshot, but
vm-snapshot-load/save/del. And possibly add a -sync as well.

Kevin

Patch

diff --git a/hmp-commands.hx b/hmp-commands.hx
index b145612..3be9ad4 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -299,7 +299,7 @@  ETEXI
         .args_type  = "name:s",
         .params     = "tag|id",
         .help       = "delete a VM snapshot from its tag or id",
-        .mhandler.cmd = do_delvm,
+        .mhandler.cmd = hmp_delvm,
     },
 
 STEXI
diff --git a/hmp.c b/hmp.c
index 6ed4c3f..a4668ca 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1022,3 +1022,13 @@  void hmp_loadvm(Monitor *mon, const QDict *qdict)
 
     hmp_handle_error(mon, &err);
 }
+
+void hmp_delvm(Monitor *mon, const QDict *qdict)
+{
+    const char *name = qdict_get_str(qdict, "name");
+    Error *err = NULL;
+
+    qmp_delvm(name, &err);
+
+    hmp_handle_error(mon, &err);
+}
diff --git a/hmp.h b/hmp.h
index 5623ae7..78710e2 100644
--- a/hmp.h
+++ b/hmp.h
@@ -66,5 +66,6 @@  void hmp_netdev_add(Monitor *mon, const QDict *qdict);
 void hmp_netdev_del(Monitor *mon, const QDict *qdict);
 void hmp_savevm(Monitor *mon, const QDict *qdict);
 void hmp_loadvm(Monitor *mon, const QDict *qdict);
+void hmp_delvm(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 7df6a90..a22d26a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1912,3 +1912,20 @@ 
 # Since: 1.2
 ##
 { 'command': 'loadvm', 'data': {'name': 'str'} }
+
+##
+# @delvm:
+#
+# Delete the snapshot identified by 'tag' or 'id'.
+#
+# @name: tag or id of existing snapshot
+#
+# Returns: Nothing on success
+#          If no block device can accept snapshots, SnapshotNotAccepted
+#          If there is a writable device not supporting snapshots,
+#            SnapshotNotSupported
+#          If delete snapshot with same 'name' fail, SnapshotDeleteFailed
+#
+# Since: 1.2
+##
+{ 'command': 'delvm', 'data': {'name': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 02550f2..1071bb9 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1112,6 +1112,30 @@  Example:
 
 EQMP
     {
+        .name       = "delvm",
+        .args_type  = "name:s",
+        .params     = "tag|id",
+        .help       = "delete a VM snapshot from its tag or id",
+        .mhandler.cmd_new = qmp_marshal_input_delvm
+    },
+
+SQMP
+delvm
+------
+
+Delete the snapshot identified by 'tag' or 'id'.
+
+Arguments:
+
+- "name": tag or id of existing snapshot
+
+Example:
+
+-> { "execute": "delvm", "arguments": { "name": "my_snapshot" } }
+<- { "return": {} }
+
+EQMP
+    {
         .name       = "qmp_capabilities",
         .args_type  = "",
         .params     = "",
diff --git a/savevm.c b/savevm.c
index c113dd4..d3e8b07 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2286,15 +2286,14 @@  void qmp_loadvm(const char *name, Error **errp)
     }
 }
 
-void do_delvm(Monitor *mon, const QDict *qdict)
+void qmp_delvm(const char *name, Error **errp)
 {
     BlockDriverState *bs, *bs1;
     int ret;
-    const char *name = qdict_get_str(qdict, "name");
 
     bs = bdrv_snapshots();
     if (!bs) {
-        monitor_printf(mon, "No block device supports snapshots\n");
+        error_set(errp, QERR_SNAPSHOT_NOT_ACCEPTED);
         return;
     }
 
@@ -2303,13 +2302,15 @@  void do_delvm(Monitor *mon, const QDict *qdict)
         if (bdrv_can_snapshot(bs1)) {
             ret = bdrv_snapshot_delete(bs1, name);
             if (ret < 0) {
-                if (ret == -ENOTSUP)
-                    monitor_printf(mon,
-                                   "Snapshots not supported on device '%s'\n",
-                                   bdrv_get_device_name(bs1));
-                else
-                    monitor_printf(mon, "Error %d while deleting snapshot on "
-                                   "'%s'\n", ret, bdrv_get_device_name(bs1));
+                if (ret == -ENOTSUP) {
+                    error_set(errp, QERR_SNAPSHOT_NOT_SUPPORTED,
+                              bdrv_get_device_name(bs1));
+                    return;
+                } else {
+                    error_set(errp, QERR_SNAPSHOT_DELETE_FAILED,
+                              bdrv_get_device_name(bs1), ret);
+                    return;
+                }
             }
         }
     }
diff --git a/sysemu.h b/sysemu.h
index 8e65651..45e071c 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -69,7 +69,6 @@  void qemu_remove_exit_notifier(Notifier *notify);
 
 void qemu_add_machine_init_done_notifier(Notifier *notify);
 
-void do_delvm(Monitor *mon, const QDict *qdict);
 void do_info_snapshots(Monitor *mon);
 
 void qemu_announce_self(void);