Re: [Qemu-devel] [PATCH 07/11] qapi: Convert loadvm

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 9203 bytes --]

On 04/16/2013 10:05 AM, Pavel Hrdina wrote:
> QMP command vm-snapshot-load and HMP command loadvm behave similar to
> vm-snapshot-delete and delvm. The only different is that they will load
> the snapshot instead of deleting it.
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  hmp-commands.hx         | 16 +++++-----
>  hmp.c                   | 35 ++++++++++++++++++++++
>  hmp.h                   |  1 +
>  include/sysemu/sysemu.h |  1 -
>  monitor.c               | 12 --------
>  qapi-schema.json        | 18 +++++++++++
>  qmp-commands.hx         | 38 ++++++++++++++++++++++++
>  savevm.c                | 79 ++++++++++++++++++++++++++++++-------------------
>  vl.c                    |  7 ++++-
>  9 files changed, 156 insertions(+), 51 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index d1701ce..e80410b 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -324,17 +324,19 @@ ETEXI
>  
>      {
>          .name       = "loadvm",
> -        .args_type  = "name:s",
> -        .params     = "tag|id",
> -        .help       = "restore a VM snapshot from its tag or id",
> -        .mhandler.cmd = do_loadvm,
> +        .args_type  = "id:-i,name:s",
> +        .params     = "[-i] tag|[id]",
> +        .help       = "restore a VM snapshot from its tag or id if -i flag is provided",

Same comments as 4/11 about possibilities of making this look nicer
('name' seems nicer than 'tag|[id]').

>  }
> +
> +void hmp_vm_snapshot_load(Monitor *mon, const QDict *qdict)
> +{
> +    const char *name = qdict_get_try_str(qdict, "name");
> +    const bool id = qdict_get_try_bool(qdict, "id", false);
> +    Error *local_err = NULL;
> +    SnapshotInfo *info = NULL;
> +
> +    if (id) {
> +        info = qmp_vm_snapshot_load(false, NULL, true, name, &local_err);
> +    } else {
> +        info = qmp_vm_snapshot_load(true, name, false, NULL, &local_err);
> +    }

Could be written without if/else:
qmp_vm_snapshot_load(id, name, !id, name, &local_err);

but doesn't really bother me as written.

> +
> +    if (info) {
> +        char buf[256];

Fixed-size buffer...

> +        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, "Loaded snapshot's info:\n");
> +        monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
> +        monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));

...but potentially large amount of information to be placed in it.  I
would MUCH rather see this code using gstring, or even direct printing.
 I think you need to base this on top of the ideas here:

https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg02540.html

These two series need to be coordinated to minimize churn and rebase
conflicts on cleanups such as removing buffer truncation from
bdrv_snapshot_dump.

Hmm, I probably should have mentioned this on 4/11 as well.

> +    } else if (!error_is_set(&local_err)) {

Same comments as 4/11, in that info == NULL should imply that local_err
is set.

> +++ b/qapi-schema.json
> @@ -3522,3 +3522,21 @@
>  ##
>  { 'command': 'vm-snapshot-delete', 'data': {'*name': 'str', '*id': 'str'},
>    'returns': 'SnapshotInfo' }
> +
> +##
> +# @vm-snapshot-load:
> +#
> +# Set the whole virtual machine to the snapshot identified by the tag
> +# or the unique snapshot id or both. It must be a snapshot of a whole VM and
> +# at least one of the name or id parameter must be specified.
> +#
> +# @name: tag of existing snapshot
> +#
> +# @id: id of existing snapshot

As in 4/11, mark these with #optional.

> +#
> +# Returns: SnapshotInfo on success
> +#
> +# Since: 1.5
> +##
> +{ 'command': 'vm-snapshot-load', 'data': {'*name': 'str', '*id': 'str'},
> +  'returns': 'SnapshotInfo' }

Same comments as 4/11 on return value design decisions.

> @@ -2393,26 +2393,35 @@ void qmp_xen_save_devices_state(const char *filename, Error **errp)
>          vm_start();
>  }
>  
> -int load_vmstate(const char *name)
> +SnapshotInfo *qmp_vm_snapshot_load(bool has_name, const char *name,
> +                                   bool has_id, const char *id, Error **errp)
>  {
>      BlockDriverState *bs, *bs_vm_state;
>      QEMUSnapshotInfo sn;
>      QEMUFile *f;
> -    Error *local_err;
> +    SnapshotInfo *info = NULL;
> +    int saved_vm_running;

This should be bool.  runstate_is_running() is currently typed as int,
but it defers to runstate_check() which is bool; fixing runstate_is* to
consistently use bool would be worthy of a separate cleanup patch.

> +
> +    if (!has_name && !has_id) {
> +        error_setg(errp, "name or id must be specified");
> +        return NULL;
> +    }
>  
>      bs_vm_state = bdrv_snapshots();
>      if (!bs_vm_state) {
> -        error_report("No block device supports snapshots");
> -        return -ENOTSUP;
> +        error_setg(errp, "no block device supports snapshots");
> +        return NULL;
>      }
>  
>      /* Don't even try to load empty VM states */
> -    if (!bdrv_snapshot_find(bs_vm_state, &sn, name, name, true)) {
> -        return -ENOENT;
> -    } else if (sn.vm_state_size == 0) {
> -        error_report("This is a disk-only snapshot. Revert to it offline "
> -            "using qemu-img.");
> -        return -EINVAL;
> +    if (!bdrv_snapshot_find(bs_vm_state, &sn, name, id, false)) {

Oh, I didn't notice this before, but 4/11 has the same issue.  You are
not guaranteed that 'name' and 'id' are initialized correctly unless you
first check 'has_name' and 'has_id'.  And given my suggestion above of
possibly compressing things to:

qmp_vm_snapshot_load(id, name, !id, name, &local_err);

then it REALLY matters that you pass NULL for the parameters that are
not provided by the caller.  To be safe, you must use one of:

if (!has_name) {
    name = NULL;
}
if (!has_id) {
    id = NULL;
}
... bdrv_snapshot_find(bs_vm_state, &sn, name, id, false) ...

or:

bdrv_snapshot_find(bs_vm_state, &sn,
                   has_name ? name : NULL,
                   has_id ? id : NULL,
                   false)

unless you take on the bigger task of ensuring that ALL callers pass in
NULL when has_name is false (and the code generator for QMP currently
doesn't guarantee that).

> +        return NULL;
> +    }
> +
> +    if (sn.vm_state_size == 0) {
> +        error_setg(errp, "this is a disk-only snapshot, revert to it offline "
> +            "using qemu-img");

Indentation is off; a continuation should line up to the character after
the '(' of the line before.

> +        return NULL;
>      }
>  
>      /* Verify if there is any device that doesn't support snapshots and is
> @@ -2425,29 +2434,28 @@ int load_vmstate(const char *name)
>          }
>  
>          if (!bdrv_can_snapshot(bs)) {
> -            error_report("Device '%s' is writable but does not support snapshots.",
> -                               bdrv_get_device_name(bs));
> -            return -ENOTSUP;
> +            error_setg(errp, "device '%s' is writable but does not support "
> +                       "snapshots", bdrv_get_device_name(bs));
> +            return NULL;
>          }

Pre-existing, but why do we care?  Loading a snapshot doesn't require
writing, just reading.

>  
> -        if (!bdrv_snapshot_find(bs, &sn, name, name, true)) {
> -            error_report("Device '%s' does not have the requested snapshot '%s'",
> -                           bdrv_get_device_name(bs), name);
> -            return -ENOENT;
> +        if (!bdrv_snapshot_find(bs, &sn, name, id, false)) {
> +            return NULL;

Missing error report.

>          }
>      }
>  
> +    saved_vm_running = runstate_is_running();
> +    vm_stop(RUN_STATE_RESTORE_VM);
> +
>      /* Flush all IO requests so they don't interfere with the new state.  */
>      bdrv_drain_all();
>  
>      bs = NULL;
>      while ((bs = bdrv_next(bs))) {
>          if (bdrv_can_snapshot(bs)) {
> -            bdrv_snapshot_goto(bs, name, &local_err);
> -            if (error_is_set(&local_err)) {
> -                error_report("%s", error_get_pretty(local_err));
> -                error_free(local_err);
> -                return -1;
> +            bdrv_snapshot_goto(bs, sn.name, errp);
> +            if (error_is_set(errp)) {
> +                return NULL;

Pre-existing, but yet another case of partial failure, where it might be
nicer to let the caller know if we failed halfway through loading a
located snapshot, differently than complete failure where we didn't
change any state.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]