[v2,13/17] qapi: Convert savevm

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 hmp-commands.hx  |  4 ++--
 hmp.c            |  9 +++++++++
 hmp.h            |  1 +
 qapi-schema.json | 19 +++++++++++++++++++
 qmp-commands.hx  | 29 +++++++++++++++++++++++++++++
 savevm.c         | 26 ++++++++++----------------
 sysemu.h         |  1 -
 7 files changed, 70 insertions(+), 19 deletions(-)

On Thu, 13 Dec 2012 16:40:47 +0100
Pavel Hrdina <phrdina@redhat.com> wrote:

> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  hmp-commands.hx  |  4 ++--
>  hmp.c            |  9 +++++++++
>  hmp.h            |  1 +
>  qapi-schema.json | 19 +++++++++++++++++++
>  qmp-commands.hx  | 29 +++++++++++++++++++++++++++++
>  savevm.c         | 26 ++++++++++----------------
>  sysemu.h         |  1 -
>  7 files changed, 70 insertions(+), 19 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 010b8c9..3b781f7 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -310,7 +310,7 @@ ETEXI
>          .args_type  = "name:s?",
>          .params     = "[tag|id]",
>          .help       = "save a VM snapshot. If no tag or id are provided, a new snapshot is created",
> -        .mhandler.cmd = do_savevm,
> +        .mhandler.cmd = hmp_vm_snapshot_save,
>      },
>  
>  STEXI
> @@ -318,7 +318,7 @@ STEXI
>  @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
> +a snapshot with the same @var{tag} or @var{id}, it is replaced. More info at
>  @ref{vm_snapshots}.
>  ETEXI
>  
> diff --git a/hmp.c b/hmp.c
> index 180ba2b..2dbdcbf 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1335,3 +1335,12 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
>      qmp_nbd_server_stop(&errp);
>      hmp_handle_error(mon, &errp);
>  }
> +
> +void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict)
> +{
> +    const char *name = qdict_get_try_str(qdict, "name");
> +    Error *err = NULL;
> +
> +    qmp_vm_snapshot_save(!!name, name, &err);
> +    hmp_handle_error(mon, &err);
> +}
> diff --git a/hmp.h b/hmp.h
> index 0ab03be..0bd7e47 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -80,5 +80,6 @@ void hmp_screen_dump(Monitor *mon, const QDict *qdict);
>  void hmp_nbd_server_start(Monitor *mon, const QDict *qdict);
>  void hmp_nbd_server_add(Monitor *mon, const QDict *qdict);
>  void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict);
> +void hmp_vm_snapshot_save(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5dfa052..08543d2 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3017,3 +3017,22 @@
>  # Since: 1.3.0
>  ##
>  { 'command': 'nbd-server-stop' }
> +
> +##
> +# @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.
> +#
> +# 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

I wonder if we should allow 'name' to be optional in QMP.

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

You can drop this last line.

> +#
> +# Since: 1.4.0
> +##
> +{ 'command': 'vm-snapshot-save', 'data': {'*name': 'str'} }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 5c692d0..58e2d0a 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1348,6 +1348,35 @@ 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",
> +        .mhandler.cmd_new = qmp_marshal_input_vm_snapshot_save
> +    },
> +
> +SQMP
> +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.
> +
> +The VM is automatically stopped and resumed and saving a snapshot can take
> +a long time.
> +
> +Arguments:
> +
> +- "name": tag of new snapshot or tag|id of existing snapshot (json-string, optional)
> +
> +Example:
> +
> +-> { "execute": "vm-snapshot-save", "arguments": { "name": "my_snapshot" } }
> +<- { "return": {} }
> +
> +EQMP
> +    {
>          .name       = "qmp_capabilities",
>          .args_type  = "",
>          .params     = "",
> diff --git a/savevm.c b/savevm.c
> index 2c63aad..91120c4 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2141,7 +2141,7 @@ static int del_existing_snapshots(const char *name,
>      return 0;
>  }
>  
> -void do_savevm(Monitor *mon, const QDict *qdict)
> +void qmp_vm_snapshot_save(bool has_name, const char *name, Error **errp)
>  {
>      BlockDriverState *bs, *bs1;
>      QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
> @@ -2156,7 +2156,6 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>      struct timeval tv;
>      struct tm tm;
>  #endif
> -    const char *name = qdict_get_try_str(qdict, "name");
>  
>      /* Verify if there is a device that doesn't support snapshots and is writable */
>      bs = NULL;
> @@ -2167,15 +2166,15 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>          }
>  
>          if (!bdrv_can_snapshot(bs)) {
> -            monitor_printf(mon, "Device '%s' is writable but does not support snapshots.\n",
> -                               bdrv_get_device_name(bs));
> +            error_setg(errp, "Device '%s' is writable but does not support "
> +                       "snapshots.", bdrv_get_device_name(bs));
>              return;
>          }
>      }
>  
>      bs = bdrv_snapshots();
>      if (!bs) {
> -        monitor_printf(mon, "No block device can accept snapshots\n");
> +        error_setg(errp, "No block device can accept snapshots.");
>          return;
>      }
>  
> @@ -2196,7 +2195,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>  #endif
>      sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
>  
> -    if (name) {
> +    if (has_name) {
>          ret = bdrv_snapshot_find(bs, old_sn, name, NULL);
>          if (ret >= 0) {
>              pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
> @@ -2217,21 +2216,20 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>      }
>  
>      /* Delete old snapshots of the same name */
> -    if (name && del_existing_snapshots(name, NULL) < 0) {
> +    if (has_name && del_existing_snapshots(name, errp) < 0) {

You shouldn't use errp (the argument passed to qmp_vm_snapshot_save) to
detect internal error. You should have Error *local_err, pass it instead
and propagate errors with error_propagate().

>          goto the_end;
>      }
>  
>      /* save the VM state */
>      f = qemu_fopen_bdrv(bs, 1);
>      if (!f) {
> -        monitor_printf(mon, "Could not open VM state file\n");
> +        error_setg(errp, "Failed to open '%s' file.", bdrv_get_device_name(bs));
>          goto the_end;
>      }
> -    ret = qemu_savevm_state(f, NULL);
> +    ret = qemu_savevm_state(f, errp);
>      vm_state_size = qemu_ftell(f);
>      qemu_fclose(f);
> -    if (ret < 0) {
> -        monitor_printf(mon, "Error %d while writing VM\n", ret);
> +    if (error_is_set(errp)) {
>          goto the_end;
>      }
>  
> @@ -2242,11 +2240,7 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>          if (bdrv_can_snapshot(bs1)) {
>              /* Write VM state size only to the image that contains the state */
>              sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
> -            ret = bdrv_snapshot_create(bs1, sn, NULL);
> -            if (ret < 0) {
> -                monitor_printf(mon, "Error while creating snapshot on '%s'\n",
> -                               bdrv_get_device_name(bs1));
> -            }
> +            bdrv_snapshot_create(bs1, sn, errp);
>          }
>      }
>  
> diff --git a/sysemu.h b/sysemu.h
> index c08f7a3..96b4879 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -65,7 +65,6 @@ void qemu_remove_exit_notifier(Notifier *notify);
>  
>  void qemu_add_machine_init_done_notifier(Notifier *notify);
>  
> -void do_savevm(Monitor *mon, const QDict *qdict);
>  int load_vmstate(const char *name);
>  void do_delvm(Monitor *mon, const QDict *qdict);
>  void do_info_snapshots(Monitor *mon);

On 12/14/2012 09:57 AM, Luiz Capitulino wrote:

>> +##
>> +# @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.
>> +#
>> +# 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
> 
> I wonder if we should allow 'name' to be optional in QMP.

Libvirt will always pass a 'name'.  Where it gets confusing is that
'name' can be all digits; so if I say a name of '3', but it gets id '2',
then future operations get way confusing (especially if later we also
create a snapshot whose id becomes 3).

> 
>> +#
>> +# Returns: Nothing on success

One thing is for certain - if 'name' remains optional, then you MUST
return the 'id' that was auto-allocated.  And even if name is not
optional, returning the 'id' that was either auto-allocated or
successfully looked up as an existing snapshot would be more useful than
returning nothing.