Re: [Qemu-devel] [RFC PATCH 1/4] qapi: Convert savevm

[Luiz Capitulino]

On Thu, 12 Jul 2012 18:55:15 +0200
Pavel Hrdina <address@hidden> wrote:

> Signed-off-by: Pavel Hrdina <address@hidden>
> ---
>  hmp-commands.hx  |    2 +-
>  hmp.c            |   10 ++++++++++
>  hmp.h            |    1 +
>  qapi-schema.json |   22 ++++++++++++++++++++++
>  qerror.c         |   24 ++++++++++++++++++++++++
>  qerror.h         |   18 ++++++++++++++++++
>  qmp-commands.hx  |   26 ++++++++++++++++++++++++++
>  savevm.c         |   29 ++++++++++++++---------------
>  sysemu.h         |    1 -
>  9 files changed, 116 insertions(+), 17 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index f5d9d91..e8c5325 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -267,7 +267,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_savevm,
>      },
>  
>  STEXI
> diff --git a/hmp.c b/hmp.c
> index 4c6d4ae..491599d 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -1002,3 +1002,13 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
>      qmp_netdev_del(id, &err);
>      hmp_handle_error(mon, &err);
>  }
> +
> +void hmp_savevm(Monitor *mon, const QDict *qdict)
> +{
> +    const char *name = qdict_get_try_str(qdict, "name");
> +    Error *err = NULL;
> +
> +    qmp_savevm(!!name, name, &err);
> +
> +    hmp_handle_error(mon, &err);
> +}
> diff --git a/hmp.h b/hmp.h
> index 79d138d..dc6410b 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -64,5 +64,6 @@ void hmp_device_del(Monitor *mon, const QDict *qdict);
>  void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
>  void hmp_netdev_add(Monitor *mon, const QDict *qdict);
>  void hmp_netdev_del(Monitor *mon, const QDict *qdict);
> +void hmp_savevm(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 1ab5dbd..4db1447 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1868,3 +1868,25 @@
>  # Since: 0.14.0
>  ##
>  { 'command': 'netdev_del', 'data': {'id': 'str'} }
> +
> +##
> +# @savevm:

Let's call it save-vm.

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

Please, document that the vm is automatically stopped and resumed, and that
this can take a long time.

> +#
> +# @name: tag or id of new or existing snapshot

I don't like the idea of making 'name' optional in QMP. We could (and should)
have it as optional in HMP though. This means that we should move the code
that auto-generates it to HMP.

Also, I remember an old discussion about distinguishing between 'tag' and 'id'.
I remember it was difficult to do, so we could choose not to do it, but let's
re-evaluate this again.

> +#
> +# Returns: Nothing on success
> +#          If there is a writable device not supporting snapshots,
> +#            SnapshotNotSupported
> +#          If no block device can accept snapshots, SnapshotNotAccepted
> +#          If an error occures while creating a snapshot, 
> SnapshotCreateFailed
> +#          If open a block device for vm state fail, SnapshotOpenFailed
> +#          If an error uccures while writing vm state, SnapshotWriteFailed
> +#          If delete snapshot with same 'name' fail, SnapshotDeleteFailed

I don't think we should re-create all these errors, more comments on that
below.

> +#
> +# Since: 1.2
> +##
> +{ 'command': 'savevm', 'data': {'*name': 'str'} }
> \ No newline at end of file
> diff --git a/qerror.c b/qerror.c
> index 92c4eff..4e6efa1 100644
> --- a/qerror.c
> +++ b/qerror.c
> @@ -309,6 +309,30 @@ static const QErrorStringTable qerror_table[] = {
>          .desc      = "Could not start VNC server on %(target)",
>      },
>      {
> +        .error_fmt = QERR_SNAPSHOT_CREATE_FAILED,
> +        .desc      = "Error '%(errno)' while creating snapshot on 
> '%(device)'",
> +    },
> +    {
> +        .error_fmt = QERR_SNAPSHOT_DELETE_FAILED,
> +        .desc      = "Error '%(errno)' while deleting snapshot on 
> '%(device)'",
> +    },
> +    {
> +        .error_fmt = QERR_SNAPSHOT_NOT_ACCEPTED,
> +        .desc      = "No block device can accept snapshots",
> +    },
> +    {
> +        .error_fmt = QERR_SNAPSHOT_NOT_SUPPORTED,
> +        .desc      = "Device '%(device)' is writable but does not support 
> snapshots",
> +    },
> +    {
> +        .error_fmt = QERR_SNAPSHOT_OPEN_FAILED,
> +        .desc      = "Error while opening snapshot on '%(device)'",
> +    },
> +    {
> +        .error_fmt = QERR_SNAPSHOT_WRITE_FAILED,
> +        .desc      = "Error '%(errno)' while writing snapshot on 
> '%(device)'",
> +    },
> +    {
>          .error_fmt = QERR_SOCKET_CONNECT_IN_PROGRESS,
>          .desc      = "Connection can not be completed immediately",
>      },
> diff --git a/qerror.h b/qerror.h
> index b4c8758..bc47f4a 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -251,6 +251,24 @@ QError *qobject_to_qerror(const QObject *obj);
>  #define QERR_VNC_SERVER_FAILED \
>      "{ 'class': 'VNCServerFailed', 'data': { 'target': %s } }"
>  
> +#define QERR_SNAPSHOT_CREATE_FAILED \
> +    "{ 'class': 'SnapshotCreateFailed', 'data': { 'device': %s, 'errno': %d 
> } }"
> +
> +#define QERR_SNAPSHOT_DELETE_FAILED \
> +    "{ 'class': 'SnapshotDeleteFailed', 'data': { 'device': %s, 'errno': %d 
> } }"
> +
> +#define QERR_SNAPSHOT_NOT_ACCEPTED \
> +    "{ 'class': 'SnapshotNotAccepted', 'data': {} }"
> +
> +#define QERR_SNAPSHOT_NOT_SUPPORTED \
> +    "{ 'class': 'SnapshotNotSupported', 'data': { 'device': %s } }"
> +
> +#define QERR_SNAPSHOT_OPEN_FAILED \
> +    "{ 'class': 'SnapshotOpenFailed', 'data': { 'device': %s } }"
> +
> +#define QERR_SNAPSHOT_WRITE_FAILED \
> +    "{ 'class': 'SnapshotWriteFailed', 'data': { 'device': %s, 'errno': %d } 
> }"
> +
>  #define QERR_SOCKET_CONNECT_IN_PROGRESS \
>      "{ 'class': 'SockConnectInprogress', 'data': {} }"
>  
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 2e1a38e..a1c82f7 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1061,6 +1061,32 @@ Example:
>  
>  EQMP
>      {
> +        .name       = "savevm",
> +        .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_savevm
> +    },
> +
> +SQMP
> +savevm
> +------
> +
> +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.
> +
> +Arguments:
> +
> +- "name": tag or id of new or existing snapshot
> +
> +Example:
> +
> +-> { "execute": "savevm", "arguments": { "name": "my_snapshot" } }
> +<- { "return": {} }
> +
> +EQMP
> +    {
>          .name       = "qmp_capabilities",
>          .args_type  = "",
>          .params     = "",
> diff --git a/savevm.c b/savevm.c
> index a15c163..9fc1b53 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2033,7 +2033,7 @@ static int bdrv_snapshot_find(BlockDriverState *bs, 
> QEMUSnapshotInfo *sn_info,
>  /*
>   * Deletes snapshots of a given name in all opened images.
>   */
> -static int del_existing_snapshots(Monitor *mon, const char *name)
> +static int del_existing_snapshots(Error **errp, const char *name)
>  {
>      BlockDriverState *bs;
>      QEMUSnapshotInfo sn1, *snapshot = &sn1;
> @@ -2046,9 +2046,8 @@ static int del_existing_snapshots(Monitor *mon, const 
> char *name)
>          {
>              ret = bdrv_snapshot_delete(bs, name);
>              if (ret < 0) {
> -                monitor_printf(mon,
> -                               "Error while deleting snapshot on '%s'\n",
> -                               bdrv_get_device_name(bs));
> +                error_set(errp, QERR_SNAPSHOT_DELETE_FAILED,
> +                          bdrv_get_device_name(bs), ret);

The Right Thing to do here is to change bdrv_snapshot_delete() to take an Error
argument and let it set the real error cause. Three considerations:

 1. The QMP command shouldn't delete existing snapshots by default. Either,
    we add a 'force' argument to it or don't delete snapshots in save-vm
    at all (in which case a mngt app would have to delete the snapshots with the
    same name manually, I prefer this approach btw)

 2. This has to be done in a separate series

 3. It's a good idea to wait for my series improving error_set() to be merged
    before doing this

>                  return -1;
>              }
>          }
> @@ -2057,7 +2056,7 @@ static int del_existing_snapshots(Monitor *mon, const 
> char *name)
>      return 0;
>  }
>  
> -void do_savevm(Monitor *mon, const QDict *qdict)
> +void qmp_savevm(bool has_name, const char *name, Error **errp)
>  {
>      BlockDriverState *bs, *bs1;
>      QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
> @@ -2072,7 +2071,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;
> @@ -2083,15 +2081,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_set(errp, QERR_SNAPSHOT_NOT_SUPPORTED,
> +                      bdrv_get_device_name(bs));

Please, use QERR_NOT_SUPPORTED instead. I know it doesn't allow any arguments
today, but I'm working on a series which will allow you to pass any arguments
you wish.

>              return;
>          }
>      }
>  
>      bs = bdrv_snapshots();
>      if (!bs) {
> -        monitor_printf(mon, "No block device can accept snapshots\n");
> +        error_set(errp, QERR_SNAPSHOT_NOT_ACCEPTED);

I think we should use QERR_NOT_SUPPORTED here too.

>          return;
>      }
>  
> @@ -2112,7 +2110,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);
>          if (ret >= 0) {
>              pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
> @@ -2133,21 +2131,22 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>      }
>  
>      /* Delete old snapshots of the same name */
> -    if (name && del_existing_snapshots(mon, name) < 0) {
> +    if (has_name && del_existing_snapshots(errp, name) < 0) {
>          goto the_end;
>      }

The QMP command should not delete existing snapshots, as said above.

>  
>      /* save the VM state */
>      f = qemu_fopen_bdrv(bs, 1);
>      if (!f) {
> -        monitor_printf(mon, "Could not open VM state file\n");
> +        error_set(errp, QERR_SNAPSHOT_OPEN_FAILED, bdrv_get_device_name(bs));

Please, use QERR_OPEN_FILE_FAILED instead. The filename should be the backing
file name.

>          goto the_end;
>      }
>      ret = qemu_savevm_state(f);
>      vm_state_size = qemu_ftell(f);
>      qemu_fclose(f);
>      if (ret < 0) {
> -        monitor_printf(mon, "Error %d while writing VM\n", ret);
> +        error_set(errp, QERR_SNAPSHOT_WRITE_FAILED,
> +                  bdrv_get_device_name(bs), ret);

The Right Thing to do here it to convert qemu_savevm_sstate() to take
an Error argument. The same considerations I wrote above about
del_existing_snapshots() apply here, except that you can report IOError
for most qemu_savevm_state() errors.

>          goto the_end;
>      }
>  
> @@ -2160,8 +2159,8 @@ void do_savevm(Monitor *mon, const QDict *qdict)
>              sn->vm_state_size = (bs == bs1 ? vm_state_size : 0);
>              ret = bdrv_snapshot_create(bs1, sn);
>              if (ret < 0) {
> -                monitor_printf(mon, "Error while creating snapshot on 
> '%s'\n",
> -                               bdrv_get_device_name(bs1));
> +                error_set(errp, QERR_SNAPSHOT_CREATE_FAILED,
> +                          bdrv_get_device_name(bs1), ret);

Here too, bdrv_snapshot_create() should be changed to take an Error
argument.

>              }
>          }
>      }
> diff --git a/sysemu.h b/sysemu.h
> index 6540c79..95d1207 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -69,7 +69,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);