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 <phrd...@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
signature.asc
Description: OpenPGP digital signature
