Patchwork [v2,11/12] vm-snapshot-save: add force parameter

login
register
mail settings
Submitter Pavel Hrdina
Date March 22, 2013, 1:16 p.m.
Message ID <a1441b4d63ee1a4531f1492c82aa2db2e916e7f8.1363957855.git.phrdina@redhat.com>
Download mbox | patch
Permalink /patch/230020/
State New
Headers show

Comments

Pavel Hrdina - March 22, 2013, 1:16 p.m.
HMP command "savevm" now takes extra optional force parameter to specify 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  | 15 ++++++++-------
 hmp.c            |  3 ++-
 qapi-schema.json |  7 +++++--
 qmp-commands.hx  | 16 ++++++++++------
 savevm.c         | 25 ++++++++++++++++---------
 5 files changed, 41 insertions(+), 25 deletions(-)
Eric Blake - March 26, 2013, 10:04 p.m.
On 03/22/2013 07:16 AM, Pavel Hrdina wrote:
> HMP command "savevm" now takes extra optional force parameter to specify whether
> replace existing snapshot or not.
> 
> QMP command "vm-snapshot-save" has also extra optional force parameter and
> name parameter isn't optional anymore.

Again, my question from 7/12 - why not rebase this series to implement
QMP correctly from the getgo, and have this patch be JUST an HMP
addition to expose the new QMP capability?

> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  hmp-commands.hx  | 15 ++++++++-------
>  hmp.c            |  3 ++-
>  qapi-schema.json |  7 +++++--
>  qmp-commands.hx  | 16 ++++++++++------
>  savevm.c         | 25 ++++++++++++++++---------
>  5 files changed, 41 insertions(+), 25 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 3c1cb05..bfe0645 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -307,18 +307,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  = "force:-f,name:s?",
> +        .params     = "[-f] [tag|id]",
> +        .help       = "save a VM snapshot. To replace existing snapshot use force flag.",

Not an error message, so I'm not duplicating myself :)
Prevailing convention seems to be omitting trailing '.' in .help messages.

>          .mhandler.cmd = hmp_vm_snapshot_save,
>      },
>  
>  STEXI
> -@item savevm [@var{tag}|@var{id}]
> +@item savevm [@var{-f}] @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 @var{tag} or @var{id}, it is replaced. More info at
> +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 @var{tag} or @var{id}, @var{-f} flag needs to
> +be specified. More info at @ref{vm_snapshots}.
>  @ref{vm_snapshots}.

@ref{vm_snapshots} now appears twice in the output.

Patch

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 3c1cb05..bfe0645 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -307,18 +307,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  = "force:-f,name:s?",
+        .params     = "[-f] [tag|id]",
+        .help       = "save a VM snapshot. To replace existing snapshot use force flag.",
         .mhandler.cmd = hmp_vm_snapshot_save,
     },
 
 STEXI
-@item savevm [@var{tag}|@var{id}]
+@item savevm [@var{-f}] @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 @var{tag} or @var{id}, it is replaced. More info at
+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 @var{tag} or @var{id}, @var{-f} flag needs to
+be specified. More info at @ref{vm_snapshots}.
 @ref{vm_snapshots}.
 ETEXI
 
diff --git a/hmp.c b/hmp.c
index 914d88c..ff20917 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1427,8 +1427,9 @@  void hmp_chardev_remove(Monitor *mon, const QDict *qdict)
 void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)
 {
     const char *name = qdict_get_try_str(qdict, "name");
+    bool force = qdict_get_try_bool(qdict, "force", 0);
     Error *err = NULL;
 
-    qmp_vm_snapshot_save(!!name, name, &err);
+    qmp_vm_snapshot_save(!!name, name, !!force, force, &err);
     hmp_handle_error(mon, &err);
 }
diff --git a/qapi-schema.json b/qapi-schema.json
index 87842d3..9bceb8f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3448,15 +3448,18 @@ 
 #
 # Create a snapshot of the whole virtual machine. If tag is provided as @name,
 # it is used as human readable identifier. If there is already a snapshot
-# with the same tag or ID, it is replaced.
+# with the same tag or id, the force argument needs to be true to replace it.
 #
 # The VM is automatically stopped and resumed and saving a snapshot can take
 # a long time.
 #
 # @name: #optional tag of new snapshot or tag|id of existing snapshot
 #
+# @force: #optional specify whether existing snapshot is replaced or not,
+#         default is false
+#
 # Returns: Nothing on success
 #
 # Since: 1.5
 ##
-{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }
+{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str', '*force': 'bool'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 0f5e544..7e02131 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1424,9 +1424,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
     },
 
@@ -1435,15 +1435,19 @@  vm-snapshot-save
 ------
 
 Create a snapshot of the whole virtual machine. If tag is provided as name,
-it is used as human readable identifier. If there is already a snapshot
-with the same tag or id, it is replaced.
+it is used as human readable identifier. If there is already a snapshot with
+the same tag, the force argument needs to be true to replace it.
 
 The VM is automatically stopped and resumed and saving a snapshot can take
 a long time.
 
 Arguments:
 
-- "name": #optional tag of new snapshot or tag|id of existing snapshot (json-string, optional)
+- "name": tag of new snapshot or tag|id of existing snapshot
+          (json-string, optional)
+
+- "force": specify whether existing snapshot is replaced or not,
+           default is false (json-bool, optional)
 
 Example:
 
diff --git a/savevm.c b/savevm.c
index 2e5f029..3e3d5f8 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2135,7 +2135,8 @@  static int del_existing_snapshots(const char *name, Error **errp)
     return 0;
 }
 
-void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp)
+void qmp_vm_snapshot_save(bool has_name, const char *name, bool has_force,
+                          bool force, Error **errp)
 {
     BlockDriverState *bs, *bs1;
     QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
@@ -2182,8 +2183,20 @@  void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp)
     if (has_name) {
         ret = bdrv_snapshot_find(bs, old_sn, name);
         if (ret >= 0) {
-            pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
-            pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
+            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);
+
+                /* Delete old snapshots of the same name */
+                if (del_existing_snapshots(name, &local_err) < 0) {
+                    error_propagate(errp, local_err);
+                    goto the_end;
+                }
+            } else {
+                error_setg(errp, "Snapshot '%s' exists. For override use "
+                           "'force' parameter.", name);
+                goto the_end;
+            }
         } else {
             pstrcpy(sn->name, sizeof(sn->name), name);
         }
@@ -2193,12 +2206,6 @@  void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp)
         strftime(sn->name, sizeof(sn->name), "vm-%Y%m%d%H%M%S", &tm);
     }
 
-    /* Delete old snapshots of the same name */
-    if (has_name && del_existing_snapshots(name, &local_err) < 0) {
-        error_propagate(errp, local_err);
-        goto the_end;
-    }
-
     /* save the VM state */
     f = qemu_fopen_bdrv(bs, 1);
     if (!f) {