Patchwork [18/18] vm-snapshot-save: add force parameter

login
register
mail settings
Submitter Pavel Hrdina
Date Aug. 15, 2012, 7:41 a.m.
Message ID <582c2debbce809a86fe62d6a998d42bee8ef274c.1345016001.git.phrdina@redhat.com>
Download mbox | patch
Permalink /patch/177574/
State New
Headers show

Comments

Pavel Hrdina - Aug. 15, 2012, 7:41 a.m.
HMP command "savevm" now takes extra optional force parameter to specifi whether
replace existing snapshot or not.

QMP command "vm-snapshot-save" has also extra optional force parameter and
name parameter isn't optional anymore.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 hmp-commands.hx  | 14 +++++++-------
 hmp.c            | 25 ++++++++++++++++++++++++-
 qapi-schema.json | 12 +++++++-----
 qmp-commands.hx  | 16 +++++++++-------
 savevm.c         | 35 ++++++++++++++---------------------
 5 files changed, 61 insertions(+), 41 deletions(-)
Eric Blake - Aug. 16, 2012, 5 a.m.
On 08/15/2012 01:41 AM, Pavel Hrdina wrote:
> HMP command "savevm" now takes extra optional force parameter to specifi whether

s/specifi/specify/

> replace existing snapshot or not.
> 
> QMP command "vm-snapshot-save" has also extra optional force parameter and
> name parameter isn't optional anymore.

When HMP omits the name, what name are you using in it's place?  Either
a nameless snapshot is okay (and QMP must expose it), or it is an error
(and HMP must now be passing a reasonable name).

/me reads patch

Oh, there was always a name; the default name was a timestamp, and you
moved the timestamp creation into HMP.


> +++ b/hmp-commands.hx
> @@ -264,19 +264,19 @@ ETEXI
>  
>      {
>          .name       = "savevm",
> -        .args_type  = "name:s?",
> -        .params     = "[tag|id]",
> -        .help       = "save a VM snapshot. If no tag or id are provided, a new snapshot is created",
> +        .args_type  = "name:s?,force:b?",
> +        .params     = "[tag|id] [on|off]",

Rather than sticking 'force' as an optional parameter at the end,
instead you want to add '-f' as a flag at the beginning.

.args_type = "force:-b,name:s?",
.params = "[-f] name"

By doing this, you no longer need 17/18.

> +        .help       = "save a VM snapshot. To replace existing snapshot use force parameter.",
>          .mhandler.cmd = hmp_vm_snapshot_save,
>      },
>  
>  STEXI
>  @item savevm [@var{tag}|@var{id}]
>  @findex savevm
> -Create a snapshot of the whole virtual machine. If @var{tag} is
> -provided, it is used as human readable identifier. If there is already
> -a snapshot with the same tag or ID, it is replaced. More info at
> -@ref{vm_snapshots}.
> +Create a snapshot of the whole virtual machine. Parameter "name" is optional.
> +If @var{tag} is provided, it is used as human readable identifier. If there is
> +already a snapshot with the same tag, force argument need to be "yes" to

s/force argument need/the force argument needs/

> +++ b/qapi-schema.json
> @@ -2394,21 +2394,23 @@
>  ##
>  # @vm-snapshot-save:
>  #
> -# Create a snapshot of the whole virtual machine. If 'tag' is provided,
> -# it is used as human readable identifier. If there is already a snapshot
> -# with the same tag or ID, it is replaced.
> +# Create a snapshot of the whole virtual machine. Provided 'tag' is used as
> +# human readable identifier. If there is already a snapshot with the same tag,
> +# force argument need to be "yes" to replace it.

s/need to be "yes"/needs to be 'true'/

>  #
>  # The VM is automatically stopped and resumed and saving a snapshot can take
>  # a long time.
>  #
> -# @name: tag or id of new or existing snapshot
> +# @name: tag of new or existing snapshot

Given your new semantics, @name must be tag when @force is false; but
the old HMP semantics allowed 'savevm 1' to rewrite snapshot id 1
regardless of the name, all while keeping the old tag.  Does the new HMP
code do an id lookup, and pass in the correct @name to the QMP code?
Should the QMP code allow the same semantics of being able to pass
@force=true and @name=id instead of the normal creation of @name=tag?

> +#
> +# @force: specify whether existing snapshot is replaced or not

Based on the JSON below, this needs an #optional marking, as well as
mentioning that the default is false.

>  #
>  # Returns: Nothing on success
>  #          If an error occurs, GenericError with error message

Knowing that a creation failed due to a snapshot already existing by the
same name or id might be worth a distinct error class (do we already
have an error class that we could reuse?)
Pavel Hrdina - Aug. 16, 2012, 10:19 a.m.
On 08/16/2012 07:00 AM, Eric Blake wrote:
> On 08/15/2012 01:41 AM, Pavel Hrdina wrote:
>> HMP command "savevm" now takes extra optional force parameter to specifi whether
> s/specifi/specify/
>
>> replace existing snapshot or not.
>>
>> QMP command "vm-snapshot-save" has also extra optional force parameter and
>> name parameter isn't optional anymore.
> When HMP omits the name, what name are you using in it's place?  Either
> a nameless snapshot is okay (and QMP must expose it), or it is an error
> (and HMP must now be passing a reasonable name).
>
> /me reads patch
>
> Oh, there was always a name; the default name was a timestamp, and you
> moved the timestamp creation into HMP.
>
>
>> +++ b/hmp-commands.hx
>> @@ -264,19 +264,19 @@ ETEXI
>>   
>>       {
>>           .name       = "savevm",
>> -        .args_type  = "name:s?",
>> -        .params     = "[tag|id]",
>> -        .help       = "save a VM snapshot. If no tag or id are provided, a new snapshot is created",
>> +        .args_type  = "name:s?,force:b?",
>> +        .params     = "[tag|id] [on|off]",
> Rather than sticking 'force' as an optional parameter at the end,
> instead you want to add '-f' as a flag at the beginning.
>
> .args_type = "force:-b,name:s?",
> .params = "[-f] name"
>
> By doing this, you no longer need 17/18.
Thanks, I'll rewrite it for v2.
>> +        .help       = "save a VM snapshot. To replace existing snapshot use force parameter.",
>>           .mhandler.cmd = hmp_vm_snapshot_save,
>>       },
>>   
>>   STEXI
>>   @item savevm [@var{tag}|@var{id}]
>>   @findex savevm
>> -Create a snapshot of the whole virtual machine. If @var{tag} is
>> -provided, it is used as human readable identifier. If there is already
>> -a snapshot with the same tag or ID, it is replaced. More info at
>> -@ref{vm_snapshots}.
>> +Create a snapshot of the whole virtual machine. Parameter "name" is optional.
>> +If @var{tag} is provided, it is used as human readable identifier. If there is
>> +already a snapshot with the same tag, force argument need to be "yes" to
> s/force argument need/the force argument needs/
>
>> +++ b/qapi-schema.json
>> @@ -2394,21 +2394,23 @@
>>   ##
>>   # @vm-snapshot-save:
>>   #
>> -# Create a snapshot of the whole virtual machine. If 'tag' is provided,
>> -# it is used as human readable identifier. If there is already a snapshot
>> -# with the same tag or ID, it is replaced.
>> +# Create a snapshot of the whole virtual machine. Provided 'tag' is used as
>> +# human readable identifier. If there is already a snapshot with the same tag,
>> +# force argument need to be "yes" to replace it.
> s/need to be "yes"/needs to be 'true'/
>
>>   #
>>   # The VM is automatically stopped and resumed and saving a snapshot can take
>>   # a long time.
>>   #
>> -# @name: tag or id of new or existing snapshot
>> +# @name: tag of new or existing snapshot
> Given your new semantics, @name must be tag when @force is false; but
> the old HMP semantics allowed 'savevm 1' to rewrite snapshot id 1
> regardless of the name, all while keeping the old tag.  Does the new HMP
> code do an id lookup, and pass in the correct @name to the QMP code?
> Should the QMP code allow the same semantics of being able to pass
> @force=true and @name=id instead of the normal creation of @name=tag?
You can override existing snapshot specified by id. I'll fix the 
documentation.
>> +#
>> +# @force: specify whether existing snapshot is replaced or not
> Based on the JSON below, this needs an #optional marking, as well as
> mentioning that the default is false.
>
>>   #
>>   # Returns: Nothing on success
>>   #          If an error occurs, GenericError with error message
> Knowing that a creation failed due to a snapshot already existing by the
> same name or id might be worth a distinct error class (do we already
> have an error class that we could reuse?)
>
Regarding the Luiz's new error handling, for new errors we should use 
one error class.

Patch

diff --git a/hmp-commands.hx b/hmp-commands.hx
index d6cd94c..58fd1f2 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -264,19 +264,19 @@  ETEXI
 
     {
         .name       = "savevm",
-        .args_type  = "name:s?",
-        .params     = "[tag|id]",
-        .help       = "save a VM snapshot. If no tag or id are provided, a new snapshot is created",
+        .args_type  = "name:s?,force:b?",
+        .params     = "[tag|id] [on|off]",
+        .help       = "save a VM snapshot. To replace existing snapshot use force parameter.",
         .mhandler.cmd = hmp_vm_snapshot_save,
     },
 
 STEXI
 @item savevm [@var{tag}|@var{id}]
 @findex savevm
-Create a snapshot of the whole virtual machine. If @var{tag} is
-provided, it is used as human readable identifier. If there is already
-a snapshot with the same tag or ID, it is replaced. More info at
-@ref{vm_snapshots}.
+Create a snapshot of the whole virtual machine. Parameter "name" is optional.
+If @var{tag} is provided, it is used as human readable identifier. If there is
+already a snapshot with the same tag, force argument need to be "yes" to
+replace it. More info at @ref{vm_snapshots}.
 ETEXI
 
     {
diff --git a/hmp.c b/hmp.c
index cf0e036..14df8f1 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1139,9 +1139,32 @@  void hmp_closefd(Monitor *mon, const QDict *qdict)
 void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)
 {
     const char *name = qdict_get_try_str(qdict, "name");
+    char new_name[256];
+    bool force = qdict_get_try_bool(qdict, "force", 0);
     Error *err = NULL;
+#ifdef _WIN32
+    struct _timeb tb;
+    struct tm *ptm;
+#else
+    struct timeval tv;
+    struct tm tm;
+#endif
+
+    if (!name) {
+#ifdef _WIN32
+        time_t t = tb.time;
+        ptm = localtime(&t);
+        strftime(new_name, sizeof(new_name), "vm-%Y%m%d%H%M%S", ptm);
+#else
+        /* cast below needed for OpenBSD where tv_sec is still 'long' */
+        localtime_r((const time_t *)&tv.tv_sec, &tm);
+        strftime(new_name, sizeof(new_name), "vm-%Y%m%d%H%M%S", &tm);
+#endif
+    } else {
+        pstrcpy(new_name, sizeof(new_name), name);
+    }
 
-    qmp_vm_snapshot_save(!!name, name, &err);
+    qmp_vm_snapshot_save(new_name, !!force, force, &err);
 
     hmp_handle_error(mon, &err);
 }
diff --git a/qapi-schema.json b/qapi-schema.json
index 763b51e..e68b259 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2394,21 +2394,23 @@ 
 ##
 # @vm-snapshot-save:
 #
-# Create a snapshot of the whole virtual machine. If 'tag' is provided,
-# it is used as human readable identifier. If there is already a snapshot
-# with the same tag or ID, it is replaced.
+# Create a snapshot of the whole virtual machine. Provided 'tag' is used as
+# human readable identifier. If there is already a snapshot with the same tag,
+# force argument need to be "yes" to replace it.
 #
 # The VM is automatically stopped and resumed and saving a snapshot can take
 # a long time.
 #
-# @name: tag or id of new or existing snapshot
+# @name: tag of new or existing snapshot
+#
+# @force: specify whether existing snapshot is replaced or not
 #
 # Returns: Nothing on success
 #          If an error occurs, GenericError with error message
 #
 # Since: 1.2
 ##
-{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }
+{ 'command': 'vm-snapshot-save', 'data': {'name': 'str', '*force': 'bool'} }
 
 ##
 # @vm-snapshot-load:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 3609de8..c86ceb5 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1112,9 +1112,9 @@  Example:
 EQMP
     {
         .name       = "vm-snapshot-save",
-        .args_type  = "name:s?",
-        .params     = "name",
-        .help       = "save a VM snapshot. If no tag or id are provided, a new snapshot is created",
+        .args_type  = "name:s,force:b?",
+        .params     = "name [force]",
+        .help       = "save a VM snapshot. To replace existing snapshot use force parameter.",
         .mhandler.cmd_new = qmp_marshal_input_vm_snapshot_save
     },
 
@@ -1122,16 +1122,18 @@  SQMP
 vm-snapshot-save
 ------
 
-Create a snapshot of the whole virtual machine. If 'tag' is provided,
-it is used as human readable identifier. If there is already a snapshot
-with the same tag or ID, it is replaced.
+Create a snapshot of the whole virtual machine. Provided 'tag' is used as
+human readable identifier. If there is already a snapshot with the same tag,
+force argument need to be "yes" to replace it.
 
 The VM is automatically stopped and resumed and saving a snapshot can take
 a long time.
 
 Arguments:
 
-- "name": tag or id of new or existing snapshot
+- "name": tag of new or existing snapshot
+
+- "force": specify whether existing snapshot is replaced or not
 
 Example:
 
diff --git a/savevm.c b/savevm.c
index ca7448d..4ec7663 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2100,7 +2100,7 @@  static int del_existing_snapshots(const char *name,
     return 0;
 }
 
-void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp)
+void qmp_vm_snapshot_save(const char *name, bool has_force, bool force, Error **errp)
 {
     BlockDriverState *bs, *bs1;
     QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
@@ -2110,10 +2110,8 @@  void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp)
     uint64_t vm_state_size;
 #ifdef _WIN32
     struct _timeb tb;
-    struct tm *ptm;
 #else
     struct timeval tv;
-    struct tm tm;
 #endif
 
     /* Verify if there is a device that doesn't support snapshots and is writable */
@@ -2153,29 +2151,24 @@  void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp)
 #endif
     sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
 
-    if (has_name) {
-        ret = bdrv_snapshot_find(bs, old_sn, name, NULL);
-        if (ret >= 0) {
+    ret = bdrv_snapshot_find(bs, old_sn, name, NULL);
+    if (ret >= 0) {
+        if (has_force && force) {
             pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
             pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
+
+            del_existing_snapshots(name, errp);
+            if (error_is_set(errp)) {
+                goto the_end;
+            }
         } else {
-            pstrcpy(sn->name, sizeof(sn->name), name);
+            error_set(errp, ERROR_CLASS_GENERIC_ERROR,
+                      "Snapshot '%s' exist. For override specify force=yes",
+                      name);
+            goto the_end;
         }
     } else {
-#ifdef _WIN32
-        time_t t = tb.time;
-        ptm = localtime(&t);
-        strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", ptm);
-#else
-        /* cast below needed for OpenBSD where tv_sec is still 'long' */
-        localtime_r((const time_t *)&tv.tv_sec, &tm);
-        strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
-#endif
-    }
-
-    /* Delete old snapshots of the same name */
-    if (has_name && del_existing_snapshots(name, errp) < 0) {
-        goto the_end;
+        pstrcpy(sn->name, sizeof(sn->name), name);
     }
 
     /* save the VM state */