Re: [Qemu-devel] [PATCH v4 11/11] savevm: add force parameter to HMP command and return snapshot info

[Markus Armbruster]

Pavel Hrdina <address@hidden> 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 <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
>  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
> address@hidden savevm address@hidden|@var{id}]
> address@hidden savevm address@hidden @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
> address@hidden
> +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);
>  }