Patchwork [v4,11/11] savevm: add force parameter to HMP command and return snapshot info

login
register
mail settings
Submitter Pavel Hrdina
Date March 29, 2013, 2:12 p.m.
Message ID <f8e52c68ad53b2020ac0e1eaaa3c6bfa853e4dfb.1364565637.git.phrdina@redhat.com>
Download mbox | patch
Permalink /patch/232423/
State New
Headers show

Comments

Pavel Hrdina - March 29, 2013, 2:12 p.m.
HMP command "savevm" now takes extra optional force parameter to specify
whether replace existing snapshot or not. It also returns information
about created snapshot.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 hmp-commands.hx | 16 ++++++++--------
 hmp.c           | 18 +++++++++++++++++-
 2 files changed, 25 insertions(+), 9 deletions(-)
Markus Armbruster - April 9, 2013, 4:45 p.m.
Pavel Hrdina <phrdina@redhat.com> writes:

> HMP command "savevm" now takes extra optional force parameter to specify
> whether replace existing snapshot or not. It also returns information
> about created snapshot.

It doesn't return anything, it prints :)

> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  hmp-commands.hx | 16 ++++++++--------
>  hmp.c           | 18 +++++++++++++++++-
>  2 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 382b87d..9719cc0 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -307,19 +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",

Humor me: semicolon instead of comma.

>          .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
> -@ref{vm_snapshots}.
> +Create a snapshot of the whole virtual machine. Parameter "name" is optional.

Huh?  There is no parameter "name".

> +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}.

Suggest "needs to be specified to replace it".

>  ETEXI
>  
>      {

Since you're touching the texinfo in this patch anyway, suggest to move
the markup fix from PATCH 07 here.  Only if you respin anyway.

> diff --git a/hmp.c b/hmp.c
> index b38b6ce..151e48b 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1437,10 +1437,26 @@ 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;
>      SnapshotInfo *info = NULL;
>  
> -    info = qmp_vm_snapshot_save(!!name, name, true, true, &err);
> +    info = qmp_vm_snapshot_save(!!name, name, !!force, force, &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);

Need to copy because QAPI type SnapshotInfo duplicates QEMUSnapshotInfo.
Cleaning that up is outside the scope of this series.

> +        monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));

Printing this makes lots of sense :)

Stick in a print of bdrv_snapshot_dump(..., NULL) to get a nice header,
like we do elsewhere?

> +    }
> +
>      qapi_free_SnapshotInfo(info);
>      hmp_handle_error(mon, &err);
>  }

Patch

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 382b87d..9719cc0 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -307,19 +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
-@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 @var{tag} or @var{id}, @var{-f} flag needs to
+be specified. More info at @ref{vm_snapshots}.
 ETEXI
 
     {
diff --git a/hmp.c b/hmp.c
index b38b6ce..151e48b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1437,10 +1437,26 @@  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;
     SnapshotInfo *info = NULL;
 
-    info = qmp_vm_snapshot_save(!!name, name, true, true, &err);
+    info = qmp_vm_snapshot_save(!!name, name, !!force, force, &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, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));
+    }
+
     qapi_free_SnapshotInfo(info);
     hmp_handle_error(mon, &err);
 }