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 <address@hidden>
---
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.