* [PATCH v3 0/7] migration: bring improved savevm/loadvm/delvm to QMP
@ 2020-08-27 11:15 Daniel P. Berrangé
2020-08-27 11:16 ` [PATCH v3 1/7] migration: improve error reporting of block driver state name Daniel P. Berrangé
` (7 more replies)
0 siblings, 8 replies; 19+ messages in thread
From: Daniel P. Berrangé @ 2020-08-27 11:15 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Peter Krempa, Daniel P. Berrangé,
Denis V. Lunev, qemu-block, Juan Quintela, John Snow,
Markus Armbruster, Dr. David Alan Gilbert, Pavel Dovgalyuk,
Paolo Bonzini, Max Reitz
v1: https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg00866.html
v2: https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg07523.html
When QMP was first introduced some 10+ years ago now, the snapshot
related commands (savevm/loadvm/delvm) were not converted. This was
primarily because their implementation causes blocking of the thread
running the monitor commands. This was (and still is) considered
undesirable behaviour both in HMP and QMP.
In theory someone was supposed to fix this flaw at some point in the
past 10 years and bring them into the QMP world. Sadly, thus far it
hasn't happened as people always had more important things to work
on. Enterprise apps were much more interested in external snapshots
than internal snapshots as they have many more features.
Meanwhile users still want to use internal snapshots as there is
a certainly simplicity in having everything self-contained in one
image, even though it has limitations. Thus the apps that end up
executing the savevm/loadvm/delvm via the "human-monitor-command"
QMP command.
IOW, the problematic blocking behaviour that was one of the reasons
for not having savevm/loadvm/delvm in QMP is experienced by applications
regardless. By not portting the commands to QMP due to one design flaw,
we've forced apps and users to suffer from other design flaws of HMP (
bad error reporting, strong type checking of args, no introspection) for
an additional 10 years. This feels rather sub-optimal :-(
In practice users don't appear to care strongly about the fact that these
commands block the VM while they run. I might have seen one bug report
about it, but it certainly isn't something that comes up as a frequent
topic except among us QEMU maintainers. Users do care about having
access to the snapshot feature.
Where I am seeing frequent complaints is wrt the use of OVMF combined
with snapshots which has some serious pain points. This is getting worse
as the push to ditch legacy BIOS in favour of UEFI gain momentum both
across OS vendors and mgmt apps. Solving it requires new parameters to
the commands, but doing this in HMP is super unappealing.
After 10 years, I think it is time for us to be a little pragmatic about
our handling of snapshots commands. My desire is that libvirt should never
use "human-monitor-command" under any circumstances, because of the
inherant flaws in HMP as a protocol for machine consumption.
Thus in this series I'm proposing a fairly direct mapping of the existing
HMP commands for savevm/loadvm/delvm into QMP as a first step. This does
not solve the blocking thread problem, but it does put in a place a
design using the jobs framework which can facilitate solving it later.
It does also solve the error reporting, type checking and introspection
problems inherant to HMP. So we're winning on 3 out of the 4 problems,
and pushed apps to a QMP design that will let us solve the last
remaining problem.
With a QMP variant, we reasonably deal with the problems related to OVMF:
- The logic to pick which disk to store the vmstate in is not
satsifactory.
The first block driver state cannot be assumed to be the root disk
image, it might be OVMF varstore and we don't want to store vmstate
in there.
- The logic to decide which disks must be snapshotted is hardwired
to all disks which are writable
Again with OVMF there might be a writable varstore, but this can be
raw rather than qcow2 format, and thus unable to be snapshotted.
While users might wish to snapshot their varstore, in some/many/most
cases it is entirely uneccessary. Users are blocked from snapshotting
their VM though due to this varstore.
These are solved by adding two parameters to the commands. The first is
a block device node name that identifies the image to store vmstate in,
and the second is a list of node names to include for the snapshots.
If the list of nodes isn't given, it falls back to the historical
behaviour of using all disks matching some undocumented criteria.
In the block code I've only dealt with node names for block devices, as
IIUC, this is all that libvirt should need in the -blockdev world it now
lives in. IOW, I've made not attempt to cope with people wanting to use
these QMP commands in combination with -drive args, as libvirt will
never use -drive with a QEMU new enough to have these new commands.
The main limitations of this current impl
- The snapshot process runs serialized in the main thread. ie QEMU
guest execution is blocked for the duration. The job framework
lets us fix this in future without changing the QMP semantics
exposed to the apps.
- Most vmstate loading errors just go to stderr, as they are not
using Error **errp reporting. Thus the job framework just
reports a fairly generic message
"Error -22 while loading VM state"
Again this can be fixed later without changing the QMP semantics
exposed to apps.
I've done some minimal work in libvirt to start to make use of the new
commands to validate their functionality, but this isn't finished yet.
My ultimate goal is to make the GNOME Boxes maintainer happy again by
having internal snapshots work with OVMF:
https://gitlab.gnome.org/GNOME/gnome-boxes/-/commit/c486da262f6566326fbcb5e=
f45c5f64048f16a6e
Changed in v3:
- Schedule a bottom half to escape from coroutine context in
the jobs. This is needed because the locking in the snapshot
code goes horribly wrong when run from a background coroutine
instead of the main event thread.
- Re-factor way we iterate over devices, so that we correctly
report non-existant devices passed by the user over QMP.
- Add QAPI docs notes about limitations wrt vmstate error
reporting (it all goes to stderr not an Error **errp)
so QMP only gets a fairly generic error message currently.
- Add I/O test to validate many usage scenarios / errors
- Add I/O test helpers to handle QMP events with a deterministic
ordering
- Ensure 'delete-snapshot' reports an error if requesting
delete from devices that don't support snapshot, instead of
silently succeeding with no erro.
Changed in v2:
- Use new command names "snapshot-{load,save,delete}" to make it
clear that these are different from the "savevm|loadvm|delvm"
as they use the Job framework
- Use an include list for block devs, not an exclude list
Daniel P. Berrang=C3=A9 (7):
migration: improve error reporting of block driver state name
block: push error reporting into bdrv_all_*_snapshot functions
migration: stop returning errno from load_snapshot()
block: add ability to specify list of blockdevs during snapshot
block: allow specifying name of block device for vmstate storage
iotests: add support for capturing and matching QMP events
migration: introduce snapshot-{save,load,delete} QMP commands
block/monitor/block-hmp-cmds.c | 7 +-
block/snapshot.c | 233 ++++++++++++++-------
include/block/snapshot.h | 19 +-
include/migration/snapshot.h | 10 +-
migration/savevm.c | 258 +++++++++++++++++++----
monitor/hmp-cmds.c | 11 +-
qapi/job.json | 9 +-
qapi/migration.json | 135 ++++++++++++
replay/replay-snapshot.c | 4 +-
softmmu/vl.c | 2 +-
tests/qemu-iotests/267.out | 14 +-
tests/qemu-iotests/310 | 255 +++++++++++++++++++++++
tests/qemu-iotests/310.out | 369 +++++++++++++++++++++++++++++++++
tests/qemu-iotests/common.qemu | 107 +++++++++-
tests/qemu-iotests/group | 1 +
15 files changed, 1289 insertions(+), 145 deletions(-)
create mode 100755 tests/qemu-iotests/310
create mode 100644 tests/qemu-iotests/310.out
--=20
2.26.2
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 1/7] migration: improve error reporting of block driver state name
2020-08-27 11:15 [PATCH v3 0/7] migration: bring improved savevm/loadvm/delvm to QMP Daniel P. Berrangé
@ 2020-08-27 11:16 ` Daniel P. Berrangé
2020-08-27 17:17 ` Dr. David Alan Gilbert
2020-08-27 11:16 ` [PATCH v3 2/7] block: push error reporting into bdrv_all_*_snapshot functions Daniel P. Berrangé
` (6 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Daniel P. Berrangé @ 2020-08-27 11:16 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Peter Krempa, Daniel P. Berrangé,
Denis V. Lunev, qemu-block, Juan Quintela, John Snow,
Markus Armbruster, Dr. David Alan Gilbert, Pavel Dovgalyuk,
Paolo Bonzini, Max Reitz
With blockdev, a BlockDriverState may not have a device name,
so using a node name is required as an alternative.
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
migration/savevm.c | 12 ++++++------
tests/qemu-iotests/267.out | 4 ++--
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index a843d202b5..304d98ff78 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2682,7 +2682,7 @@ int save_snapshot(const char *name, Error **errp)
if (!bdrv_all_can_snapshot(&bs)) {
error_setg(errp, "Device '%s' is writable but does not support "
- "snapshots", bdrv_get_device_name(bs));
+ "snapshots", bdrv_get_device_or_node_name(bs));
return ret;
}
@@ -2691,7 +2691,7 @@ int save_snapshot(const char *name, Error **errp)
ret = bdrv_all_delete_snapshot(name, &bs1, errp);
if (ret < 0) {
error_prepend(errp, "Error while deleting snapshot on device "
- "'%s': ", bdrv_get_device_name(bs1));
+ "'%s': ", bdrv_get_device_or_node_name(bs1));
return ret;
}
}
@@ -2766,7 +2766,7 @@ int save_snapshot(const char *name, Error **errp)
ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs);
if (ret < 0) {
error_setg(errp, "Error while creating snapshot on '%s'",
- bdrv_get_device_name(bs));
+ bdrv_get_device_or_node_name(bs));
goto the_end;
}
@@ -2884,14 +2884,14 @@ int load_snapshot(const char *name, Error **errp)
if (!bdrv_all_can_snapshot(&bs)) {
error_setg(errp,
"Device '%s' is writable but does not support snapshots",
- bdrv_get_device_name(bs));
+ bdrv_get_device_or_node_name(bs));
return -ENOTSUP;
}
ret = bdrv_all_find_snapshot(name, &bs);
if (ret < 0) {
error_setg(errp,
"Device '%s' does not have the requested snapshot '%s'",
- bdrv_get_device_name(bs), name);
+ bdrv_get_device_or_node_name(bs), name);
return ret;
}
@@ -2920,7 +2920,7 @@ int load_snapshot(const char *name, Error **errp)
ret = bdrv_all_goto_snapshot(name, &bs, errp);
if (ret < 0) {
error_prepend(errp, "Could not load snapshot '%s' on '%s': ",
- name, bdrv_get_device_name(bs));
+ name, bdrv_get_device_or_node_name(bs));
goto err_drain;
}
diff --git a/tests/qemu-iotests/267.out b/tests/qemu-iotests/267.out
index d6d80c099f..215902b3ad 100644
--- a/tests/qemu-iotests/267.out
+++ b/tests/qemu-iotests/267.out
@@ -81,11 +81,11 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm snap0
-Error: Device '' is writable but does not support snapshots
+Error: Device 'file' is writable but does not support snapshots
(qemu) info snapshots
No available block device supports snapshots
(qemu) loadvm snap0
-Error: Device '' is writable but does not support snapshots
+Error: Device 'file' is writable but does not support snapshots
(qemu) quit
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
--
2.26.2
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v3 2/7] block: push error reporting into bdrv_all_*_snapshot functions
2020-08-27 11:15 [PATCH v3 0/7] migration: bring improved savevm/loadvm/delvm to QMP Daniel P. Berrangé
2020-08-27 11:16 ` [PATCH v3 1/7] migration: improve error reporting of block driver state name Daniel P. Berrangé
@ 2020-08-27 11:16 ` Daniel P. Berrangé
2020-08-27 11:16 ` [PATCH v3 3/7] migration: stop returning errno from load_snapshot() Daniel P. Berrangé
` (5 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Daniel P. Berrangé @ 2020-08-27 11:16 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Peter Krempa, Daniel P. Berrangé,
Denis V. Lunev, qemu-block, Juan Quintela, John Snow,
Markus Armbruster, Dr. David Alan Gilbert, Pavel Dovgalyuk,
Paolo Bonzini, Max Reitz
The bdrv_all_*_snapshot functions return a BlockDriverState pointer
for the invalid backend, which the callers then use to report an
error message. In some cases multiple callers are reporting the
same error message, but with slightly different text. In the future
there will be more error scenarios for some of these methods, which
will benefit from fine grained error message reporting. So it is
helpful to push error reporting down a level.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
block/monitor/block-hmp-cmds.c | 7 ++--
block/snapshot.c | 77 +++++++++++++++++-----------------
include/block/snapshot.h | 14 +++----
migration/savevm.c | 37 +++++-----------
monitor/hmp-cmds.c | 7 +---
tests/qemu-iotests/267.out | 10 ++---
6 files changed, 65 insertions(+), 87 deletions(-)
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 4c8c375172..9df11494d6 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -898,10 +898,11 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
ImageEntry *image_entry, *next_ie;
SnapshotEntry *snapshot_entry;
+ Error *err = NULL;
- bs = bdrv_all_find_vmstate_bs();
+ bs = bdrv_all_find_vmstate_bs(&err);
if (!bs) {
- monitor_printf(mon, "No available block device supports snapshots\n");
+ error_report_err(err);
return;
}
aio_context = bdrv_get_aio_context(bs);
@@ -951,7 +952,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
total = 0;
for (i = 0; i < nb_sns; i++) {
SnapshotEntry *next_sn;
- if (bdrv_all_find_snapshot(sn_tab[i].name, &bs1) == 0) {
+ if (bdrv_all_find_snapshot(sn_tab[i].name, NULL) == 0) {
global_snapshots[total] = i;
total++;
QTAILQ_FOREACH(image_entry, &image_list, next) {
diff --git a/block/snapshot.c b/block/snapshot.c
index bd9fb01817..6839060622 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -400,14 +400,14 @@ static bool bdrv_all_snapshots_includes_bs(BlockDriverState *bs)
* These functions will properly handle dataplane (take aio_context_acquire
* when appropriate for appropriate block drivers) */
-bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
+bool bdrv_all_can_snapshot(Error **errp)
{
- bool ok = true;
BlockDriverState *bs;
BdrvNextIterator it;
for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
AioContext *ctx = bdrv_get_aio_context(bs);
+ bool ok;
aio_context_acquire(ctx);
if (bdrv_all_snapshots_includes_bs(bs)) {
@@ -415,26 +415,25 @@ bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs)
}
aio_context_release(ctx);
if (!ok) {
+ error_setg(errp, "Device '%s' is writable but does not support "
+ "snapshots", bdrv_get_device_or_node_name(bs));
bdrv_next_cleanup(&it);
- goto fail;
+ return false;
}
}
-fail:
- *first_bad_bs = bs;
- return ok;
+ return true;
}
-int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
- Error **errp)
+int bdrv_all_delete_snapshot(const char *name, Error **errp)
{
- int ret = 0;
BlockDriverState *bs;
BdrvNextIterator it;
QEMUSnapshotInfo sn1, *snapshot = &sn1;
for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
AioContext *ctx = bdrv_get_aio_context(bs);
+ int ret;
aio_context_acquire(ctx);
if (bdrv_all_snapshots_includes_bs(bs) &&
@@ -445,26 +444,25 @@ int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
}
aio_context_release(ctx);
if (ret < 0) {
+ error_prepend(errp, "Could not delete snapshot '%s' on '%s': ",
+ name, bdrv_get_device_or_node_name(bs));
bdrv_next_cleanup(&it);
- goto fail;
+ return -1;
}
}
-fail:
- *first_bad_bs = bs;
- return ret;
+ return 0;
}
-int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
- Error **errp)
+int bdrv_all_goto_snapshot(const char *name, Error **errp)
{
- int ret = 0;
BlockDriverState *bs;
BdrvNextIterator it;
for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
AioContext *ctx = bdrv_get_aio_context(bs);
+ int ret;
aio_context_acquire(ctx);
if (bdrv_all_snapshots_includes_bs(bs)) {
@@ -472,75 +470,75 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
}
aio_context_release(ctx);
if (ret < 0) {
+ error_prepend(errp, "Could not load snapshot '%s' on '%s': ",
+ name, bdrv_get_device_or_node_name(bs));
bdrv_next_cleanup(&it);
- goto fail;
+ return -1;
}
}
-fail:
- *first_bad_bs = bs;
- return ret;
+ return 0;
}
-int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
+int bdrv_all_find_snapshot(const char *name, Error **errp)
{
QEMUSnapshotInfo sn;
- int err = 0;
BlockDriverState *bs;
BdrvNextIterator it;
for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
AioContext *ctx = bdrv_get_aio_context(bs);
+ int ret;
aio_context_acquire(ctx);
if (bdrv_all_snapshots_includes_bs(bs)) {
- err = bdrv_snapshot_find(bs, &sn, name);
+ ret = bdrv_snapshot_find(bs, &sn, name);
}
aio_context_release(ctx);
- if (err < 0) {
+ if (ret < 0) {
+ error_setg(errp, "Could not find snapshot '%s' on '%s'",
+ name, bdrv_get_device_or_node_name(bs));
bdrv_next_cleanup(&it);
- goto fail;
+ return -1;
}
}
-fail:
- *first_bad_bs = bs;
- return err;
+ return 0;
}
int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
BlockDriverState *vm_state_bs,
uint64_t vm_state_size,
- BlockDriverState **first_bad_bs)
+ Error **errp)
{
- int err = 0;
BlockDriverState *bs;
BdrvNextIterator it;
for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
AioContext *ctx = bdrv_get_aio_context(bs);
+ int ret;
aio_context_acquire(ctx);
if (bs == vm_state_bs) {
sn->vm_state_size = vm_state_size;
- err = bdrv_snapshot_create(bs, sn);
+ ret = bdrv_snapshot_create(bs, sn);
} else if (bdrv_all_snapshots_includes_bs(bs)) {
sn->vm_state_size = 0;
- err = bdrv_snapshot_create(bs, sn);
+ ret = bdrv_snapshot_create(bs, sn);
}
aio_context_release(ctx);
- if (err < 0) {
+ if (ret < 0) {
+ error_setg(errp, "Could not create snapshot '%s' on '%s'",
+ sn->name, bdrv_get_device_or_node_name(bs));
bdrv_next_cleanup(&it);
- goto fail;
+ return -1;
}
}
-fail:
- *first_bad_bs = bs;
- return err;
+ return 0;
}
-BlockDriverState *bdrv_all_find_vmstate_bs(void)
+BlockDriverState *bdrv_all_find_vmstate_bs(Error **errp)
{
BlockDriverState *bs;
BdrvNextIterator it;
@@ -558,5 +556,8 @@ BlockDriverState *bdrv_all_find_vmstate_bs(void)
break;
}
}
+ if (!bs) {
+ error_setg(errp, "No block device supports snapshots");
+ }
return bs;
}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 2bfcd57578..ba1528eee0 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -76,17 +76,15 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
* These functions will properly handle dataplane (take aio_context_acquire
* when appropriate for appropriate block drivers */
-bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
-int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
- Error **errp);
-int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
- Error **errp);
-int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs);
+bool bdrv_all_can_snapshot(Error **errp);
+int bdrv_all_delete_snapshot(const char *name, Error **errp);
+int bdrv_all_goto_snapshot(const char *name, Error **errp);
+int bdrv_all_find_snapshot(const char *name, Error **errp);
int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
BlockDriverState *vm_state_bs,
uint64_t vm_state_size,
- BlockDriverState **first_bad_bs);
+ Error **errp);
-BlockDriverState *bdrv_all_find_vmstate_bs(void);
+BlockDriverState *bdrv_all_find_vmstate_bs(Error **errp);
#endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 304d98ff78..3826c437cc 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2660,7 +2660,7 @@ int qemu_load_device_state(QEMUFile *f)
int save_snapshot(const char *name, Error **errp)
{
- BlockDriverState *bs, *bs1;
+ BlockDriverState *bs;
QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
int ret = -1, ret2;
QEMUFile *f;
@@ -2680,25 +2680,19 @@ int save_snapshot(const char *name, Error **errp)
return ret;
}
- if (!bdrv_all_can_snapshot(&bs)) {
- error_setg(errp, "Device '%s' is writable but does not support "
- "snapshots", bdrv_get_device_or_node_name(bs));
+ if (!bdrv_all_can_snapshot(errp)) {
return ret;
}
/* Delete old snapshots of the same name */
if (name) {
- ret = bdrv_all_delete_snapshot(name, &bs1, errp);
- if (ret < 0) {
- error_prepend(errp, "Error while deleting snapshot on device "
- "'%s': ", bdrv_get_device_or_node_name(bs1));
+ if (bdrv_all_delete_snapshot(name, errp) < 0) {
return ret;
}
}
- bs = bdrv_all_find_vmstate_bs();
+ bs = bdrv_all_find_vmstate_bs(errp);
if (bs == NULL) {
- error_setg(errp, "No block device can accept snapshots");
return ret;
}
aio_context = bdrv_get_aio_context(bs);
@@ -2763,10 +2757,8 @@ int save_snapshot(const char *name, Error **errp)
aio_context_release(aio_context);
aio_context = NULL;
- ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs);
+ ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, errp);
if (ret < 0) {
- error_setg(errp, "Error while creating snapshot on '%s'",
- bdrv_get_device_or_node_name(bs));
goto the_end;
}
@@ -2868,7 +2860,7 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
int load_snapshot(const char *name, Error **errp)
{
- BlockDriverState *bs, *bs_vm_state;
+ BlockDriverState *bs_vm_state;
QEMUSnapshotInfo sn;
QEMUFile *f;
int ret;
@@ -2881,23 +2873,16 @@ int load_snapshot(const char *name, Error **errp)
return -EINVAL;
}
- if (!bdrv_all_can_snapshot(&bs)) {
- error_setg(errp,
- "Device '%s' is writable but does not support snapshots",
- bdrv_get_device_or_node_name(bs));
+ if (!bdrv_all_can_snapshot(errp)) {
return -ENOTSUP;
}
- ret = bdrv_all_find_snapshot(name, &bs);
+ ret = bdrv_all_find_snapshot(name, errp);
if (ret < 0) {
- error_setg(errp,
- "Device '%s' does not have the requested snapshot '%s'",
- bdrv_get_device_or_node_name(bs), name);
return ret;
}
- bs_vm_state = bdrv_all_find_vmstate_bs();
+ bs_vm_state = bdrv_all_find_vmstate_bs(errp);
if (!bs_vm_state) {
- error_setg(errp, "No block device supports snapshots");
return -ENOTSUP;
}
aio_context = bdrv_get_aio_context(bs_vm_state);
@@ -2917,10 +2902,8 @@ int load_snapshot(const char *name, Error **errp)
/* Flush all IO requests so they don't interfere with the new state. */
bdrv_drain_all_begin();
- ret = bdrv_all_goto_snapshot(name, &bs, errp);
+ ret = bdrv_all_goto_snapshot(name, errp);
if (ret < 0) {
- error_prepend(errp, "Could not load snapshot '%s' on '%s': ",
- name, bdrv_get_device_or_node_name(bs));
goto err_drain;
}
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 7711726fd2..9bb50b9abf 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1137,15 +1137,10 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
void hmp_delvm(Monitor *mon, const QDict *qdict)
{
- BlockDriverState *bs;
Error *err = NULL;
const char *name = qdict_get_str(qdict, "name");
- if (bdrv_all_delete_snapshot(name, &bs, &err) < 0) {
- error_prepend(&err,
- "deleting snapshot on device '%s': ",
- bdrv_get_device_name(bs));
- }
+ bdrv_all_delete_snapshot(name, &err);
hmp_handle_error(mon, err);
}
diff --git a/tests/qemu-iotests/267.out b/tests/qemu-iotests/267.out
index 215902b3ad..c65cce893a 100644
--- a/tests/qemu-iotests/267.out
+++ b/tests/qemu-iotests/267.out
@@ -6,9 +6,9 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
Testing:
QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm snap0
-Error: No block device can accept snapshots
+Error: No block device supports snapshots
(qemu) info snapshots
-No available block device supports snapshots
+No block device supports snapshots
(qemu) loadvm snap0
Error: No block device supports snapshots
(qemu) quit
@@ -22,7 +22,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm snap0
Error: Device 'none0' is writable but does not support snapshots
(qemu) info snapshots
-No available block device supports snapshots
+No block device supports snapshots
(qemu) loadvm snap0
Error: Device 'none0' is writable but does not support snapshots
(qemu) quit
@@ -58,7 +58,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm snap0
Error: Device 'virtio0' is writable but does not support snapshots
(qemu) info snapshots
-No available block device supports snapshots
+No block device supports snapshots
(qemu) loadvm snap0
Error: Device 'virtio0' is writable but does not support snapshots
(qemu) quit
@@ -83,7 +83,7 @@ QEMU X.Y.Z monitor - type 'help' for more information
(qemu) savevm snap0
Error: Device 'file' is writable but does not support snapshots
(qemu) info snapshots
-No available block device supports snapshots
+No block device supports snapshots
(qemu) loadvm snap0
Error: Device 'file' is writable but does not support snapshots
(qemu) quit
--
2.26.2
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v3 3/7] migration: stop returning errno from load_snapshot()
2020-08-27 11:15 [PATCH v3 0/7] migration: bring improved savevm/loadvm/delvm to QMP Daniel P. Berrangé
2020-08-27 11:16 ` [PATCH v3 1/7] migration: improve error reporting of block driver state name Daniel P. Berrangé
2020-08-27 11:16 ` [PATCH v3 2/7] block: push error reporting into bdrv_all_*_snapshot functions Daniel P. Berrangé
@ 2020-08-27 11:16 ` Daniel P. Berrangé
2020-08-27 11:16 ` [PATCH v3 4/7] block: add ability to specify list of blockdevs during snapshot Daniel P. Berrangé
` (4 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Daniel P. Berrangé @ 2020-08-27 11:16 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Peter Krempa, Daniel P. Berrangé,
Denis V. Lunev, qemu-block, Juan Quintela, John Snow,
Markus Armbruster, Dr. David Alan Gilbert, Pavel Dovgalyuk,
Paolo Bonzini, Max Reitz
None of the callers care about the errno value since there is a full
Error object populated. This gives consistency with save_snapshot()
which already just returns -1.
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
migration/savevm.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/migration/savevm.c b/migration/savevm.c
index 3826c437cc..711137bcbe 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2870,20 +2870,20 @@ int load_snapshot(const char *name, Error **errp)
if (!replay_can_snapshot()) {
error_setg(errp, "Record/replay does not allow loading snapshot "
"right now. Try once more later.");
- return -EINVAL;
+ return -1;
}
if (!bdrv_all_can_snapshot(errp)) {
- return -ENOTSUP;
+ return -1;
}
ret = bdrv_all_find_snapshot(name, errp);
if (ret < 0) {
- return ret;
+ return -1;
}
bs_vm_state = bdrv_all_find_vmstate_bs(errp);
if (!bs_vm_state) {
- return -ENOTSUP;
+ return -1;
}
aio_context = bdrv_get_aio_context(bs_vm_state);
@@ -2892,11 +2892,11 @@ int load_snapshot(const char *name, Error **errp)
ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
aio_context_release(aio_context);
if (ret < 0) {
- return ret;
+ return -1;
} else if (sn.vm_state_size == 0) {
error_setg(errp, "This is a disk-only snapshot. Revert to it "
" offline using qemu-img");
- return -EINVAL;
+ return -1;
}
/* Flush all IO requests so they don't interfere with the new state. */
@@ -2911,7 +2911,6 @@ int load_snapshot(const char *name, Error **errp)
f = qemu_fopen_bdrv(bs_vm_state, 0);
if (!f) {
error_setg(errp, "Could not open VM state file");
- ret = -EINVAL;
goto err_drain;
}
@@ -2927,14 +2926,14 @@ int load_snapshot(const char *name, Error **errp)
if (ret < 0) {
error_setg(errp, "Error %d while loading VM state", ret);
- return ret;
+ return -1;
}
return 0;
err_drain:
bdrv_drain_all_end();
- return ret;
+ return -1;
}
void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev)
--
2.26.2
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v3 4/7] block: add ability to specify list of blockdevs during snapshot
2020-08-27 11:15 [PATCH v3 0/7] migration: bring improved savevm/loadvm/delvm to QMP Daniel P. Berrangé
` (2 preceding siblings ...)
2020-08-27 11:16 ` [PATCH v3 3/7] migration: stop returning errno from load_snapshot() Daniel P. Berrangé
@ 2020-08-27 11:16 ` Daniel P. Berrangé
2020-08-27 11:16 ` [PATCH v3 5/7] block: allow specifying name of block device for vmstate storage Daniel P. Berrangé
` (3 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Daniel P. Berrangé @ 2020-08-27 11:16 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Peter Krempa, Daniel P. Berrangé,
Denis V. Lunev, qemu-block, Juan Quintela, John Snow,
Markus Armbruster, Dr. David Alan Gilbert, Pavel Dovgalyuk,
Paolo Bonzini, Max Reitz
When running snapshot operations, there are various rules for which
blockdevs are included/excluded. While this provides reasonable default
behaviour, there are scenarios that are not well handled by the default
logic. Some of the conditions do not have a single correct answer.
Thus there needs to be a way for the mgmt app to provide an explicit
list of blockdevs to perform snapshots across. This can be achieved
by passing a list of node names that should be used.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
block/monitor/block-hmp-cmds.c | 4 +-
block/snapshot.c | 167 +++++++++++++++++++++++----------
include/block/snapshot.h | 13 +--
migration/savevm.c | 16 ++--
monitor/hmp-cmds.c | 2 +-
5 files changed, 138 insertions(+), 64 deletions(-)
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 9df11494d6..db76c43cc2 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -900,7 +900,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
SnapshotEntry *snapshot_entry;
Error *err = NULL;
- bs = bdrv_all_find_vmstate_bs(&err);
+ bs = bdrv_all_find_vmstate_bs(NULL, &err);
if (!bs) {
error_report_err(err);
return;
@@ -952,7 +952,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
total = 0;
for (i = 0; i < nb_sns; i++) {
SnapshotEntry *next_sn;
- if (bdrv_all_find_snapshot(sn_tab[i].name, NULL) == 0) {
+ if (bdrv_all_find_snapshot(sn_tab[i].name, NULL, NULL) == 0) {
global_snapshots[total] = i;
total++;
QTAILQ_FOREACH(image_entry, &image_list, next) {
diff --git a/block/snapshot.c b/block/snapshot.c
index 6839060622..5691cdc6cb 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -385,6 +385,36 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
return ret;
}
+
+static int bdrv_all_get_snapshot_devices(strList *devices,
+ GList **all_bdrvs,
+ Error **errp)
+{
+ g_autoptr(GList) bdrvs = NULL;
+
+ if (devices) {
+ while (devices) {
+ BlockDriverState *bs = bdrv_find_node(devices->value);
+ if (!bs) {
+ error_setg(errp, "No block device node '%s'", devices->value);
+ return -1;
+ }
+ bdrvs = g_list_append(bdrvs, bs);
+ devices = devices->next;
+ }
+ } else {
+ BlockDriverState *bs;
+ BdrvNextIterator it;
+ for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
+ bdrvs = g_list_append(bdrvs, bs);
+ }
+ }
+
+ *all_bdrvs = g_steal_pointer(&bdrvs);
+ return 0;
+}
+
+
static bool bdrv_all_snapshots_includes_bs(BlockDriverState *bs)
{
if (!bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
@@ -400,43 +430,56 @@ static bool bdrv_all_snapshots_includes_bs(BlockDriverState *bs)
* These functions will properly handle dataplane (take aio_context_acquire
* when appropriate for appropriate block drivers) */
-bool bdrv_all_can_snapshot(Error **errp)
+bool bdrv_all_can_snapshot(strList *devices, Error **errp)
{
- BlockDriverState *bs;
- BdrvNextIterator it;
+ g_autoptr(GList) bdrvs = NULL;
+ GList *iterbdrvs;
- for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
+ if (bdrv_all_get_snapshot_devices(devices, &bdrvs, errp) < 0) {
+ return false;
+ }
+
+ iterbdrvs = bdrvs;
+ while (iterbdrvs) {
+ BlockDriverState *bs = iterbdrvs->data;
AioContext *ctx = bdrv_get_aio_context(bs);
bool ok;
aio_context_acquire(ctx);
- if (bdrv_all_snapshots_includes_bs(bs)) {
+ if (devices || bdrv_all_snapshots_includes_bs(bs)) {
ok = bdrv_can_snapshot(bs);
}
aio_context_release(ctx);
if (!ok) {
error_setg(errp, "Device '%s' is writable but does not support "
"snapshots", bdrv_get_device_or_node_name(bs));
- bdrv_next_cleanup(&it);
return false;
}
+
+ iterbdrvs = iterbdrvs->next;
}
return true;
}
-int bdrv_all_delete_snapshot(const char *name, Error **errp)
+int bdrv_all_delete_snapshot(const char *name, strList *devices, Error **errp)
{
- BlockDriverState *bs;
- BdrvNextIterator it;
- QEMUSnapshotInfo sn1, *snapshot = &sn1;
+ g_autoptr(GList) bdrvs = NULL;
+ GList *iterbdrvs;
- for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
+ if (bdrv_all_get_snapshot_devices(devices, &bdrvs, errp) < 0) {
+ return -1;
+ }
+
+ iterbdrvs = bdrvs;
+ while (iterbdrvs) {
+ BlockDriverState *bs = iterbdrvs->data;
AioContext *ctx = bdrv_get_aio_context(bs);
- int ret;
+ QEMUSnapshotInfo sn1, *snapshot = &sn1;
+ int ret = 0;
aio_context_acquire(ctx);
- if (bdrv_all_snapshots_includes_bs(bs) &&
+ if ((devices || bdrv_all_snapshots_includes_bs(bs)) &&
bdrv_snapshot_find(bs, snapshot, name) >= 0)
{
ret = bdrv_snapshot_delete(bs, snapshot->id_str,
@@ -446,61 +489,76 @@ int bdrv_all_delete_snapshot(const char *name, Error **errp)
if (ret < 0) {
error_prepend(errp, "Could not delete snapshot '%s' on '%s': ",
name, bdrv_get_device_or_node_name(bs));
- bdrv_next_cleanup(&it);
return -1;
}
+
+ iterbdrvs = iterbdrvs->next;
}
return 0;
}
-int bdrv_all_goto_snapshot(const char *name, Error **errp)
+int bdrv_all_goto_snapshot(const char *name, strList *devices, Error **errp)
{
- BlockDriverState *bs;
- BdrvNextIterator it;
+ g_autoptr(GList) bdrvs = NULL;
+ GList *iterbdrvs;
- for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
+ if (bdrv_all_get_snapshot_devices(devices, &bdrvs, errp) < 0) {
+ return -1;
+ }
+
+ iterbdrvs = bdrvs;
+ while (iterbdrvs) {
+ BlockDriverState *bs = iterbdrvs->data;
AioContext *ctx = bdrv_get_aio_context(bs);
- int ret;
+ int ret = 0;
aio_context_acquire(ctx);
- if (bdrv_all_snapshots_includes_bs(bs)) {
+ if (devices || bdrv_all_snapshots_includes_bs(bs)) {
ret = bdrv_snapshot_goto(bs, name, errp);
}
aio_context_release(ctx);
if (ret < 0) {
error_prepend(errp, "Could not load snapshot '%s' on '%s': ",
name, bdrv_get_device_or_node_name(bs));
- bdrv_next_cleanup(&it);
return -1;
}
+
+ iterbdrvs = iterbdrvs->next;
}
return 0;
}
-int bdrv_all_find_snapshot(const char *name, Error **errp)
+int bdrv_all_find_snapshot(const char *name, strList *devices, Error **errp)
{
- QEMUSnapshotInfo sn;
- BlockDriverState *bs;
- BdrvNextIterator it;
+ g_autoptr(GList) bdrvs = NULL;
+ GList *iterbdrvs;
- for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
+ if (bdrv_all_get_snapshot_devices(devices, &bdrvs, errp) < 0) {
+ return -1;
+ }
+
+ iterbdrvs = bdrvs;
+ while (iterbdrvs) {
+ BlockDriverState *bs = iterbdrvs->data;
AioContext *ctx = bdrv_get_aio_context(bs);
- int ret;
+ QEMUSnapshotInfo sn;
+ int ret = 0;
aio_context_acquire(ctx);
- if (bdrv_all_snapshots_includes_bs(bs)) {
+ if (devices || bdrv_all_snapshots_includes_bs(bs)) {
ret = bdrv_snapshot_find(bs, &sn, name);
}
aio_context_release(ctx);
if (ret < 0) {
error_setg(errp, "Could not find snapshot '%s' on '%s'",
name, bdrv_get_device_or_node_name(bs));
- bdrv_next_cleanup(&it);
return -1;
}
+
+ iterbdrvs = iterbdrvs->next;
}
return 0;
@@ -509,20 +567,27 @@ int bdrv_all_find_snapshot(const char *name, Error **errp)
int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
BlockDriverState *vm_state_bs,
uint64_t vm_state_size,
+ strList *devices,
Error **errp)
{
- BlockDriverState *bs;
- BdrvNextIterator it;
+ g_autoptr(GList) bdrvs = NULL;
+ GList *iterbdrvs;
- for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
+ if (bdrv_all_get_snapshot_devices(devices, &bdrvs, errp) < 0) {
+ return -1;
+ }
+
+ iterbdrvs = bdrvs;
+ while (iterbdrvs) {
+ BlockDriverState *bs = iterbdrvs->data;
AioContext *ctx = bdrv_get_aio_context(bs);
- int ret;
+ int ret = 0;
aio_context_acquire(ctx);
if (bs == vm_state_bs) {
sn->vm_state_size = vm_state_size;
ret = bdrv_snapshot_create(bs, sn);
- } else if (bdrv_all_snapshots_includes_bs(bs)) {
+ } else if (devices || bdrv_all_snapshots_includes_bs(bs)) {
sn->vm_state_size = 0;
ret = bdrv_snapshot_create(bs, sn);
}
@@ -530,34 +595,42 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
if (ret < 0) {
error_setg(errp, "Could not create snapshot '%s' on '%s'",
sn->name, bdrv_get_device_or_node_name(bs));
- bdrv_next_cleanup(&it);
return -1;
}
+
+ iterbdrvs = iterbdrvs->next;
}
return 0;
}
-BlockDriverState *bdrv_all_find_vmstate_bs(Error **errp)
+BlockDriverState *bdrv_all_find_vmstate_bs(strList *devices, Error **errp)
{
- BlockDriverState *bs;
- BdrvNextIterator it;
+ g_autoptr(GList) bdrvs = NULL;
+ GList *iterbdrvs;
+
+ if (bdrv_all_get_snapshot_devices(devices, &bdrvs, errp) < 0) {
+ return NULL;
+ }
- for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
+ iterbdrvs = bdrvs;
+ while (iterbdrvs) {
+ BlockDriverState *bs = iterbdrvs->data;
AioContext *ctx = bdrv_get_aio_context(bs);
- bool found;
+ bool found = false;
aio_context_acquire(ctx);
- found = bdrv_all_snapshots_includes_bs(bs) && bdrv_can_snapshot(bs);
+ found = (devices || bdrv_all_snapshots_includes_bs(bs)) &&
+ bdrv_can_snapshot(bs);
aio_context_release(ctx);
if (found) {
- bdrv_next_cleanup(&it);
- break;
+ return bs;
}
+
+ iterbdrvs = iterbdrvs->next;
}
- if (!bs) {
- error_setg(errp, "No block device supports snapshots");
- }
- return bs;
+
+ error_setg(errp, "No block device supports snapshots");
+ return NULL;
}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index ba1528eee0..1c5b0705a9 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -25,7 +25,7 @@
#ifndef SNAPSHOT_H
#define SNAPSHOT_H
-
+#include "qapi/qapi-builtin-types.h"
#define SNAPSHOT_OPT_BASE "snapshot."
#define SNAPSHOT_OPT_ID "snapshot.id"
@@ -76,15 +76,16 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
* These functions will properly handle dataplane (take aio_context_acquire
* when appropriate for appropriate block drivers */
-bool bdrv_all_can_snapshot(Error **errp);
-int bdrv_all_delete_snapshot(const char *name, Error **errp);
-int bdrv_all_goto_snapshot(const char *name, Error **errp);
-int bdrv_all_find_snapshot(const char *name, Error **errp);
+bool bdrv_all_can_snapshot(strList *devices, Error **errp);
+int bdrv_all_delete_snapshot(const char *name, strList *devices, Error **errp);
+int bdrv_all_goto_snapshot(const char *name, strList *devices, Error **errp);
+int bdrv_all_find_snapshot(const char *name, strList *devices, Error **errp);
int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
BlockDriverState *vm_state_bs,
uint64_t vm_state_size,
+ strList *devices,
Error **errp);
-BlockDriverState *bdrv_all_find_vmstate_bs(Error **errp);
+BlockDriverState *bdrv_all_find_vmstate_bs(strList *devices, Error **errp);
#endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 711137bcbe..ae56de1a85 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2680,18 +2680,18 @@ int save_snapshot(const char *name, Error **errp)
return ret;
}
- if (!bdrv_all_can_snapshot(errp)) {
+ if (!bdrv_all_can_snapshot(NULL, errp)) {
return ret;
}
/* Delete old snapshots of the same name */
if (name) {
- if (bdrv_all_delete_snapshot(name, errp) < 0) {
+ if (bdrv_all_delete_snapshot(name, NULL, errp) < 0) {
return ret;
}
}
- bs = bdrv_all_find_vmstate_bs(errp);
+ bs = bdrv_all_find_vmstate_bs(NULL, errp);
if (bs == NULL) {
return ret;
}
@@ -2757,7 +2757,7 @@ int save_snapshot(const char *name, Error **errp)
aio_context_release(aio_context);
aio_context = NULL;
- ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, errp);
+ ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, NULL, errp);
if (ret < 0) {
goto the_end;
}
@@ -2873,15 +2873,15 @@ int load_snapshot(const char *name, Error **errp)
return -1;
}
- if (!bdrv_all_can_snapshot(errp)) {
+ if (!bdrv_all_can_snapshot(NULL, errp)) {
return -1;
}
- ret = bdrv_all_find_snapshot(name, errp);
+ ret = bdrv_all_find_snapshot(name, NULL, errp);
if (ret < 0) {
return -1;
}
- bs_vm_state = bdrv_all_find_vmstate_bs(errp);
+ bs_vm_state = bdrv_all_find_vmstate_bs(NULL, errp);
if (!bs_vm_state) {
return -1;
}
@@ -2902,7 +2902,7 @@ int load_snapshot(const char *name, Error **errp)
/* Flush all IO requests so they don't interfere with the new state. */
bdrv_drain_all_begin();
- ret = bdrv_all_goto_snapshot(name, errp);
+ ret = bdrv_all_goto_snapshot(name, NULL, errp);
if (ret < 0) {
goto err_drain;
}
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 9bb50b9abf..51f624b5ea 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1140,7 +1140,7 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
Error *err = NULL;
const char *name = qdict_get_str(qdict, "name");
- bdrv_all_delete_snapshot(name, &err);
+ bdrv_all_delete_snapshot(name, NULL, &err);
hmp_handle_error(mon, err);
}
--
2.26.2
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v3 5/7] block: allow specifying name of block device for vmstate storage
2020-08-27 11:15 [PATCH v3 0/7] migration: bring improved savevm/loadvm/delvm to QMP Daniel P. Berrangé
` (3 preceding siblings ...)
2020-08-27 11:16 ` [PATCH v3 4/7] block: add ability to specify list of blockdevs during snapshot Daniel P. Berrangé
@ 2020-08-27 11:16 ` Daniel P. Berrangé
2020-08-27 11:16 ` [PATCH v3 6/7] iotests: add support for capturing and matching QMP events Daniel P. Berrangé
` (2 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Daniel P. Berrangé @ 2020-08-27 11:16 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Peter Krempa, Daniel P. Berrangé,
Denis V. Lunev, qemu-block, Juan Quintela, John Snow,
Markus Armbruster, Dr. David Alan Gilbert, Pavel Dovgalyuk,
Paolo Bonzini, Max Reitz
Currently the vmstate will be stored in the first block device that
supports snapshots. Historically this would have usually been the
root device, but with UEFI it might be the variable store. There
needs to be a way to override the choice of block device to store
the state in.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
block/monitor/block-hmp-cmds.c | 2 +-
block/snapshot.c | 17 +++++++++++++++--
include/block/snapshot.h | 4 +++-
migration/savevm.c | 4 ++--
4 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index db76c43cc2..81d1b52262 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -900,7 +900,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
SnapshotEntry *snapshot_entry;
Error *err = NULL;
- bs = bdrv_all_find_vmstate_bs(NULL, &err);
+ bs = bdrv_all_find_vmstate_bs(NULL, NULL, &err);
if (!bs) {
error_report_err(err);
return;
diff --git a/block/snapshot.c b/block/snapshot.c
index 5691cdc6cb..1f7b9a5146 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -604,7 +604,9 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
return 0;
}
-BlockDriverState *bdrv_all_find_vmstate_bs(strList *devices, Error **errp)
+BlockDriverState *bdrv_all_find_vmstate_bs(const char *vmstate_bs,
+ strList *devices,
+ Error **errp)
{
g_autoptr(GList) bdrvs = NULL;
GList *iterbdrvs;
@@ -624,6 +626,13 @@ BlockDriverState *bdrv_all_find_vmstate_bs(strList *devices, Error **errp)
bdrv_can_snapshot(bs);
aio_context_release(ctx);
+ if (vmstate_bs && g_str_equal(vmstate_bs,
+ bdrv_get_node_name(bs))) {
+ error_setg(errp, "block device '%s' does not support snapshots",
+ vmstate_bs);
+ return NULL;
+ }
+
if (found) {
return bs;
}
@@ -631,6 +640,10 @@ BlockDriverState *bdrv_all_find_vmstate_bs(strList *devices, Error **errp)
iterbdrvs = iterbdrvs->next;
}
- error_setg(errp, "No block device supports snapshots");
+ if (vmstate_bs) {
+ error_setg(errp, "Block device '%s' does not exist", vmstate_bs);
+ } else {
+ error_setg(errp, "No block device supports snapshots");
+ }
return NULL;
}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 1c5b0705a9..05550e5da1 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -86,6 +86,8 @@ int bdrv_all_create_snapshot(QEMUSnapshotInfo *sn,
strList *devices,
Error **errp);
-BlockDriverState *bdrv_all_find_vmstate_bs(strList *devices, Error **errp);
+BlockDriverState *bdrv_all_find_vmstate_bs(const char *vmstate_bs,
+ strList *devices,
+ Error **errp);
#endif
diff --git a/migration/savevm.c b/migration/savevm.c
index ae56de1a85..4a52704132 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2691,7 +2691,7 @@ int save_snapshot(const char *name, Error **errp)
}
}
- bs = bdrv_all_find_vmstate_bs(NULL, errp);
+ bs = bdrv_all_find_vmstate_bs(NULL, NULL, errp);
if (bs == NULL) {
return ret;
}
@@ -2881,7 +2881,7 @@ int load_snapshot(const char *name, Error **errp)
return -1;
}
- bs_vm_state = bdrv_all_find_vmstate_bs(NULL, errp);
+ bs_vm_state = bdrv_all_find_vmstate_bs(NULL, NULL, errp);
if (!bs_vm_state) {
return -1;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v3 6/7] iotests: add support for capturing and matching QMP events
2020-08-27 11:15 [PATCH v3 0/7] migration: bring improved savevm/loadvm/delvm to QMP Daniel P. Berrangé
` (4 preceding siblings ...)
2020-08-27 11:16 ` [PATCH v3 5/7] block: allow specifying name of block device for vmstate storage Daniel P. Berrangé
@ 2020-08-27 11:16 ` Daniel P. Berrangé
2020-08-27 11:16 ` [PATCH v3 7/7] migration: introduce snapshot-{save, load, delete} QMP commands Daniel P. Berrangé
2020-09-11 11:52 ` [PATCH v3 0/7] migration: bring improved savevm/loadvm/delvm to QMP Dr. David Alan Gilbert
7 siblings, 0 replies; 19+ messages in thread
From: Daniel P. Berrangé @ 2020-08-27 11:16 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Peter Krempa, Daniel P. Berrangé,
Denis V. Lunev, qemu-block, Juan Quintela, John Snow,
Markus Armbruster, Dr. David Alan Gilbert, Pavel Dovgalyuk,
Paolo Bonzini, Max Reitz
When using the _launch_qemu and _send_qemu_cmd functions from
common.qemu, any QMP events get mixed in with the output from
the commands and responses.
This makes it difficult to write a test case as the ordering
of events in the output is not stable.
This introduces a variable 'capture_events' which can be set
to a list of event names. Any events listed in this variable
will not be printed, instead collected in the $QEMU_EVENTS
environment variable.
A new '_wait_event' function can be invoked to collect events
at a fixed point in time. The function will first pull events
cached in $QEMU_EVENTS variable, and if none are found, will
then read more from QMP.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
tests/qemu-iotests/common.qemu | 107 ++++++++++++++++++++++++++++++++-
1 file changed, 106 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
index de680cf1c7..87d7a54001 100644
--- a/tests/qemu-iotests/common.qemu
+++ b/tests/qemu-iotests/common.qemu
@@ -53,6 +53,15 @@ _in_fd=4
# If $mismatch_only is set, only non-matching responses will
# be echoed.
#
+# If $capture_events is non-empty, then any QMP event names it lists
+# will not be echoed out, but instead collected in the $QEMU_EVENTS
+# variable. The _wait_event function can later be used to received
+# the cached events.
+#
+# If $only_capture_events is set to anything but an empty string,
+# when an error will be raised if a QMP message is seen which is
+# not an event listed in $capture_events.
+#
# If $success_or_failure is set, the meaning of the arguments is
# changed as follows:
# $2: A string to search for in the response; if found, this indicates
@@ -78,6 +87,32 @@ _timed_wait_for()
QEMU_STATUS[$h]=0
while IFS= read -t ${QEMU_COMM_TIMEOUT} resp <&${QEMU_OUT[$h]}
do
+ if [ -n "$capture_events" ]; then
+ capture=0
+ local evname
+ for evname in $capture_events
+ do
+ grep -q "\"event\": \"${evname}\"" < <(echo "${resp}")
+ if [ $? -eq 0 ]; then
+ capture=1
+ fi
+ done
+ if [ $capture = 1 ];
+ then
+ ev=$(echo "${resp}" | tr -d '\r' | tr % .)
+ QEMU_EVENTS="${QEMU_EVENTS:+${QEMU_EVENTS}%}${ev}"
+ if [ -n "$only_capture_events" ]; then
+ return
+ else
+ continue
+ fi
+ fi
+ fi
+ if [ -n "$only_capture_events" ]; then
+ echo "Only expected $capture_events but got ${resp}"
+ exit 1
+ fi
+
if [ -z "${silent}" ] && [ -z "${mismatch_only}" ]; then
echo "${resp}" | _filter_testdir | _filter_qemu \
| _filter_qemu_io | _filter_qmp | _filter_hmp
@@ -177,12 +212,82 @@ _send_qemu_cmd()
let count--;
done
if [ ${QEMU_STATUS[$h]} -ne 0 ] && [ -z "${qemu_error_no_exit}" ]; then
- echo "Timeout waiting for ${1} on handle ${h}"
+ echo "Timeout waiting for command ${1} response on handle ${h}"
exit 1 #Timeout means the test failed
fi
}
+# Check event cache for a named QMP event
+#
+# Input parameters:
+# $1: Name of the QMP event to check for
+#
+# Checks if the named QMP event that was previously captured
+# into $QEMU_EVENTS. When matched, the QMP event will be echoed
+# and the $matched variable set to 1.
+#
+# _wait_event is more suitable for test usage in most cases
+_check_cached_events()
+{
+ local evname=${1}
+
+ local match="\"event\": \"$evname\""
+
+ matched=0
+ if [ -n "$QEMU_EVENTS" ]; then
+ CURRENT_QEMU_EVENTS=$QEMU_EVENTS
+ QEMU_EVENTS=
+ old_IFS=$IFS
+ IFS="%"
+ for ev in $CURRENT_QEMU_EVENTS
+ do
+ grep -q "$match" < <(echo "${ev}")
+ if [ $? -eq 0 -a $matched = 0 ]; then
+ echo "${ev}" | _filter_testdir | _filter_qemu \
+ | _filter_qemu_io | _filter_qmp | _filter_hmp
+ matched=1
+ else
+ QEMU_EVENTS="${QEMU_EVENTS:+${QEMU_EVENTS}%}${ev}"
+ fi
+ done
+ IFS=$old_IFS
+ fi
+}
+
+# Wait for a named QMP event
+#
+# Input parameters:
+# $1: QEMU handle to use
+# $2: Name of the QMP event to wait for
+#
+# Checks if the named QMP event that was previously captured
+# into $QEMU_EVENTS. If none are present, then waits for the
+# event to arrive on the QMP channel. When matched, the QMP
+# event will be echoed
+_wait_event()
+{
+ local h=${1}
+ local evname=${2}
+
+ while true
+ do
+ _check_cached_events $evname
+
+ if [ $matched = 1 ];
+ then
+ return
+ fi
+
+ only_capture_events=1 qemu_error_no_exit=1 _timed_wait_for ${h}
+
+ if [ ${QEMU_STATUS[$h]} -ne 0 ] ; then
+ echo "Timeout waiting for event ${evname} on handle ${h}"
+ exit 1 #Timeout means the test failed
+ fi
+ done
+}
+
# Launch a QEMU process.
#
# Input parameters:
--
2.26.2
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v3 7/7] migration: introduce snapshot-{save, load, delete} QMP commands
2020-08-27 11:15 [PATCH v3 0/7] migration: bring improved savevm/loadvm/delvm to QMP Daniel P. Berrangé
` (5 preceding siblings ...)
2020-08-27 11:16 ` [PATCH v3 6/7] iotests: add support for capturing and matching QMP events Daniel P. Berrangé
@ 2020-08-27 11:16 ` Daniel P. Berrangé
2020-09-01 14:20 ` Markus Armbruster
2020-09-11 11:52 ` [PATCH v3 0/7] migration: bring improved savevm/loadvm/delvm to QMP Dr. David Alan Gilbert
7 siblings, 1 reply; 19+ messages in thread
From: Daniel P. Berrangé @ 2020-08-27 11:16 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Peter Krempa, Daniel P. Berrangé,
Denis V. Lunev, qemu-block, Juan Quintela, John Snow,
Markus Armbruster, Dr. David Alan Gilbert, Pavel Dovgalyuk,
Paolo Bonzini, Max Reitz
savevm, loadvm and delvm are some of the few HMP commands that have never
been converted to use QMP. The primary reason for this lack of conversion
is that they block execution of the thread for as long as they run.
Despite this downside, however, libvirt and applications using libvirt
have used these commands for as long as QMP has existed, via the
"human-monitor-command" passthrough command. IOW, while it is clearly
desirable to be able to fix the blocking problem, this is not an
immediate obstacle to real world usage.
Meanwhile there is a need for other features which involve adding new
parameters to the commands. This is possible with HMP passthrough, but
it provides no reliable way for apps to introspect features, so using
QAPI modelling is highly desirable.
This patch thus introduces new snapshot-{load,save,delete} commands to
QMP that are intended to replace the old HMP counterparts. The new
commands are given different names, because they will be using the new
QEMU job framework and thus will have diverging behaviour from the HMP
originals. It would thus be misleading to keep the same name.
While this design uses the generic job framework, the current impl is
still blocking. The intention that the blocking problem is fixed later.
None the less applications using these new commands should assume that
they are asynchronous and thus wait for the job status change event to
indicate completion.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
include/migration/snapshot.h | 10 +-
migration/savevm.c | 220 ++++++++++++++++++++-
monitor/hmp-cmds.c | 4 +-
qapi/job.json | 9 +-
qapi/migration.json | 135 +++++++++++++
replay/replay-snapshot.c | 4 +-
softmmu/vl.c | 2 +-
tests/qemu-iotests/310 | 255 ++++++++++++++++++++++++
tests/qemu-iotests/310.out | 369 +++++++++++++++++++++++++++++++++++
tests/qemu-iotests/group | 1 +
10 files changed, 991 insertions(+), 18 deletions(-)
create mode 100755 tests/qemu-iotests/310
create mode 100644 tests/qemu-iotests/310.out
diff --git a/include/migration/snapshot.h b/include/migration/snapshot.h
index c85b6ec75b..f2ed9d1e43 100644
--- a/include/migration/snapshot.h
+++ b/include/migration/snapshot.h
@@ -15,7 +15,13 @@
#ifndef QEMU_MIGRATION_SNAPSHOT_H
#define QEMU_MIGRATION_SNAPSHOT_H
-int save_snapshot(const char *name, Error **errp);
-int load_snapshot(const char *name, Error **errp);
+#include "qapi/qapi-builtin-types.h"
+
+int save_snapshot(const char *name,
+ const char *vmstate, strList *devices,
+ Error **errp);
+int load_snapshot(const char *name,
+ const char *vmstate, strList *devices,
+ Error **errp);
#endif
diff --git a/migration/savevm.c b/migration/savevm.c
index 4a52704132..c5d8131d82 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -43,6 +43,8 @@
#include "qapi/error.h"
#include "qapi/qapi-commands-migration.h"
#include "qapi/qapi-commands-misc.h"
+#include "qapi/clone-visitor.h"
+#include "qapi/qapi-builtin-visit.h"
#include "qapi/qmp/qerror.h"
#include "qemu/error-report.h"
#include "sysemu/cpus.h"
@@ -2658,7 +2660,8 @@ int qemu_load_device_state(QEMUFile *f)
return 0;
}
-int save_snapshot(const char *name, Error **errp)
+int save_snapshot(const char *name, const char *vmstate,
+ strList *devices, Error **errp)
{
BlockDriverState *bs;
QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
@@ -2680,18 +2683,18 @@ int save_snapshot(const char *name, Error **errp)
return ret;
}
- if (!bdrv_all_can_snapshot(NULL, errp)) {
+ if (!bdrv_all_can_snapshot(devices, errp)) {
return ret;
}
/* Delete old snapshots of the same name */
if (name) {
- if (bdrv_all_delete_snapshot(name, NULL, errp) < 0) {
+ if (bdrv_all_delete_snapshot(name, devices, errp) < 0) {
return ret;
}
}
- bs = bdrv_all_find_vmstate_bs(NULL, NULL, errp);
+ bs = bdrv_all_find_vmstate_bs(vmstate, devices, errp);
if (bs == NULL) {
return ret;
}
@@ -2757,7 +2760,7 @@ int save_snapshot(const char *name, Error **errp)
aio_context_release(aio_context);
aio_context = NULL;
- ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, NULL, errp);
+ ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, devices, errp);
if (ret < 0) {
goto the_end;
}
@@ -2858,7 +2861,8 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
migration_incoming_state_destroy();
}
-int load_snapshot(const char *name, Error **errp)
+int load_snapshot(const char *name, const char *vmstate,
+ strList *devices, Error **errp)
{
BlockDriverState *bs_vm_state;
QEMUSnapshotInfo sn;
@@ -2873,15 +2877,15 @@ int load_snapshot(const char *name, Error **errp)
return -1;
}
- if (!bdrv_all_can_snapshot(NULL, errp)) {
+ if (!bdrv_all_can_snapshot(devices, errp)) {
return -1;
}
- ret = bdrv_all_find_snapshot(name, NULL, errp);
+ ret = bdrv_all_find_snapshot(name, devices, errp);
if (ret < 0) {
return -1;
}
- bs_vm_state = bdrv_all_find_vmstate_bs(NULL, NULL, errp);
+ bs_vm_state = bdrv_all_find_vmstate_bs(vmstate, devices, errp);
if (!bs_vm_state) {
return -1;
}
@@ -2902,7 +2906,7 @@ int load_snapshot(const char *name, Error **errp)
/* Flush all IO requests so they don't interfere with the new state. */
bdrv_drain_all_begin();
- ret = bdrv_all_goto_snapshot(name, NULL, errp);
+ ret = bdrv_all_goto_snapshot(name, devices, errp);
if (ret < 0) {
goto err_drain;
}
@@ -2936,6 +2940,19 @@ err_drain:
return -1;
}
+static int delete_snapshot(const char *name, strList *devices, Error **errp)
+{
+ if (!bdrv_all_can_snapshot(devices, errp)) {
+ return -1;
+ }
+
+ if (bdrv_all_delete_snapshot(name, devices, errp) < 0) {
+ return -1;
+ }
+
+ return 0;
+}
+
void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev)
{
qemu_ram_set_idstr(mr->ram_block,
@@ -2963,3 +2980,186 @@ bool vmstate_check_only_migratable(const VMStateDescription *vmsd)
return !(vmsd && vmsd->unmigratable);
}
+
+typedef struct SnapshotJob {
+ Job common;
+ char *tag;
+ char *vmstate;
+ strList *devices;
+ Coroutine *co;
+ Error **errp;
+ int ret;
+} SnapshotJob;
+
+static void qmp_snapshot_job_free(SnapshotJob *s)
+{
+ g_free(s->tag);
+ g_free(s->vmstate);
+ qapi_free_strList(s->devices);
+}
+
+
+static void snapshot_load_job_bh(void *opaque)
+{
+ Job *job = opaque;
+ SnapshotJob *s = container_of(job, SnapshotJob, common);
+ int saved_vm_running;
+
+ job_progress_set_remaining(&s->common, 1);
+
+ saved_vm_running = runstate_is_running();
+ vm_stop(RUN_STATE_RESTORE_VM);
+
+ s->ret = load_snapshot(s->tag, s->vmstate, s->devices, s->errp);
+ if (s->ret == 0 && saved_vm_running) {
+ vm_start();
+ }
+
+ job_progress_update(&s->common, 1);
+
+ qmp_snapshot_job_free(s);
+ aio_co_wake(s->co);
+}
+
+static void snapshot_save_job_bh(void *opaque)
+{
+ Job *job = opaque;
+ SnapshotJob *s = container_of(job, SnapshotJob, common);
+
+ job_progress_set_remaining(&s->common, 1);
+ s->ret = save_snapshot(s->tag, s->vmstate, s->devices, s->errp);
+ job_progress_update(&s->common, 1);
+
+ qmp_snapshot_job_free(s);
+ aio_co_wake(s->co);
+}
+
+static void snapshot_delete_job_bh(void *opaque)
+{
+ Job *job = opaque;
+ SnapshotJob *s = container_of(job, SnapshotJob, common);
+
+ job_progress_set_remaining(&s->common, 1);
+ s->ret = delete_snapshot(s->tag, s->devices, s->errp);
+ job_progress_update(&s->common, 1);
+
+ qmp_snapshot_job_free(s);
+ aio_co_wake(s->co);
+}
+
+static int coroutine_fn snapshot_save_job_run(Job *job, Error **errp)
+{
+ SnapshotJob *s = container_of(job, SnapshotJob, common);
+ s->errp = errp;
+ s->co = qemu_coroutine_self();
+ aio_bh_schedule_oneshot(qemu_get_aio_context(),
+ snapshot_save_job_bh, job);
+ qemu_coroutine_yield();
+ return s->ret;
+}
+
+static int coroutine_fn snapshot_load_job_run(Job *job, Error **errp)
+{
+ SnapshotJob *s = container_of(job, SnapshotJob, common);
+ s->errp = errp;
+ s->co = qemu_coroutine_self();
+ aio_bh_schedule_oneshot(qemu_get_aio_context(),
+ snapshot_load_job_bh, job);
+ qemu_coroutine_yield();
+ return s->ret;
+}
+
+static int coroutine_fn snapshot_delete_job_run(Job *job, Error **errp)
+{
+ SnapshotJob *s = container_of(job, SnapshotJob, common);
+ s->errp = errp;
+ s->co = qemu_coroutine_self();
+ aio_bh_schedule_oneshot(qemu_get_aio_context(),
+ snapshot_delete_job_bh, job);
+ qemu_coroutine_yield();
+ return s->ret;
+}
+
+
+static const JobDriver snapshot_load_job_driver = {
+ .instance_size = sizeof(SnapshotJob),
+ .job_type = JOB_TYPE_SNAPSHOT_LOAD,
+ .run = snapshot_load_job_run,
+};
+
+static const JobDriver snapshot_save_job_driver = {
+ .instance_size = sizeof(SnapshotJob),
+ .job_type = JOB_TYPE_SNAPSHOT_SAVE,
+ .run = snapshot_save_job_run,
+};
+
+static const JobDriver snapshot_delete_job_driver = {
+ .instance_size = sizeof(SnapshotJob),
+ .job_type = JOB_TYPE_SNAPSHOT_DELETE,
+ .run = snapshot_delete_job_run,
+};
+
+
+void qmp_snapshot_save(const char *job_id,
+ const char *tag,
+ bool has_vmstate, const char *vmstate,
+ bool has_devices, strList *devices,
+ Error **errp)
+{
+ SnapshotJob *s;
+
+ s = job_create(job_id, &snapshot_save_job_driver, NULL,
+ qemu_get_aio_context(), JOB_MANUAL_DISMISS,
+ NULL, NULL, errp);
+ if (!s) {
+ return;
+ }
+
+ s->tag = g_strdup(tag);
+ s->vmstate = has_vmstate ? g_strdup(vmstate) : NULL;
+ s->devices = has_devices ? QAPI_CLONE(strList, devices) : NULL;
+
+ job_start(&s->common);
+}
+
+void qmp_snapshot_load(const char *job_id,
+ const char *tag,
+ bool has_vmstate, const char *vmstate,
+ bool has_devices, strList *devices,
+ Error **errp)
+{
+ SnapshotJob *s;
+
+ s = job_create(job_id, &snapshot_load_job_driver, NULL,
+ qemu_get_aio_context(), JOB_MANUAL_DISMISS,
+ NULL, NULL, errp);
+ if (!s) {
+ return;
+ }
+
+ s->tag = g_strdup(tag);
+ s->vmstate = has_vmstate ? g_strdup(vmstate) : NULL;
+ s->devices = has_devices ? QAPI_CLONE(strList, devices) : NULL;
+
+ job_start(&s->common);
+}
+
+void qmp_snapshot_delete(const char *job_id,
+ const char *tag,
+ bool has_devices, strList *devices,
+ Error **errp)
+{
+ SnapshotJob *s;
+
+ s = job_create(job_id, &snapshot_delete_job_driver, NULL,
+ qemu_get_aio_context(), JOB_MANUAL_DISMISS,
+ NULL, NULL, errp);
+ if (!s) {
+ return;
+ }
+
+ s->tag = g_strdup(tag);
+ s->devices = has_devices ? QAPI_CLONE(strList, devices) : NULL;
+
+ job_start(&s->common);
+}
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index 51f624b5ea..2ac2c109db 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -1121,7 +1121,7 @@ void hmp_loadvm(Monitor *mon, const QDict *qdict)
vm_stop(RUN_STATE_RESTORE_VM);
- if (load_snapshot(name, &err) == 0 && saved_vm_running) {
+ if (load_snapshot(name, NULL, NULL, &err) == 0 && saved_vm_running) {
vm_start();
}
hmp_handle_error(mon, err);
@@ -1131,7 +1131,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
{
Error *err = NULL;
- save_snapshot(qdict_get_try_str(qdict, "name"), &err);
+ save_snapshot(qdict_get_try_str(qdict, "name"), NULL, NULL, &err);
hmp_handle_error(mon, err);
}
diff --git a/qapi/job.json b/qapi/job.json
index 280c2f76f1..51bee470f0 100644
--- a/qapi/job.json
+++ b/qapi/job.json
@@ -22,10 +22,17 @@
#
# @amend: image options amend job type, see "x-blockdev-amend" (since 5.1)
#
+# @snapshot-load: snapshot load job type, see "loadvm" (since 5.2)
+#
+# @snapshot-save: snapshot save job type, see "savevm" (since 5.2)
+#
+# @snapshot-delete: snapshot delete job type, see "delvm" (since 5.2)
+#
# Since: 1.7
##
{ 'enum': 'JobType',
- 'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend'] }
+ 'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend',
+ 'snapshot-load', 'snapshot-save', 'snapshot-delete'] }
##
# @JobStatus:
diff --git a/qapi/migration.json b/qapi/migration.json
index 5f6b06172c..d70f627b77 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1720,3 +1720,138 @@
##
{ 'event': 'UNPLUG_PRIMARY',
'data': { 'device-id': 'str' } }
+
+##
+# @snapshot-save:
+#
+# Save a VM snapshot
+#
+# @job-id: identifier for the newly created job
+# @tag: name of the snapshot to create. If it already
+# exists it will be replaced.
+# @devices: list of block device node names to save a snapshot to
+# @vmstate: block device node name to save vmstate to
+#
+# Applications should not assume that the snapshot save is complete
+# when this command returns. Completion is indicated by the job
+# status. Clients can wait for the JOB_STATUS_CHANGE event. If the
+# job aborts, errors can be obtained via the 'query-jobs' command,
+# though. Note that at this time most vmstate procssing errors only
+# get printed to stderr. This limitation will be fixed at a future
+# date.
+#
+# Note that the VM CPUs will be paused during the time it takes to
+# save the snapshot
+#
+# If @devices is not specified, or is an empty list, then the
+# historical default logic for picking devices will be used.
+#
+# If @vmstate is not specified, then the first valid block
+# device will be used for vmstate.
+#
+# Returns: nothing
+#
+# Example:
+#
+# -> { "execute": "snapshot-save",
+# "data": {
+# "job-id": "snapsave0",
+# "tag": "my-snap",
+# "vmstate": "disk0",
+# "devices": ["disk0", "disk1"]
+# }
+# }
+# <- { "return": { } }
+#
+# Since: 5.2
+##
+{ 'command': 'snapshot-save',
+ 'data': { 'job-id': 'str',
+ 'tag': 'str',
+ '*vmstate': 'str',
+ '*devices': ['str'] } }
+
+##
+# @snapshot-load:
+#
+# Load a VM snapshot
+#
+# @job-id: identifier for the newly created job
+# @tag: name of the snapshot to load.
+# @devices: list of block device node names to load a snapshot from
+# @vmstate: block device node name to load vmstate from
+#
+# Applications should not assume that the snapshot load is complete
+# when this command returns. Completion is indicated by the job
+# status. Clients can wait for the JOB_STATUS_CHANGE event. If the
+# job aborts, errors can be obtained via the 'query-jobs' command,
+# though. Note that at this time most vmstate procssing errors only
+# get printed to stderr. This limitation will be fixed at a future
+# date.
+#
+# If @devices is not specified, or is an empty list, then the
+# historical default logic for picking devices will be used.
+#
+# If @vmstate is not specified, then the first valid block
+# device will be used for vmstate.
+#
+# Returns: nothing
+#
+# Example:
+#
+# -> { "execute": "snapshot-load",
+# "data": {
+# "job-id": "snapload0",
+# "tag": "my-snap",
+# "vmstate": "disk0",
+# "devices": ["disk0", "disk1"]
+# }
+# }
+# <- { "return": { } }
+#
+# Since: 5.2
+##
+{ 'command': 'snapshot-load',
+ 'data': { 'job-id': 'str',
+ 'tag': 'str',
+ '*vmstate': 'str',
+ '*devices': ['str'] } }
+
+##
+# @snapshot-delete:
+#
+# Delete a VM snapshot
+#
+# @job-id: identifier for the newly created job
+# @tag: name of the snapshot to delete.
+# @devices: list of block device node names to delete a snapshot from
+#
+# Applications should not assume that the snapshot load is complete
+# when this command returns. Completion is indicated by the job
+# status. Clients can wait for the JOB_STATUS_CHANGE event.
+#
+# Note that the VM CPUs will be paused during the time it takes to
+# delete the snapshot
+#
+# If @devices is not specified, or is an empty list, then the
+# historical default logic for picking devices will be used.
+#
+# Returns: nothing
+#
+# Example:
+#
+# -> { "execute": "snapshot-delete",
+# "data": {
+# "job-id": "snapdelete0",
+# "tag": "my-snap",
+# "devices": ["disk0", "disk1"]
+# }
+# }
+# <- { "return": { } }
+#
+# Since: 5.2
+##
+{ 'command': 'snapshot-delete',
+ 'data': { 'job-id': 'str',
+ 'tag': 'str',
+ '*devices': ['str'] } }
diff --git a/replay/replay-snapshot.c b/replay/replay-snapshot.c
index e26fa4c892..f0f45a4f24 100644
--- a/replay/replay-snapshot.c
+++ b/replay/replay-snapshot.c
@@ -77,13 +77,13 @@ void replay_vmstate_init(void)
if (replay_snapshot) {
if (replay_mode == REPLAY_MODE_RECORD) {
- if (save_snapshot(replay_snapshot, &err) != 0) {
+ if (save_snapshot(replay_snapshot, NULL, NULL, &err) != 0) {
error_report_err(err);
error_report("Could not create snapshot for icount record");
exit(1);
}
} else if (replay_mode == REPLAY_MODE_PLAY) {
- if (load_snapshot(replay_snapshot, &err) != 0) {
+ if (load_snapshot(replay_snapshot, NULL, NULL, &err) != 0) {
error_report_err(err);
error_report("Could not load snapshot for icount replay");
exit(1);
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 0cc86b0766..8dbc2fe638 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -4459,7 +4459,7 @@ void qemu_init(int argc, char **argv, char **envp)
register_global_state();
if (loadvm) {
Error *local_err = NULL;
- if (load_snapshot(loadvm, &local_err) < 0) {
+ if (load_snapshot(loadvm, NULL, NULL, &local_err) < 0) {
error_report_err(local_err);
autostart = 0;
exit(1);
diff --git a/tests/qemu-iotests/310 b/tests/qemu-iotests/310
new file mode 100755
index 0000000000..66cb58d2c8
--- /dev/null
+++ b/tests/qemu-iotests/310
@@ -0,0 +1,255 @@
+#!/usr/bin/env bash
+#
+# Test which nodes are involved in internal snapshots
+#
+# Copyright (C) 2019 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=berrange@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1 # failure is the default!
+
+_cleanup()
+{
+ _cleanup_qemu
+ _cleanup_test_img
+ TEST_IMG="$TEST_IMG.alt1" _cleanup_test_img
+ TEST_IMG="$TEST_IMG.alt2" _cleanup_test_img
+ rm -f "$SOCK_DIR/nbd"
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+_require_drivers copy-on-read
+
+# Internal snapshots are (currently) impossible with refcount_bits=1,
+# and generally impossible with external data files
+_unsupported_imgopts 'refcount_bits=1[^0-9]' data_file
+
+_require_devices virtio-blk
+
+
+size=128M
+
+if [ -n "$BACKING_FILE" ]; then
+ _make_test_img -b "$BACKING_FILE" -F $IMGFMT $size
+else
+ _make_test_img $size
+fi
+
+ _make_test_img $size
+
+echo "Single disk test"
+
+export capture_events="JOB_STATUS_CHANGE STOP RESUME"
+
+run_snapshot()
+{
+ local op=$1
+
+ _send_qemu_cmd $QEMU_HANDLE "{\"execute\": \"snapshot-${op}\", \"arguments\": {\"job-id\": \"snap${op}0\", \"tag\": \"snap0\", \"devices\": $SNAP_DEVICES}}" "return"
+ # created
+ _wait_event $QEMU_HANDLE "JOB_STATUS_CHANGE"
+ # running
+ _wait_event $QEMU_HANDLE "JOB_STATUS_CHANGE"
+ if [ $op = "delete" ]; then
+ if [ $SNAP_DELETE_FAIL = 1 ]; then
+ # aborting
+ _wait_event $QEMU_HANDLE "JOB_STATUS_CHANGE"
+ else
+ # waiting
+ _wait_event $QEMU_HANDLE "JOB_STATUS_CHANGE"
+ # pending
+ _wait_event $QEMU_HANDLE "JOB_STATUS_CHANGE"
+ fi
+ else
+ if [ $SNAP_FAIL = 0 ]; then
+ _wait_event $QEMU_HANDLE "STOP"
+ _wait_event $QEMU_HANDLE "RESUME"
+ # waiting
+ _wait_event $QEMU_HANDLE "JOB_STATUS_CHANGE"
+ # pending
+ _wait_event $QEMU_HANDLE "JOB_STATUS_CHANGE"
+ else
+ if [ $op = "load" ]; then
+ _wait_event $QEMU_HANDLE "STOP"
+ fi
+ # aborting
+ _wait_event $QEMU_HANDLE "JOB_STATUS_CHANGE"
+ fi
+ fi
+ # concluded
+ _wait_event $QEMU_HANDLE "JOB_STATUS_CHANGE"
+ _send_qemu_cmd $QEMU_HANDLE "{\"execute\": \"query-jobs\"}" "return"
+ _send_qemu_cmd $QEMU_HANDLE "{\"execute\": \"job-dismiss\", \"arguments\": {\"id\": \"snap${op}0\"}}" "return"
+ # null
+ _wait_event $QEMU_HANDLE "JOB_STATUS_CHANGE"
+}
+
+run_test()
+{
+ keep_stderr=y
+ _launch_qemu -nodefaults -nographic "$@"
+
+ _send_qemu_cmd $QEMU_HANDLE '{"execute": "qmp_capabilities"}' 'return'
+
+ run_snapshot "save"
+ run_snapshot "load"
+ run_snapshot "delete"
+
+ _send_qemu_cmd $QEMU_HANDLE '{"execute": "quit"}' 'return'
+
+ wait=1 _cleanup_qemu
+}
+
+
+echo
+echo "===== Snapshot single qcow2 image ====="
+echo
+
+SNAP_DEVICES='["diskfmt0"]' SNAP_FAIL=0 SNAP_DELETE_FAIL=0 \
+ run_test \
+ -blockdev "{'driver':'file','filename':'$TEST_IMG','node-name':'disk0'}" \
+ -blockdev "{'driver':'qcow2','file':'disk0','node-name':'diskfmt0'}"
+
+echo
+echo "===== Snapshot no image ====="
+echo
+
+TEST_IMG="$TEST_IMG.alt1" _make_test_img $size
+
+SNAP_DEVICES="[]" SNAP_FAIL=1 SNAP_DELETE_FAIL=0 \
+ run_test
+
+
+echo
+echo "===== Snapshot missing image ====="
+echo
+
+SNAP_DEVICES='["diskfmt1729"]' SNAP_FAIL=1 SNAP_DELETE_FAIL=1 \
+ run_test \
+ -blockdev "{'driver':'file','filename':'$TEST_IMG','node-name':'disk0'}" \
+ -blockdev "{'driver':'qcow2','file':'disk0','node-name':'diskfmt0'}"
+
+
+echo
+echo "===== Snapshot protocol instead of format ====="
+echo
+
+SNAP_DEVICES='["disk0"]' SNAP_FAIL=1 SNAP_DELETE_FAIL=1 \
+ run_test \
+ -blockdev "{'driver':'file','filename':'$TEST_IMG','node-name':'disk0'}" \
+ -blockdev "{'driver':'qcow2','file':'disk0','node-name':'diskfmt0'}"
+
+
+echo
+echo "===== Snapshot dual qcow2 image ====="
+echo
+
+SNAP_DEVICES='["diskfmt0", "diskfmt1"]' SNAP_FAIL=0 SNAP_DELETE_FAIL=0 \
+ run_test \
+ -blockdev "{'driver':'file','filename':'$TEST_IMG','node-name':'disk0'}" \
+ -blockdev "{'driver':'qcow2','file':'disk0','node-name':'diskfmt0'}" \
+ -blockdev "{'driver':'file','filename':'$TEST_IMG.alt1','node-name':'disk1'}" \
+ -blockdev "{'driver':'qcow2','file':'disk1','node-name':'diskfmt1'}"
+
+
+echo
+echo "===== Snapshot error with raw image ====="
+echo
+
+IMGOPTS= IMGFMT=raw TEST_IMG="$TEST_IMG.alt2" _make_test_img $size
+
+SNAP_DEVICES='["diskfmt0", "diskfmt1", "diskfmt2"]' SNAP_FAIL=1 SNAP_DELETE_FAIL=1 \
+ run_test \
+ -blockdev "{'driver':'file','filename':'$TEST_IMG','node-name':'disk0'}" \
+ -blockdev "{'driver':'qcow2','file':'disk0','node-name':'diskfmt0'}" \
+ -blockdev "{'driver':'file','filename':'$TEST_IMG.alt1','node-name':'disk1'}" \
+ -blockdev "{'driver':'qcow2','file':'disk1','node-name':'diskfmt1'}" \
+ -blockdev "{'driver':'file','filename':'$TEST_IMG.alt2','node-name':'disk2'}" \
+ -blockdev "{'driver':'raw','file':'disk2','node-name':'diskfmt2'}"
+
+
+echo
+echo "===== Snapshot with raw image excluded ====="
+echo
+
+SNAP_DEVICES='["diskfmt0", "diskfmt1"]' SNAP_FAIL=0 SNAP_DELETE_FAIL=0 \
+ run_test \
+ -blockdev "{'driver':'file','filename':'$TEST_IMG','node-name':'disk0'}" \
+ -blockdev "{'driver':'qcow2','file':'disk0','node-name':'diskfmt0'}" \
+ -blockdev "{'driver':'file','filename':'$TEST_IMG.alt1','node-name':'disk1'}" \
+ -blockdev "{'driver':'qcow2','file':'disk1','node-name':'diskfmt1'}" \
+ -blockdev "{'driver':'file','filename':'$TEST_IMG.alt2','node-name':'disk2'}" \
+ -blockdev "{'driver':'raw','file':'disk2','node-name':'diskfmt2'}"
+
+echo
+echo "===== Snapshot bad error reporting to stderr ====="
+echo
+
+# This demonstrates that we're not capturing vmstate loading failures
+# into QMP errors, they're ending up in stderr instead. vmstate needs
+# to report errors via Error object but that is a major piece of work
+# for the future. This test case's expected output log will need
+# adjusting when that is done.
+
+SNAP_DEVICES='["diskfmt0"]'
+SNAP_FAIL=1
+SNAP_DELETE_FAIL=0
+
+keep_stderr=y
+_launch_qemu -nodefaults -nographic \
+ -device virtio-rng \
+ -blockdev "{'driver':'file','filename':'$TEST_IMG','node-name':'disk0'}" \
+ -blockdev "{'driver':'qcow2','file':'disk0','node-name':'diskfmt0'}"
+
+_send_qemu_cmd $QEMU_HANDLE '{"execute": "qmp_capabilities"}' 'return'
+
+run_snapshot "save"
+
+_send_qemu_cmd $QEMU_HANDLE '{"execute": "quit"}' 'return'
+
+wait=1 _cleanup_qemu
+
+# leave off virtio-rng to provoke vmstate failure
+_launch_qemu -nodefaults -nographic \
+ -blockdev "{'driver':'file','filename':'$TEST_IMG','node-name':'disk0'}" \
+ -blockdev "{'driver':'qcow2','file':'disk0','node-name':'diskfmt0'}"
+
+_send_qemu_cmd $QEMU_HANDLE '{"execute": "qmp_capabilities"}' 'return'
+
+run_snapshot "load"
+run_snapshot "delete"
+
+_send_qemu_cmd $QEMU_HANDLE '{"execute": "quit"}' 'return'
+
+wait=1 _cleanup_qemu
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/310.out b/tests/qemu-iotests/310.out
new file mode 100644
index 0000000000..32c797ab1e
--- /dev/null
+++ b/tests/qemu-iotests/310.out
@@ -0,0 +1,369 @@
+QA output created by 310
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+Single disk test
+
+===== Snapshot single qcow2 image =====
+
+{"execute": "qmp_capabilities"}
+{"return": {}}
+{"execute": "snapshot-save", "arguments": {"job-id": "snapsave0", "tag": "snap0", "devices": ["diskfmt0"]}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "STOP"}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "RESUME"}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapsave0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-save", "id": "snapsave0"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapsave0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "snapsave0"}}
+{"execute": "snapshot-load", "arguments": {"job-id": "snapload0", "tag": "snap0", "devices": ["diskfmt0"]}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "STOP"}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "RESUME"}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapload0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-load", "id": "snapload0"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapload0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "snapload0"}}
+{"execute": "snapshot-delete", "arguments": {"job-id": "snapdelete0", "tag": "snap0", "devices": ["diskfmt0"]}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapdelete0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-delete", "id": "snapdelete0"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapdelete0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "snapdelete0"}}
+{"execute": "quit"}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
+
+===== Snapshot no image =====
+
+Formatting 'TEST_DIR/t.IMGFMT.alt1', fmt=IMGFMT size=134217728
+{"execute": "qmp_capabilities"}
+{"return": {}}
+{"execute": "snapshot-save", "arguments": {"job-id": "snapsave0", "tag": "snap0", "devices": []}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapsave0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-save", "id": "snapsave0", "error": "No block device supports snapshots"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapsave0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "snapsave0"}}
+{"execute": "snapshot-load", "arguments": {"job-id": "snapload0", "tag": "snap0", "devices": []}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "STOP"}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapload0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-load", "id": "snapload0", "error": "No block device supports snapshots"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapload0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "snapload0"}}
+{"execute": "snapshot-delete", "arguments": {"job-id": "snapdelete0", "tag": "snap0", "devices": []}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapdelete0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-delete", "id": "snapdelete0"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapdelete0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "snapdelete0"}}
+{"execute": "quit"}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
+
+===== Snapshot missing image =====
+
+{"execute": "qmp_capabilities"}
+{"return": {}}
+{"execute": "snapshot-save", "arguments": {"job-id": "snapsave0", "tag": "snap0", "devices": ["diskfmt1729"]}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapsave0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-save", "id": "snapsave0", "error": "No block device node 'diskfmt1729'"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapsave0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "snapsave0"}}
+{"execute": "snapshot-load", "arguments": {"job-id": "snapload0", "tag": "snap0", "devices": ["diskfmt1729"]}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "STOP"}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapload0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-load", "id": "snapload0", "error": "No block device node 'diskfmt1729'"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapload0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "snapload0"}}
+{"execute": "snapshot-delete", "arguments": {"job-id": "snapdelete0", "tag": "snap0", "devices": ["diskfmt1729"]}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapdelete0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-delete", "id": "snapdelete0", "error": "No block device node 'diskfmt1729'"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapdelete0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "snapdelete0"}}
+{"execute": "quit"}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
+
+===== Snapshot protocol instead of format =====
+
+{"execute": "qmp_capabilities"}
+{"return": {}}
+{"execute": "snapshot-save", "arguments": {"job-id": "snapsave0", "tag": "snap0", "devices": ["disk0"]}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapsave0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-save", "id": "snapsave0", "error": "Device 'disk0' is writable but does not support snapshots"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapsave0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "snapsave0"}}
+{"execute": "snapshot-load", "arguments": {"job-id": "snapload0", "tag": "snap0", "devices": ["disk0"]}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "STOP"}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapload0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-load", "id": "snapload0", "error": "Device 'disk0' is writable but does not support snapshots"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapload0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "snapload0"}}
+{"execute": "snapshot-delete", "arguments": {"job-id": "snapdelete0", "tag": "snap0", "devices": ["disk0"]}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapdelete0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-delete", "id": "snapdelete0", "error": "Device 'disk0' is writable but does not support snapshots"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapdelete0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "snapdelete0"}}
+{"execute": "quit"}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
+
+===== Snapshot dual qcow2 image =====
+
+{"execute": "qmp_capabilities"}
+{"return": {}}
+{"execute": "snapshot-save", "arguments": {"job-id": "snapsave0", "tag": "snap0", "devices": ["diskfmt0", "diskfmt1"]}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "STOP"}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "RESUME"}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapsave0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-save", "id": "snapsave0"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapsave0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "snapsave0"}}
+{"execute": "snapshot-load", "arguments": {"job-id": "snapload0", "tag": "snap0", "devices": ["diskfmt0", "diskfmt1"]}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "STOP"}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "RESUME"}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapload0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-load", "id": "snapload0"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapload0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "snapload0"}}
+{"execute": "snapshot-delete", "arguments": {"job-id": "snapdelete0", "tag": "snap0", "devices": ["diskfmt0", "diskfmt1"]}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapdelete0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-delete", "id": "snapdelete0"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapdelete0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "snapdelete0"}}
+{"execute": "quit"}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
+
+===== Snapshot error with raw image =====
+
+Formatting 'TEST_DIR/t.qcow2.alt2', fmt=IMGFMT size=134217728
+{"execute": "qmp_capabilities"}
+{"return": {}}
+{"execute": "snapshot-save", "arguments": {"job-id": "snapsave0", "tag": "snap0", "devices": ["diskfmt0", "diskfmt1", "diskfmt2"]}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapsave0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-save", "id": "snapsave0", "error": "Device 'diskfmt2' is writable but does not support snapshots"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapsave0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "snapsave0"}}
+{"execute": "snapshot-load", "arguments": {"job-id": "snapload0", "tag": "snap0", "devices": ["diskfmt0", "diskfmt1", "diskfmt2"]}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "STOP"}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapload0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-load", "id": "snapload0", "error": "Device 'diskfmt2' is writable but does not support snapshots"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapload0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "snapload0"}}
+{"execute": "snapshot-delete", "arguments": {"job-id": "snapdelete0", "tag": "snap0", "devices": ["diskfmt0", "diskfmt1", "diskfmt2"]}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapdelete0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-delete", "id": "snapdelete0", "error": "Device 'diskfmt2' is writable but does not support snapshots"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapdelete0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "snapdelete0"}}
+{"execute": "quit"}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
+
+===== Snapshot with raw image excluded =====
+
+{"execute": "qmp_capabilities"}
+{"return": {}}
+{"execute": "snapshot-save", "arguments": {"job-id": "snapsave0", "tag": "snap0", "devices": ["diskfmt0", "diskfmt1"]}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "STOP"}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "RESUME"}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapsave0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-save", "id": "snapsave0"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapsave0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "snapsave0"}}
+{"execute": "snapshot-load", "arguments": {"job-id": "snapload0", "tag": "snap0", "devices": ["diskfmt0", "diskfmt1"]}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "STOP"}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "RESUME"}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapload0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-load", "id": "snapload0"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapload0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "snapload0"}}
+{"execute": "snapshot-delete", "arguments": {"job-id": "snapdelete0", "tag": "snap0", "devices": ["diskfmt0", "diskfmt1"]}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapdelete0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-delete", "id": "snapdelete0"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapdelete0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "snapdelete0"}}
+{"execute": "quit"}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
+
+===== Snapshot bad error reporting to stderr =====
+
+{"execute": "qmp_capabilities"}
+{"return": {}}
+{"execute": "snapshot-save", "arguments": {"job-id": "snapsave0", "tag": "snap0", "devices": ["diskfmt0"]}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "snapsave0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-save", "id": "snapsave0"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapsave0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapsave0"}}
+{"execute": "quit"}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
+{"execute": "qmp_capabilities"}
+{"return": {}}
+{"execute": "snapshot-load", "arguments": {"job-id": "snapload0", "tag": "snap0", "devices": ["diskfmt0"]}}
+qemu-system-x86_64: Unknown savevm section or instance '0000:00:02.0/virtio-rng' 0. Make sure that your current VM setup matches your saved VM setup, including any hotplugged devices
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "snapsave0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "STOP"}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "aborting", "id": "snapload0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-load", "id": "snapload0", "error": "Error -22 while loading VM state"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapload0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapload0"}}
+{"execute": "snapshot-delete", "arguments": {"job-id": "snapdelete0", "tag": "snap0", "devices": ["diskfmt0"]}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "snapload0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "running", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "waiting", "id": "snapdelete0"}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "pending", "id": "snapdelete0"}}
+{"execute": "query-jobs"}
+{"return": [{"current-progress": 1, "status": "concluded", "total-progress": 1, "type": "snapshot-delete", "id": "snapdelete0"}]}
+{"execute": "job-dismiss", "arguments": {"id": "snapdelete0"}}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "concluded", "id": "snapdelete0"}}
+{"execute": "quit"}
+{"return": {}}
+{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index a53ea7f78b..b2a5dfbc5a 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -290,6 +290,7 @@
277 rw quick
279 rw backing quick
280 rw migration quick
+310 rw quick
281 rw quick
282 rw img quick
283 auto quick
--
2.26.2
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v3 1/7] migration: improve error reporting of block driver state name
2020-08-27 11:16 ` [PATCH v3 1/7] migration: improve error reporting of block driver state name Daniel P. Berrangé
@ 2020-08-27 17:17 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 19+ messages in thread
From: Dr. David Alan Gilbert @ 2020-08-27 17:17 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Kevin Wolf, Peter Krempa, Denis V. Lunev, qemu-block,
Juan Quintela, John Snow, qemu-devel, Markus Armbruster,
Pavel Dovgalyuk, Paolo Bonzini, Max Reitz
* Daniel P. Berrangé (berrange@redhat.com) wrote:
> With blockdev, a BlockDriverState may not have a device name,
> so using a node name is required as an alternative.
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Queuing this one by itself since it's useful anyway.
Dave
> ---
> migration/savevm.c | 12 ++++++------
> tests/qemu-iotests/267.out | 4 ++--
> 2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index a843d202b5..304d98ff78 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2682,7 +2682,7 @@ int save_snapshot(const char *name, Error **errp)
>
> if (!bdrv_all_can_snapshot(&bs)) {
> error_setg(errp, "Device '%s' is writable but does not support "
> - "snapshots", bdrv_get_device_name(bs));
> + "snapshots", bdrv_get_device_or_node_name(bs));
> return ret;
> }
>
> @@ -2691,7 +2691,7 @@ int save_snapshot(const char *name, Error **errp)
> ret = bdrv_all_delete_snapshot(name, &bs1, errp);
> if (ret < 0) {
> error_prepend(errp, "Error while deleting snapshot on device "
> - "'%s': ", bdrv_get_device_name(bs1));
> + "'%s': ", bdrv_get_device_or_node_name(bs1));
> return ret;
> }
> }
> @@ -2766,7 +2766,7 @@ int save_snapshot(const char *name, Error **errp)
> ret = bdrv_all_create_snapshot(sn, bs, vm_state_size, &bs);
> if (ret < 0) {
> error_setg(errp, "Error while creating snapshot on '%s'",
> - bdrv_get_device_name(bs));
> + bdrv_get_device_or_node_name(bs));
> goto the_end;
> }
>
> @@ -2884,14 +2884,14 @@ int load_snapshot(const char *name, Error **errp)
> if (!bdrv_all_can_snapshot(&bs)) {
> error_setg(errp,
> "Device '%s' is writable but does not support snapshots",
> - bdrv_get_device_name(bs));
> + bdrv_get_device_or_node_name(bs));
> return -ENOTSUP;
> }
> ret = bdrv_all_find_snapshot(name, &bs);
> if (ret < 0) {
> error_setg(errp,
> "Device '%s' does not have the requested snapshot '%s'",
> - bdrv_get_device_name(bs), name);
> + bdrv_get_device_or_node_name(bs), name);
> return ret;
> }
>
> @@ -2920,7 +2920,7 @@ int load_snapshot(const char *name, Error **errp)
> ret = bdrv_all_goto_snapshot(name, &bs, errp);
> if (ret < 0) {
> error_prepend(errp, "Could not load snapshot '%s' on '%s': ",
> - name, bdrv_get_device_name(bs));
> + name, bdrv_get_device_or_node_name(bs));
> goto err_drain;
> }
>
> diff --git a/tests/qemu-iotests/267.out b/tests/qemu-iotests/267.out
> index d6d80c099f..215902b3ad 100644
> --- a/tests/qemu-iotests/267.out
> +++ b/tests/qemu-iotests/267.out
> @@ -81,11 +81,11 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> Testing: -blockdev driver=file,filename=TEST_DIR/t.IMGFMT,node-name=file
> QEMU X.Y.Z monitor - type 'help' for more information
> (qemu) savevm snap0
> -Error: Device '' is writable but does not support snapshots
> +Error: Device 'file' is writable but does not support snapshots
> (qemu) info snapshots
> No available block device supports snapshots
> (qemu) loadvm snap0
> -Error: Device '' is writable but does not support snapshots
> +Error: Device 'file' is writable but does not support snapshots
> (qemu) quit
>
> Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> --
> 2.26.2
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 7/7] migration: introduce snapshot-{save, load, delete} QMP commands
2020-08-27 11:16 ` [PATCH v3 7/7] migration: introduce snapshot-{save, load, delete} QMP commands Daniel P. Berrangé
@ 2020-09-01 14:20 ` Markus Armbruster
2020-09-01 16:47 ` Daniel P. Berrangé
0 siblings, 1 reply; 19+ messages in thread
From: Markus Armbruster @ 2020-09-01 14:20 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Kevin Wolf, Peter Krempa, Denis V. Lunev, qemu-block,
Juan Quintela, qemu-devel, Dr. David Alan Gilbert,
Pavel Dovgalyuk, Paolo Bonzini, Max Reitz, John Snow
Daniel P. Berrangé <berrange@redhat.com> writes:
> savevm, loadvm and delvm are some of the few HMP commands that have never
> been converted to use QMP. The primary reason for this lack of conversion
> is that they block execution of the thread for as long as they run.
Nope. The primary reason is that the HMP interface is bonkers.
> Despite this downside, however, libvirt and applications using libvirt
> have used these commands for as long as QMP has existed, via the
> "human-monitor-command" passthrough command. IOW, while it is clearly
> desirable to be able to fix the blocking problem, this is not an
> immediate obstacle to real world usage.
>
> Meanwhile there is a need for other features which involve adding new
> parameters to the commands. This is possible with HMP passthrough, but
> it provides no reliable way for apps to introspect features, so using
> QAPI modelling is highly desirable.
>
> This patch thus introduces new snapshot-{load,save,delete} commands to
> QMP that are intended to replace the old HMP counterparts. The new
> commands are given different names, because they will be using the new
> QEMU job framework and thus will have diverging behaviour from the HMP
> originals. It would thus be misleading to keep the same name.
>
> While this design uses the generic job framework, the current impl is
> still blocking. The intention that the blocking problem is fixed later.
> None the less applications using these new commands should assume that
> they are asynchronous and thus wait for the job status change event to
> indicate completion.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
[...]
> diff --git a/qapi/job.json b/qapi/job.json
> index 280c2f76f1..51bee470f0 100644
> --- a/qapi/job.json
> +++ b/qapi/job.json
> @@ -22,10 +22,17 @@
> #
> # @amend: image options amend job type, see "x-blockdev-amend" (since 5.1)
> #
> +# @snapshot-load: snapshot load job type, see "loadvm" (since 5.2)
Do you mean 'see command @snapshot-load?
> +#
> +# @snapshot-save: snapshot save job type, see "savevm" (since 5.2)
@snapshot-save?
> +#
> +# @snapshot-delete: snapshot delete job type, see "delvm" (since 5.2)
@snapshot-delete?
> +#
> # Since: 1.7
> ##
> { 'enum': 'JobType',
> - 'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend'] }
> + 'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend',
> + 'snapshot-load', 'snapshot-save', 'snapshot-delete'] }
>
> ##
> # @JobStatus:
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 5f6b06172c..d70f627b77 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -1720,3 +1720,138 @@
> ##
> { 'event': 'UNPLUG_PRIMARY',
> 'data': { 'device-id': 'str' } }
> +
> +##
> +# @snapshot-save:
> +#
> +# Save a VM snapshot
> +#
> +# @job-id: identifier for the newly created job
> +# @tag: name of the snapshot to create. If it already
> +# exists it will be replaced.
Sounds a bit dangerous. Require a force flag for such an overwrite?
Not sure.
> +# @devices: list of block device node names to save a snapshot to
> +# @vmstate: block device node name to save vmstate to
Worth mentioning that omitting writable block devices is probably a bad
idea?
> +#
> +# Applications should not assume that the snapshot save is complete
> +# when this command returns.
Is it complete then with the current code? I'm asking because such
properties have a way to sneakily become de facto ABI. We may not be
able to do anything about that now, other than documenting "don't do
that" like you did, but I'd like to understand the state of affairs all
the same.
> +# Completion is indicated by the job
> +# status. Clients can wait for the JOB_STATUS_CHANGE event. If the
> +# job aborts, errors can be obtained via the 'query-jobs' command,
> +# though.
Sure we want to these job basics here?
> +# Note that at this time most vmstate procssing errors only
Typo: processing
Whatever a "vmstate processing error" is...
> +# get printed to stderr. This limitation will be fixed at a future
> +# date.
Is that a promise? ;)
> +#
> +# Note that the VM CPUs will be paused during the time it takes to
> +# save the snapshot
End the sentence with a period, please.
> +#
> +# If @devices is not specified, or is an empty list, then the
> +# historical default logic for picking devices will be used.
Why is this useful for QMP?
> +#
> +# If @vmstate is not specified, then the first valid block
> +# device will be used for vmstate.
Why is this useful for QMP?
> +#
> +# Returns: nothing
> +#
> +# Example:
> +#
> +# -> { "execute": "snapshot-save",
> +# "data": {
> +# "job-id": "snapsave0",
> +# "tag": "my-snap",
> +# "vmstate": "disk0",
> +# "devices": ["disk0", "disk1"]
> +# }
> +# }
> +# <- { "return": { } }
> +#
> +# Since: 5.2
> +##
> +{ 'command': 'snapshot-save',
> + 'data': { 'job-id': 'str',
> + 'tag': 'str',
> + '*vmstate': 'str',
> + '*devices': ['str'] } }
> +
> +##
> +# @snapshot-load:
> +#
> +# Load a VM snapshot
> +#
> +# @job-id: identifier for the newly created job
> +# @tag: name of the snapshot to load.
> +# @devices: list of block device node names to load a snapshot from
> +# @vmstate: block device node name to load vmstate from
Worth mentioning that omitting block devices that may have changed since
the save is probably a bad idea?
> +#
> +# Applications should not assume that the snapshot load is complete
> +# when this command returns. Completion is indicated by the job
> +# status. Clients can wait for the JOB_STATUS_CHANGE event. If the
> +# job aborts, errors can be obtained via the 'query-jobs' command,
> +# though. Note that at this time most vmstate procssing errors only
> +# get printed to stderr. This limitation will be fixed at a future
> +# date.
Comments on snapshot-load apply.
> +#
> +# If @devices is not specified, or is an empty list, then the
> +# historical default logic for picking devices will be used.
Why is this useful for QMP?
> +#
> +# If @vmstate is not specified, then the first valid block
> +# device will be used for vmstate.
Why is this useful for QMP?
A more useful default could be "if exactly one the block devices being
restored contains a vmstate, use that".
> +#
> +# Returns: nothing
> +#
> +# Example:
> +#
> +# -> { "execute": "snapshot-load",
> +# "data": {
> +# "job-id": "snapload0",
> +# "tag": "my-snap",
> +# "vmstate": "disk0",
> +# "devices": ["disk0", "disk1"]
> +# }
> +# }
> +# <- { "return": { } }
> +#
> +# Since: 5.2
> +##
> +{ 'command': 'snapshot-load',
> + 'data': { 'job-id': 'str',
> + 'tag': 'str',
> + '*vmstate': 'str',
> + '*devices': ['str'] } }
> +
> +##
> +# @snapshot-delete:
> +#
> +# Delete a VM snapshot
> +#
> +# @job-id: identifier for the newly created job
> +# @tag: name of the snapshot to delete.
> +# @devices: list of block device node names to delete a snapshot from
> +#
> +# Applications should not assume that the snapshot load is complete
> +# when this command returns. Completion is indicated by the job
> +# status. Clients can wait for the JOB_STATUS_CHANGE event.
Comments on snapshot-load apply.
One difference: no "If the job aborts, ..." Intentional?
> +#
> +# Note that the VM CPUs will be paused during the time it takes to
> +# delete the snapshot
> +#
> +# If @devices is not specified, or is an empty list, then the
> +# historical default logic for picking devices will be used.
Why is this useful for QMP?
> +#
> +# Returns: nothing
> +#
> +# Example:
> +#
> +# -> { "execute": "snapshot-delete",
> +# "data": {
> +# "job-id": "snapdelete0",
> +# "tag": "my-snap",
> +# "devices": ["disk0", "disk1"]
> +# }
> +# }
> +# <- { "return": { } }
> +#
> +# Since: 5.2
> +##
> +{ 'command': 'snapshot-delete',
> + 'data': { 'job-id': 'str',
> + 'tag': 'str',
> + '*devices': ['str'] } }
[...]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 7/7] migration: introduce snapshot-{save, load, delete} QMP commands
2020-09-01 14:20 ` Markus Armbruster
@ 2020-09-01 16:47 ` Daniel P. Berrangé
2020-09-02 9:27 ` Markus Armbruster
0 siblings, 1 reply; 19+ messages in thread
From: Daniel P. Berrangé @ 2020-09-01 16:47 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Peter Krempa, Denis V. Lunev, qemu-block,
Juan Quintela, qemu-devel, Dr. David Alan Gilbert,
Pavel Dovgalyuk, Paolo Bonzini, Max Reitz, John Snow
On Tue, Sep 01, 2020 at 04:20:47PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > savevm, loadvm and delvm are some of the few HMP commands that have never
> > been converted to use QMP. The primary reason for this lack of conversion
> > is that they block execution of the thread for as long as they run.
>
> Nope. The primary reason is that the HMP interface is bonkers.
I don't think that's very helpful description. The HMP interface has
some limitations, but it isn't bonkers - it just doesn't cope with
all the use cases we want. Many people use it succesfully without
issue
> > Despite this downside, however, libvirt and applications using libvirt
> > have used these commands for as long as QMP has existed, via the
> > "human-monitor-command" passthrough command. IOW, while it is clearly
> > desirable to be able to fix the blocking problem, this is not an
> > immediate obstacle to real world usage.
> >
> > Meanwhile there is a need for other features which involve adding new
> > parameters to the commands. This is possible with HMP passthrough, but
> > it provides no reliable way for apps to introspect features, so using
> > QAPI modelling is highly desirable.
> >
> > This patch thus introduces new snapshot-{load,save,delete} commands to
> > QMP that are intended to replace the old HMP counterparts. The new
> > commands are given different names, because they will be using the new
> > QEMU job framework and thus will have diverging behaviour from the HMP
> > originals. It would thus be misleading to keep the same name.
> >
> > While this design uses the generic job framework, the current impl is
> > still blocking. The intention that the blocking problem is fixed later.
> > None the less applications using these new commands should assume that
> > they are asynchronous and thus wait for the job status change event to
> > indicate completion.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> [...]
> > diff --git a/qapi/job.json b/qapi/job.json
> > index 280c2f76f1..51bee470f0 100644
> > --- a/qapi/job.json
> > +++ b/qapi/job.json
> > @@ -22,10 +22,17 @@
> > #
> > # @amend: image options amend job type, see "x-blockdev-amend" (since 5.1)
> > #
> > +# @snapshot-load: snapshot load job type, see "loadvm" (since 5.2)
>
> Do you mean 'see command @snapshot-load?
Yes, I guess so.
>
> > +#
> > +# @snapshot-save: snapshot save job type, see "savevm" (since 5.2)
>
> @snapshot-save?
>
> > +#
> > +# @snapshot-delete: snapshot delete job type, see "delvm" (since 5.2)
>
> @snapshot-delete?
>
> > +#
> > # Since: 1.7
> > ##
> > { 'enum': 'JobType',
> > - 'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend'] }
> > + 'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend',
> > + 'snapshot-load', 'snapshot-save', 'snapshot-delete'] }
> >
> > ##
> > # @JobStatus:
> > diff --git a/qapi/migration.json b/qapi/migration.json
> > index 5f6b06172c..d70f627b77 100644
> > --- a/qapi/migration.json
> > +++ b/qapi/migration.json
> > @@ -1720,3 +1720,138 @@
> > ##
> > { 'event': 'UNPLUG_PRIMARY',
> > 'data': { 'device-id': 'str' } }
> > +
> > +##
> > +# @snapshot-save:
> > +#
> > +# Save a VM snapshot
> > +#
> > +# @job-id: identifier for the newly created job
> > +# @tag: name of the snapshot to create. If it already
> > +# exists it will be replaced.
>
> Sounds a bit dangerous. Require a force flag for such an overwrite?
> Not sure.
Yes, replacing is quite likely to be a mistake.
"@force" could mean many things, so "replace-existing: bool" is
probably a clearer name.
>
> > +# @devices: list of block device node names to save a snapshot to
> > +# @vmstate: block device node name to save vmstate to
>
> Worth mentioning that omitting writable block devices is probably a bad
> idea?
Sure
> > +#
> > +# Applications should not assume that the snapshot save is complete
> > +# when this command returns.
>
> Is it complete then with the current code? I'm asking because such
> properties have a way to sneakily become de facto ABI. We may not be
> able to do anything about that now, other than documenting "don't do
> that" like you did, but I'd like to understand the state of affairs all
> the same.
Yes, the actual snapshot is synchronous with return of the command.
>
> > +# Completion is indicated by the job
> > +# status. Clients can wait for the JOB_STATUS_CHANGE event. If the
> > +# job aborts, errors can be obtained via the 'query-jobs' command,
> > +# though.
>
> Sure we want to these job basics here?
This ties in with the previous point. If feel if we don't document
the use of events here, then people are likely to blindly assume
synchronous completion. By explicitly telling them to wait for the
JOB_STATUS_CHANGE they are nudged towards a correct solution that
won't break if it becomes async later.
>
> > +# Note that at this time most vmstate procssing errors only
>
> Typo: processing
>
> Whatever a "vmstate processing error" is...
>
> > +# get printed to stderr. This limitation will be fixed at a future
> > +# date.
>
> Is that a promise? ;)
I don't know when I'll have time, as I've not looked at just how
complex the conversion is. It is *highly* desirable to fix this
otherwise debugging failures is an exercise in extreme pain through
lack of useful information.
>
> > +#
> > +# Note that the VM CPUs will be paused during the time it takes to
> > +# save the snapshot
>
> End the sentence with a period, please.
>
> > +#
> > +# If @devices is not specified, or is an empty list, then the
> > +# historical default logic for picking devices will be used.
>
> Why is this useful for QMP?
>
> > +#
> > +# If @vmstate is not specified, then the first valid block
> > +# device will be used for vmstate.
>
> Why is this useful for QMP?
Both of these makes QEMU just "do the right thing" with the majority
of QEMU guest configurations with no special knowledge needed by
the mgmt app.
It makes it possible for all existing apps to immediately stop using
the loadvm/savevm commands via HMP passthrough, and convert to the
QMP commands.
Without this, applications will need to first convert to use -blockdev
before they can use the load-snapshot/save-snapshot commands, because
the devices are specified exclusively using blockdev node names, not
the legacy drive IDs. I didn't want to make blockdev a mandatory
dependancy unless apps want to opt-in to the fine grained control
over disk choices
> > +##
> > +# @snapshot-load:
> > +#
> > +# Load a VM snapshot
> > +#
> > +# @job-id: identifier for the newly created job
> > +# @tag: name of the snapshot to load.
> > +# @devices: list of block device node names to load a snapshot from
> > +# @vmstate: block device node name to load vmstate from
>
> Worth mentioning that omitting block devices that may have changed since
> the save is probably a bad idea?
Yep.
>
> > +#
> > +# Applications should not assume that the snapshot load is complete
> > +# when this command returns. Completion is indicated by the job
> > +# status. Clients can wait for the JOB_STATUS_CHANGE event. If the
> > +# job aborts, errors can be obtained via the 'query-jobs' command,
> > +# though. Note that at this time most vmstate procssing errors only
> > +# get printed to stderr. This limitation will be fixed at a future
> > +# date.
>
> Comments on snapshot-load apply.
>
> > +#
> > +# If @devices is not specified, or is an empty list, then the
> > +# historical default logic for picking devices will be used.
>
> Why is this useful for QMP?
>
> > +#
> > +# If @vmstate is not specified, then the first valid block
> > +# device will be used for vmstate.
>
> Why is this useful for QMP?
>
> A more useful default could be "if exactly one the block devices being
> restored contains a vmstate, use that".
I feel it is more important to be symetric with save-snapshot. ie if you
supply or omit the same args for save-snapshot and load-snapshot, you
know both will work, or neither will work. You dont get into a situation
where you can succesfully save the snapshot, but not restore it.
> > +##
> > +# @snapshot-delete:
> > +#
> > +# Delete a VM snapshot
> > +#
> > +# @job-id: identifier for the newly created job
> > +# @tag: name of the snapshot to delete.
> > +# @devices: list of block device node names to delete a snapshot from
> > +#
> > +# Applications should not assume that the snapshot load is complete
> > +# when this command returns. Completion is indicated by the job
> > +# status. Clients can wait for the JOB_STATUS_CHANGE event.
>
> Comments on snapshot-load apply.
>
> One difference: no "If the job aborts, ..." Intentional?
I guess it can abort if the file is corrupt perhaps. Generally
thogh if the named snapshot doesnt exist in the block device, it
is considered success, not an error.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 7/7] migration: introduce snapshot-{save, load, delete} QMP commands
2020-09-01 16:47 ` Daniel P. Berrangé
@ 2020-09-02 9:27 ` Markus Armbruster
2020-09-02 11:05 ` Daniel P. Berrangé
0 siblings, 1 reply; 19+ messages in thread
From: Markus Armbruster @ 2020-09-02 9:27 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Kevin Wolf, Peter Krempa, Denis V. Lunev, qemu-block,
Juan Quintela, qemu-devel, Dr. David Alan Gilbert,
Pavel Dovgalyuk, Paolo Bonzini, Max Reitz, John Snow
Daniel P. Berrangé <berrange@redhat.com> writes:
> On Tue, Sep 01, 2020 at 04:20:47PM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>> > savevm, loadvm and delvm are some of the few HMP commands that have never
>> > been converted to use QMP. The primary reason for this lack of conversion
>> > is that they block execution of the thread for as long as they run.
>>
>> Nope. The primary reason is that the HMP interface is bonkers.
>
> I don't think that's very helpful description. The HMP interface has
> some limitations, but it isn't bonkers - it just doesn't cope with
> all the use cases we want. Many people use it succesfully without
> issue
It's non-bonkers for the case it was designed for: one disk backed by
QCOW2, plus maybe a CD-ROM. The user is responsible for ensuring the
CD-ROM's media at loadvm time matches the one at savevm time. The user
is further responsible for ensuring the guest-visible configuration
matches the one at savevm time. No worse than migration.
It becomes useless as soon as you have writable non-QCOW2 block devices.
"Stop machine, take external snapshots, savevm, restart machine" should
work, but doesn't.
It becomes bonkers as soon as you have more than one QCOW2: which one
receives the system state? It depends on the order in which they got
configured or some craziness like that. Undocumented of course.
But "bonkers" is opinion, and we don't have to agree on opinions, only
on facts.
Fact: I did not reject attempts to bring savevm & friends to QMP because
they block. I'm not rejecting your attempt because they block.
Fact: I did reject attempts because I considered their QMP interface
bonkers. I do not consider your proposed interface bonkers (some
defaults I find dubious, but I'm sure we'll figure out what to do
there).
Therefore, the commit message's claim "the primary reason for this lack
of conversion is that they block execution of the thread for as long as
they run" is factually wrong.
If you'd rather not write "because they are bonkers", that's fine; I'm
confident we can find phrasing you like that is also correct.
>> > Despite this downside, however, libvirt and applications using libvirt
>> > have used these commands for as long as QMP has existed, via the
>> > "human-monitor-command" passthrough command. IOW, while it is clearly
>> > desirable to be able to fix the blocking problem, this is not an
>> > immediate obstacle to real world usage.
>> >
>> > Meanwhile there is a need for other features which involve adding new
>> > parameters to the commands. This is possible with HMP passthrough, but
>> > it provides no reliable way for apps to introspect features, so using
>> > QAPI modelling is highly desirable.
>> >
>> > This patch thus introduces new snapshot-{load,save,delete} commands to
>> > QMP that are intended to replace the old HMP counterparts. The new
>> > commands are given different names, because they will be using the new
>> > QEMU job framework and thus will have diverging behaviour from the HMP
>> > originals. It would thus be misleading to keep the same name.
>> >
>> > While this design uses the generic job framework, the current impl is
>> > still blocking. The intention that the blocking problem is fixed later.
>> > None the less applications using these new commands should assume that
>> > they are asynchronous and thus wait for the job status change event to
>> > indicate completion.
>> >
>> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> [...]
>> > diff --git a/qapi/job.json b/qapi/job.json
>> > index 280c2f76f1..51bee470f0 100644
>> > --- a/qapi/job.json
>> > +++ b/qapi/job.json
>> > @@ -22,10 +22,17 @@
>> > #
>> > # @amend: image options amend job type, see "x-blockdev-amend" (since 5.1)
>> > #
>> > +# @snapshot-load: snapshot load job type, see "loadvm" (since 5.2)
>>
>> Do you mean 'see command @snapshot-load?
>
> Yes, I guess so.
Please write it that way then.
>> > +#
>> > +# @snapshot-save: snapshot save job type, see "savevm" (since 5.2)
>>
>> @snapshot-save?
>>
>> > +#
>> > +# @snapshot-delete: snapshot delete job type, see "delvm" (since 5.2)
>>
>> @snapshot-delete?
>>
>> > +#
>> > # Since: 1.7
>> > ##
>> > { 'enum': 'JobType',
>> > - 'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend'] }
>> > + 'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend',
>> > + 'snapshot-load', 'snapshot-save', 'snapshot-delete'] }
>> >
>> > ##
>> > # @JobStatus:
>> > diff --git a/qapi/migration.json b/qapi/migration.json
>> > index 5f6b06172c..d70f627b77 100644
>> > --- a/qapi/migration.json
>> > +++ b/qapi/migration.json
>> > @@ -1720,3 +1720,138 @@
>> > ##
>> > { 'event': 'UNPLUG_PRIMARY',
>> > 'data': { 'device-id': 'str' } }
>> > +
>> > +##
>> > +# @snapshot-save:
>> > +#
>> > +# Save a VM snapshot
>> > +#
>> > +# @job-id: identifier for the newly created job
>> > +# @tag: name of the snapshot to create. If it already
>> > +# exists it will be replaced.
>>
>> Sounds a bit dangerous. Require a force flag for such an overwrite?
>> Not sure.
>
> Yes, replacing is quite likely to be a mistake.
>
> "@force" could mean many things, so "replace-existing: bool" is
> probably a clearer name.
Yes.
>> > +# @devices: list of block device node names to save a snapshot to
>> > +# @vmstate: block device node name to save vmstate to
>>
>> Worth mentioning that omitting writable block devices is probably a bad
>> idea?
>
> Sure
>
>> > +#
>> > +# Applications should not assume that the snapshot save is complete
>> > +# when this command returns.
>>
>> Is it complete then with the current code? I'm asking because such
>> properties have a way to sneakily become de facto ABI. We may not be
>> able to do anything about that now, other than documenting "don't do
>> that" like you did, but I'd like to understand the state of affairs all
>> the same.
>
> Yes, the actual snapshot is synchronous with return of the command.
Are there any failure modes where the command succeeds, and query-jobs
shows the error?
>
>>
>> > +# Completion is indicated by the job
>> > +# status. Clients can wait for the JOB_STATUS_CHANGE event. If the
>> > +# job aborts, errors can be obtained via the 'query-jobs' command,
>> > +# though.
>>
>> Sure we want to these job basics here?
>
> This ties in with the previous point. If feel if we don't document
> the use of events here, then people are likely to blindly assume
> synchronous completion. By explicitly telling them to wait for the
> JOB_STATUS_CHANGE they are nudged towards a correct solution that
> won't break if it becomes async later.
No objection.
>>
>> > +# Note that at this time most vmstate procssing errors only
>>
>> Typo: processing
>>
>> Whatever a "vmstate processing error" is...
>>
>> > +# get printed to stderr. This limitation will be fixed at a future
>> > +# date.
>>
>> Is that a promise? ;)
>
> I don't know when I'll have time, as I've not looked at just how
> complex the conversion is. It is *highly* desirable to fix this
> otherwise debugging failures is an exercise in extreme pain through
> lack of useful information.
>
>>
>> > +#
>> > +# Note that the VM CPUs will be paused during the time it takes to
>> > +# save the snapshot
>>
>> End the sentence with a period, please.
>>
>> > +#
>> > +# If @devices is not specified, or is an empty list, then the
>> > +# historical default logic for picking devices will be used.
>>
>> Why is this useful for QMP?
>>
>> > +#
>> > +# If @vmstate is not specified, then the first valid block
>> > +# device will be used for vmstate.
>>
>> Why is this useful for QMP?
>
> Both of these makes QEMU just "do the right thing" with the majority
> of QEMU guest configurations with no special knowledge needed by
> the mgmt app.
>
> It makes it possible for all existing apps to immediately stop using
> the loadvm/savevm commands via HMP passthrough, and convert to the
> QMP commands.
I appreciate your concern for easy migration from HMP savevm/loadvm to
QMP. I'm unwilling to permanently screw up the QMP interface for it,
though.
> Without this, applications will need to first convert to use -blockdev
> before they can use the load-snapshot/save-snapshot commands, because
> the devices are specified exclusively using blockdev node names, not
> the legacy drive IDs.
Not true. *Every* block node has a node name. If the user doesn't
specify one, the system makes one up. query-named-block-nodes shows
it. So does query-block.
> I didn't want to make blockdev a mandatory
> dependancy unless apps want to opt-in to the fine grained control
> over disk choices
I guess I could accept defaulting @devices to all capable ones. That's
what the "historical default logic for picking devices" does. The
documentation should be improved to actually explain what the default
does, though.
I object to defaulting @vmstate to "first valid block device". I don't
think requiring applications be explicit on where the vmstate needs to
go is too much of a burden. If you assure me it is, I'll reconsider.
>> > +##
>> > +# @snapshot-load:
>> > +#
>> > +# Load a VM snapshot
>> > +#
>> > +# @job-id: identifier for the newly created job
>> > +# @tag: name of the snapshot to load.
>> > +# @devices: list of block device node names to load a snapshot from
>> > +# @vmstate: block device node name to load vmstate from
>>
>> Worth mentioning that omitting block devices that may have changed since
>> the save is probably a bad idea?
>
> Yep.
>
>>
>> > +#
>> > +# Applications should not assume that the snapshot load is complete
>> > +# when this command returns. Completion is indicated by the job
>> > +# status. Clients can wait for the JOB_STATUS_CHANGE event. If the
>> > +# job aborts, errors can be obtained via the 'query-jobs' command,
>> > +# though. Note that at this time most vmstate procssing errors only
>> > +# get printed to stderr. This limitation will be fixed at a future
>> > +# date.
>>
>> Comments on snapshot-load apply.
>>
>> > +#
>> > +# If @devices is not specified, or is an empty list, then the
>> > +# historical default logic for picking devices will be used.
>>
>> Why is this useful for QMP?
>>
>> > +#
>> > +# If @vmstate is not specified, then the first valid block
>> > +# device will be used for vmstate.
>>
>> Why is this useful for QMP?
>>
>> A more useful default could be "if exactly one the block devices being
>> restored contains a vmstate, use that".
>
> I feel it is more important to be symetric with save-snapshot. ie if you
> supply or omit the same args for save-snapshot and load-snapshot, you
> know both will work, or neither will work. You dont get into a situation
> where you can succesfully save the snapshot, but not restore it.
For a value of "successful".
loadvm is unsafe unless guest-visible configuration and block device
contents match.
loadvm can make block device contents match for the block devices that
were snapshot by savevm. For any others, it's the user's
responsibility.
If you have the exact same set of QCOW2 as you had at savevm time, and
didn't mess them up meanwhile, then exactly one of them has the vmstate
named @tag. This is the sane way to use savem/loadvm.
But you can also use it in other ways, and some of them even work.
If you remove QCOW2s other than the one holding the vmstate from the
set, you still got exactly one vmstate. Aside: for safe loadvm, you
must replace these QCOW2s, and their replacements must provide the same
contents.
If you remove the one holding the vmstate, you got exactly no vmstate,
and loadvm won't work regardless of default. D'oh!
If you add QCOW2s that don't have a vmstate named @tag, you still got
exactly one vmstate. Aside: for safe loadvm, these guys must replace
non-QCOW2s, and they must provide the same contents. Guests are more
likely to survive sneak addition of hardware than sneak removal,
however.
If you add QCOW2s that have a vmstage named @tag, you got multiple
vmstates. This is the only situation where your lax "default to first
vmstate found, else error" makes loadvm "work" where "default to the
only vmstate if is exactly one, else error" doesn't.
In my opinion, having the system silently pick one of the multiple
vmstates named @tag is as dangerous as it is unhelpful. Even if you can
"control" the pick by ordering your command line and / or monitor
commands the right way.
>> > +##
>> > +# @snapshot-delete:
>> > +#
>> > +# Delete a VM snapshot
>> > +#
>> > +# @job-id: identifier for the newly created job
>> > +# @tag: name of the snapshot to delete.
>> > +# @devices: list of block device node names to delete a snapshot from
>> > +#
>> > +# Applications should not assume that the snapshot load is complete
>> > +# when this command returns. Completion is indicated by the job
>> > +# status. Clients can wait for the JOB_STATUS_CHANGE event.
>>
>> Comments on snapshot-load apply.
>>
>> One difference: no "If the job aborts, ..." Intentional?
>
> I guess it can abort if the file is corrupt perhaps. Generally
> thogh if the named snapshot doesnt exist in the block device, it
> is considered success, not an error.
I'd keep the "applications should" admonishments exactly the same for
all three commands. Suggestion, not demand.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 7/7] migration: introduce snapshot-{save, load, delete} QMP commands
2020-09-02 9:27 ` Markus Armbruster
@ 2020-09-02 11:05 ` Daniel P. Berrangé
2020-09-03 9:48 ` Markus Armbruster
0 siblings, 1 reply; 19+ messages in thread
From: Daniel P. Berrangé @ 2020-09-02 11:05 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Peter Krempa, Denis V. Lunev, qemu-block,
Juan Quintela, qemu-devel, Dr. David Alan Gilbert,
Pavel Dovgalyuk, Paolo Bonzini, Max Reitz, John Snow
On Wed, Sep 02, 2020 at 11:27:17AM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > On Tue, Sep 01, 2020 at 04:20:47PM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >>
> >> > savevm, loadvm and delvm are some of the few HMP commands that have never
> >> > been converted to use QMP. The primary reason for this lack of conversion
> >> > is that they block execution of the thread for as long as they run.
> >>
> >> Nope. The primary reason is that the HMP interface is bonkers.
> >
> > I don't think that's very helpful description. The HMP interface has
> > some limitations, but it isn't bonkers - it just doesn't cope with
> > all the use cases we want. Many people use it succesfully without
> > issue
>
> It's non-bonkers for the case it was designed for: one disk backed by
> QCOW2, plus maybe a CD-ROM. The user is responsible for ensuring the
> CD-ROM's media at loadvm time matches the one at savevm time. The user
> is further responsible for ensuring the guest-visible configuration
> matches the one at savevm time. No worse than migration.
It is fine for multiple writable disks too, if they're all qcow2 backed.
> It becomes useless as soon as you have writable non-QCOW2 block devices.
> "Stop machine, take external snapshots, savevm, restart machine" should
> work, but doesn't.
External snapshots can be made to work today if you're willing to pause
the VM CPUs while you take the external snapshots, and run the migration
to capture VM state. The main reason apps like Boxes / Virt-manager don't
use external snapshots is that they require more complicated decision
making. For each type of storage you're dealing with you have a potentially
different way to manage the external snapshot. Boxes took the view that it
would only support qcow2 to avoid making those decisions. virt-manager has
support for all types of storage and simply doesn't want to add the complex
logic to deal with non-qcow2.
> It becomes bonkers as soon as you have more than one QCOW2: which one
> receives the system state? It depends on the order in which they got
> configured or some craziness like that. Undocumented of course.
Saying it is the first configured disk was not that crazy. This has been
a pretty well defined semantic historically, as disks are usually added
to QEMU in a predictable order, because that in turn controls how QEMU
assigns addresses, and thus the order in which guest OS detect them.
The introduction of use of the block layer for UEFI storage really
threw a spanner in the works from QEMU's side. Even then the semantics
aren't bad from a app's POV - first disk is still well defined as a
concept, as UEFI vars isn't really considered a disk from an app POV.
> Therefore, the commit message's claim "the primary reason for this lack
> of conversion is that they block execution of the thread for as long as
> they run" is factually wrong.
That part of the commit message is referring to the original reason for
not porting loadvm/savevm when QMP was first created 10+ years ago, which
was primarily due to its blocking nature. The issues around disk selection
post-date that. I'll modify it to describe both issues
> >> > +#
> >> > +# Applications should not assume that the snapshot save is complete
> >> > +# when this command returns.
> >>
> >> Is it complete then with the current code? I'm asking because such
> >> properties have a way to sneakily become de facto ABI. We may not be
> >> able to do anything about that now, other than documenting "don't do
> >> that" like you did, but I'd like to understand the state of affairs all
> >> the same.
> >
> > Yes, the actual snapshot is synchronous with return of the command.
>
> Are there any failure modes where the command succeeds, and query-jobs
> shows the error?
As implemented in this series, the commands always succeed with the
errors only ever reported via query-jobs. So for error detection
you are already forced to use the job framework.
If you want to see some examples, take a look at the very last patch
in the series which introduces I/O test 310. The test covers various
success and failure scenarios, so you can see the behaviour we are
currently generating with this series.
> >> > +#
> >> > +# If @vmstate is not specified, then the first valid block
> >> > +# device will be used for vmstate.
> >>
> >> Why is this useful for QMP?
> >
> > Both of these makes QEMU just "do the right thing" with the majority
> > of QEMU guest configurations with no special knowledge needed by
> > the mgmt app.
> >
> > It makes it possible for all existing apps to immediately stop using
> > the loadvm/savevm commands via HMP passthrough, and convert to the
> > QMP commands.
>
> I appreciate your concern for easy migration from HMP savevm/loadvm to
> QMP. I'm unwilling to permanently screw up the QMP interface for it,
> though.
>
> > Without this, applications will need to first convert to use -blockdev
> > before they can use the load-snapshot/save-snapshot commands, because
> > the devices are specified exclusively using blockdev node names, not
> > the legacy drive IDs.
>
> Not true. *Every* block node has a node name. If the user doesn't
> specify one, the system makes one up. query-named-block-nodes shows
> it. So does query-block.
query-named-block-nodes isn't friendly as a way to lookup node names,
as it doesn't include the "id" value for the original -drive, so
correlating block nodes to drives is not straightforward. query-block
does seem a bit better in respect.
So if an app is currently using loadvm/savevm with human_monitor_command,
and is using -drive, they have to go through a mapping process to figure
out node names. Not especially friendly if they were happy with the
current choice of disks QEMU makes by default.
> > I didn't want to make blockdev a mandatory
> > dependancy unless apps want to opt-in to the fine grained control
> > over disk choices
>
> I guess I could accept defaulting @devices to all capable ones. That's
> what the "historical default logic for picking devices" does. The
> documentation should be improved to actually explain what the default
> does, though.
>
> I object to defaulting @vmstate to "first valid block device". I don't
> think requiring applications be explicit on where the vmstate needs to
> go is too much of a burden. If you assure me it is, I'll reconsider.
Both parameters have the same issue wrt needing to have the blockdev
node names. So once you have to specify one parameter, you might as
well specify both. IOW, either we make both optional with defaults, or
both mandatory with no default. I don't think it makes sense to mix.
> >> > +#
> >> > +# If @devices is not specified, or is an empty list, then the
> >> > +# historical default logic for picking devices will be used.
> >>
> >> Why is this useful for QMP?
> >>
> >> > +#
> >> > +# If @vmstate is not specified, then the first valid block
> >> > +# device will be used for vmstate.
> >>
> >> Why is this useful for QMP?
> >>
> >> A more useful default could be "if exactly one the block devices being
> >> restored contains a vmstate, use that".
> >
> > I feel it is more important to be symetric with save-snapshot. ie if you
> > supply or omit the same args for save-snapshot and load-snapshot, you
> > know both will work, or neither will work. You dont get into a situation
> > where you can succesfully save the snapshot, but not restore it.
>
> For a value of "successful".
>
> loadvm is unsafe unless guest-visible configuration and block device
> contents match.
>
> loadvm can make block device contents match for the block devices that
> were snapshot by savevm. For any others, it's the user's
> responsibility.
Yep, this is likely another thing that should be documented for the QMP
commands. The VM frontend config must be identical across load/save
otherwise VM state will fail to restore. DIsk backends should be the
same too unless you're specifying an explicit list of node names.
> If you have the exact same set of QCOW2 as you had at savevm time, and
> didn't mess them up meanwhile, then exactly one of them has the vmstate
> named @tag. This is the sane way to use savem/loadvm.
Yep.
> But you can also use it in other ways, and some of them even work.
>
> If you remove QCOW2s other than the one holding the vmstate from the
> set, you still got exactly one vmstate. Aside: for safe loadvm, you
> must replace these QCOW2s, and their replacements must provide the same
> contents.
>
> If you remove the one holding the vmstate, you got exactly no vmstate,
> and loadvm won't work regardless of default. D'oh!
>
> If you add QCOW2s that don't have a vmstate named @tag, you still got
> exactly one vmstate. Aside: for safe loadvm, these guys must replace
> non-QCOW2s, and they must provide the same contents. Guests are more
> likely to survive sneak addition of hardware than sneak removal,
> however.
>
> If you add QCOW2s that have a vmstage named @tag, you got multiple
> vmstates. This is the only situation where your lax "default to first
> vmstate found, else error" makes loadvm "work" where "default to the
> only vmstate if is exactly one, else error" doesn't.
>
> In my opinion, having the system silently pick one of the multiple
> vmstates named @tag is as dangerous as it is unhelpful. Even if you can
> "control" the pick by ordering your command line and / or monitor
> commands the right way.
I think it would be reasonable to make the existance of multiple
vmstates on different disks be a hard error both for QMP and for
the existing HMP commands, if not specifying an explicit device,
as its almostly certainly a admin/mgmt app screw up.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 7/7] migration: introduce snapshot-{save, load, delete} QMP commands
2020-09-02 11:05 ` Daniel P. Berrangé
@ 2020-09-03 9:48 ` Markus Armbruster
2020-09-03 10:17 ` Kevin Wolf
0 siblings, 1 reply; 19+ messages in thread
From: Markus Armbruster @ 2020-09-03 9:48 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Kevin Wolf, Peter Krempa, Denis V. Lunev, qemu-block,
Juan Quintela, qemu-devel, Dr. David Alan Gilbert,
Pavel Dovgalyuk, Paolo Bonzini, Max Reitz, John Snow
Daniel P. Berrangé <berrange@redhat.com> writes:
> On Wed, Sep 02, 2020 at 11:27:17AM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>> > On Tue, Sep 01, 2020 at 04:20:47PM +0200, Markus Armbruster wrote:
>> >> Daniel P. Berrangé <berrange@redhat.com> writes:
>> >>
>> >> > savevm, loadvm and delvm are some of the few HMP commands that have never
>> >> > been converted to use QMP. The primary reason for this lack of conversion
>> >> > is that they block execution of the thread for as long as they run.
>> >>
>> >> Nope. The primary reason is that the HMP interface is bonkers.
>> >
>> > I don't think that's very helpful description. The HMP interface has
>> > some limitations, but it isn't bonkers - it just doesn't cope with
>> > all the use cases we want. Many people use it succesfully without
>> > issue
>>
>> It's non-bonkers for the case it was designed for: one disk backed by
>> QCOW2, plus maybe a CD-ROM. The user is responsible for ensuring the
>> CD-ROM's media at loadvm time matches the one at savevm time. The user
>> is further responsible for ensuring the guest-visible configuration
>> matches the one at savevm time. No worse than migration.
>
> It is fine for multiple writable disks too, if they're all qcow2 backed.
>
>> It becomes useless as soon as you have writable non-QCOW2 block devices.
>> "Stop machine, take external snapshots, savevm, restart machine" should
>> work, but doesn't.
>
> External snapshots can be made to work today if you're willing to pause
> the VM CPUs while you take the external snapshots, and run the migration
> to capture VM state.
Yes.
Pausing is necessary because we don't have a way to take snapshots when
migration completes.
Even if we had it, live migration to file would be a problematic way to
capture VM state, because it doesn't update changing state in place.
savevm is not live either, of course.
My point is that savevm is "QCOW2 or bust" unnecessarily. Your proposed
QMP interface isn't.
> The main reason apps like Boxes / Virt-manager don't
> use external snapshots is that they require more complicated decision
> making. For each type of storage you're dealing with you have a potentially
> different way to manage the external snapshot. Boxes took the view that it
> would only support qcow2 to avoid making those decisions. virt-manager has
> support for all types of storage and simply doesn't want to add the complex
> logic to deal with non-qcow2.
>
>> It becomes bonkers as soon as you have more than one QCOW2: which one
>> receives the system state? It depends on the order in which they got
>> configured or some craziness like that. Undocumented of course.
>
> Saying it is the first configured disk was not that crazy. This has been
> a pretty well defined semantic historically, as disks are usually added
> to QEMU in a predictable order, because that in turn controls how QEMU
> assigns addresses, and thus the order in which guest OS detect them.
>
> The introduction of use of the block layer for UEFI storage really
> threw a spanner in the works from QEMU's side. Even then the semantics
> aren't bad from a app's POV - first disk is still well defined as a
> concept, as UEFI vars isn't really considered a disk from an app POV.
>
>> Therefore, the commit message's claim "the primary reason for this lack
>> of conversion is that they block execution of the thread for as long as
>> they run" is factually wrong.
>
> That part of the commit message is referring to the original reason for
> not porting loadvm/savevm when QMP was first created 10+ years ago, which
> was primarily due to its blocking nature. The issues around disk selection
> post-date that. I'll modify it to describe both issues
>
>
>> >> > +#
>> >> > +# Applications should not assume that the snapshot save is complete
>> >> > +# when this command returns.
>> >>
>> >> Is it complete then with the current code? I'm asking because such
>> >> properties have a way to sneakily become de facto ABI. We may not be
>> >> able to do anything about that now, other than documenting "don't do
>> >> that" like you did, but I'd like to understand the state of affairs all
>> >> the same.
>> >
>> > Yes, the actual snapshot is synchronous with return of the command.
>>
>> Are there any failure modes where the command succeeds, and query-jobs
>> shows the error?
>
> As implemented in this series, the commands always succeed with the
> errors only ever reported via query-jobs. So for error detection
> you are already forced to use the job framework.
This is what I hoped for when I asked. It makes misuse of the interface
unlikely to survive basic testing. Pretty much eliminates the risk of
misuse becoming de facto ABI over time.
> If you want to see some examples, take a look at the very last patch
> in the series which introduces I/O test 310. The test covers various
> success and failure scenarios, so you can see the behaviour we are
> currently generating with this series.
>
>
>> >> > +#
>> >> > +# If @vmstate is not specified, then the first valid block
>> >> > +# device will be used for vmstate.
>> >>
>> >> Why is this useful for QMP?
>> >
>> > Both of these makes QEMU just "do the right thing" with the majority
>> > of QEMU guest configurations with no special knowledge needed by
>> > the mgmt app.
>> >
>> > It makes it possible for all existing apps to immediately stop using
>> > the loadvm/savevm commands via HMP passthrough, and convert to the
>> > QMP commands.
>>
>> I appreciate your concern for easy migration from HMP savevm/loadvm to
>> QMP. I'm unwilling to permanently screw up the QMP interface for it,
>> though.
>>
>> > Without this, applications will need to first convert to use -blockdev
>> > before they can use the load-snapshot/save-snapshot commands, because
>> > the devices are specified exclusively using blockdev node names, not
>> > the legacy drive IDs.
>>
>> Not true. *Every* block node has a node name. If the user doesn't
>> specify one, the system makes one up. query-named-block-nodes shows
>> it. So does query-block.
>
> query-named-block-nodes isn't friendly as a way to lookup node names,
> as it doesn't include the "id" value for the original -drive, so
> correlating block nodes to drives is not straightforward. query-block
> does seem a bit better in respect.
>
> So if an app is currently using loadvm/savevm with human_monitor_command,
> and is using -drive, they have to go through a mapping process to figure
> out node names. Not especially friendly if they were happy with the
> current choice of disks QEMU makes by default.
I take "not especially friendly to clients converting from (bonkers) HMP
commands" over "similarly bonkers" any day. Okay, I'm exaggerating for
effect, your proposed interface is much better even with this default.
I do hate this default, though.
QMP is (meant to be) explicit and non-magic.
If having to map from drive ID to node-name really is too much of a
burden, we can look for ways to make it easier, or we can make savem
optionally accept drive IDs instead of node-names, like we do in several
other places for backward compatibility.
>> > I didn't want to make blockdev a mandatory
>> > dependancy unless apps want to opt-in to the fine grained control
>> > over disk choices
>>
>> I guess I could accept defaulting @devices to all capable ones. That's
>> what the "historical default logic for picking devices" does. The
Correction: to all writable ones.
>> documentation should be improved to actually explain what the default
>> does, though.
>>
>> I object to defaulting @vmstate to "first valid block device". I don't
>> think requiring applications be explicit on where the vmstate needs to
>> go is too much of a burden. If you assure me it is, I'll reconsider.
>
> Both parameters have the same issue wrt needing to have the blockdev
> node names. So once you have to specify one parameter, you might as
> well specify both. IOW, either we make both optional with defaults, or
> both mandatory with no default. I don't think it makes sense to mix.
>
>
>> >> > +#
>> >> > +# If @devices is not specified, or is an empty list, then the
>> >> > +# historical default logic for picking devices will be used.
>> >>
>> >> Why is this useful for QMP?
>> >>
>> >> > +#
>> >> > +# If @vmstate is not specified, then the first valid block
>> >> > +# device will be used for vmstate.
>> >>
>> >> Why is this useful for QMP?
>> >>
>> >> A more useful default could be "if exactly one the block devices being
>> >> restored contains a vmstate, use that".
>> >
>> > I feel it is more important to be symetric with save-snapshot. ie if you
>> > supply or omit the same args for save-snapshot and load-snapshot, you
>> > know both will work, or neither will work. You dont get into a situation
>> > where you can succesfully save the snapshot, but not restore it.
>>
>> For a value of "successful".
>>
>> loadvm is unsafe unless guest-visible configuration and block device
>> contents match.
>>
>> loadvm can make block device contents match for the block devices that
>> were snapshot by savevm. For any others, it's the user's
>> responsibility.
>
> Yep, this is likely another thing that should be documented for the QMP
> commands. The VM frontend config must be identical across load/save
> otherwise VM state will fail to restore. DIsk backends should be the
> same too unless you're specifying an explicit list of node names.
Yes, please.
>> If you have the exact same set of QCOW2 as you had at savevm time, and
>> didn't mess them up meanwhile, then exactly one of them has the vmstate
>> named @tag. This is the sane way to use savem/loadvm.
>
> Yep.
>
>> But you can also use it in other ways, and some of them even work.
>>
>> If you remove QCOW2s other than the one holding the vmstate from the
>> set, you still got exactly one vmstate. Aside: for safe loadvm, you
>> must replace these QCOW2s, and their replacements must provide the same
>> contents.
>>
>> If you remove the one holding the vmstate, you got exactly no vmstate,
>> and loadvm won't work regardless of default. D'oh!
>>
>> If you add QCOW2s that don't have a vmstate named @tag, you still got
>> exactly one vmstate. Aside: for safe loadvm, these guys must replace
>> non-QCOW2s, and they must provide the same contents. Guests are more
>> likely to survive sneak addition of hardware than sneak removal,
>> however.
>>
>> If you add QCOW2s that have a vmstage named @tag, you got multiple
>> vmstates. This is the only situation where your lax "default to first
>> vmstate found, else error" makes loadvm "work" where "default to the
>> only vmstate if is exactly one, else error" doesn't.
>>
>> In my opinion, having the system silently pick one of the multiple
>> vmstates named @tag is as dangerous as it is unhelpful. Even if you can
>> "control" the pick by ordering your command line and / or monitor
>> commands the right way.
>
> I think it would be reasonable to make the existance of multiple
> vmstates on different disks be a hard error both for QMP and for
> the existing HMP commands, if not specifying an explicit device,
> as its almostly certainly a admin/mgmt app screw up.
I consider this a no-brainer for QMP. If you somehow manage to get
yourself into a situation where you have multiple VM states, and need to
use one of them, you can ask explicitly for the one you want.
HMP doesn't let you ask. Hmm. I think "no go, resolve the conflict
with QMP or offline" is better than "pray there is a right one, and pray
QEMU picks it", which is what we have now.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 7/7] migration: introduce snapshot-{save, load, delete} QMP commands
2020-09-03 9:48 ` Markus Armbruster
@ 2020-09-03 10:17 ` Kevin Wolf
2020-09-03 10:21 ` Daniel P. Berrangé
0 siblings, 1 reply; 19+ messages in thread
From: Kevin Wolf @ 2020-09-03 10:17 UTC (permalink / raw)
To: Markus Armbruster
Cc: Peter Krempa, Daniel P. Berrangé,
Denis V. Lunev, qemu-block, Juan Quintela, qemu-devel,
Dr. David Alan Gilbert, Pavel Dovgalyuk, Paolo Bonzini,
Max Reitz, John Snow
Am 03.09.2020 um 11:48 hat Markus Armbruster geschrieben:
> Daniel P. Berrangé <berrange@redhat.com> writes:
>
> > On Wed, Sep 02, 2020 at 11:27:17AM +0200, Markus Armbruster wrote:
> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >>
> >> > On Tue, Sep 01, 2020 at 04:20:47PM +0200, Markus Armbruster wrote:
> >> >> Daniel P. Berrangé <berrange@redhat.com> writes:
> >> >>
> >> >> > savevm, loadvm and delvm are some of the few HMP commands that have never
> >> >> > been converted to use QMP. The primary reason for this lack of conversion
> >> >> > is that they block execution of the thread for as long as they run.
> >> >>
> >> >> Nope. The primary reason is that the HMP interface is bonkers.
> >> >
> >> > I don't think that's very helpful description. The HMP interface has
> >> > some limitations, but it isn't bonkers - it just doesn't cope with
> >> > all the use cases we want. Many people use it succesfully without
> >> > issue
> >>
> >> It's non-bonkers for the case it was designed for: one disk backed by
> >> QCOW2, plus maybe a CD-ROM. The user is responsible for ensuring the
> >> CD-ROM's media at loadvm time matches the one at savevm time. The user
> >> is further responsible for ensuring the guest-visible configuration
> >> matches the one at savevm time. No worse than migration.
> >
> > It is fine for multiple writable disks too, if they're all qcow2 backed.
> >
> >> It becomes useless as soon as you have writable non-QCOW2 block devices.
> >> "Stop machine, take external snapshots, savevm, restart machine" should
> >> work, but doesn't.
This is because savevm can't snapshot all writable disks and fails in
this case, right? So the proposed alternative with a list of nodes to
create disk snapshots for would solve this.
> > External snapshots can be made to work today if you're willing to pause
> > the VM CPUs while you take the external snapshots, and run the migration
> > to capture VM state.
>
> Yes.
>
> Pausing is necessary because we don't have a way to take snapshots when
> migration completes.
>
> Even if we had it, live migration to file would be a problematic way to
> capture VM state, because it doesn't update changing state in place.
It's also not the semantics you usally want for snapshots. You want to
take a snapshot of the state _now_, not of the state at some arbitrary
time in the future.
That's why I mentioned the post-copy semantics for live snapshots.
> savevm is not live either, of course.
>
> My point is that savevm is "QCOW2 or bust" unnecessarily. Your
> proposed QMP interface isn't.
Strictly speaking, not qcow2, but "block driver that supports
snapshots". Not that it matters in practice, but this happens to include
sheepdog.
It also means that we could implement a filter driver that saves the VM
state to an external file, which would allow saving the VM state even if
you use only raw images (of course, only useful if you have means to
snapshot the disk state externally or in combination with external
snapshots).
> > If you want to see some examples, take a look at the very last patch
> > in the series which introduces I/O test 310. The test covers various
> > success and failure scenarios, so you can see the behaviour we are
> > currently generating with this series.
> >
> >
> >> >> > +#
> >> >> > +# If @vmstate is not specified, then the first valid block
> >> >> > +# device will be used for vmstate.
> >> >>
> >> >> Why is this useful for QMP?
> >> >
> >> > Both of these makes QEMU just "do the right thing" with the majority
> >> > of QEMU guest configurations with no special knowledge needed by
> >> > the mgmt app.
> >> >
> >> > It makes it possible for all existing apps to immediately stop using
> >> > the loadvm/savevm commands via HMP passthrough, and convert to the
> >> > QMP commands.
> >>
> >> I appreciate your concern for easy migration from HMP savevm/loadvm to
> >> QMP. I'm unwilling to permanently screw up the QMP interface for it,
> >> though.
> >>
> >> > Without this, applications will need to first convert to use -blockdev
> >> > before they can use the load-snapshot/save-snapshot commands, because
> >> > the devices are specified exclusively using blockdev node names, not
> >> > the legacy drive IDs.
> >>
> >> Not true. *Every* block node has a node name. If the user doesn't
> >> specify one, the system makes one up. query-named-block-nodes shows
> >> it. So does query-block.
> >
> > query-named-block-nodes isn't friendly as a way to lookup node names,
> > as it doesn't include the "id" value for the original -drive, so
> > correlating block nodes to drives is not straightforward. query-block
> > does seem a bit better in respect.
> >
> > So if an app is currently using loadvm/savevm with human_monitor_command,
> > and is using -drive, they have to go through a mapping process to figure
> > out node names. Not especially friendly if they were happy with the
> > current choice of disks QEMU makes by default.
>
> I take "not especially friendly to clients converting from (bonkers) HMP
> commands" over "similarly bonkers" any day. Okay, I'm exaggerating for
> effect, your proposed interface is much better even with this default.
> I do hate this default, though.
>
> QMP is (meant to be) explicit and non-magic.
>
> If having to map from drive ID to node-name really is too much of a
> burden, we can look for ways to make it easier, or we can make savem
> optionally accept drive IDs instead of node-names, like we do in several
> other places for backward compatibility.
Yes, letting commands accept both node-names and drive IDs is trivial
and we do it pretty much everywhere. Much better than randomly selecting
an image to save the VM state to.
But I'm not even sure if makign people switch to -blockdev would be a
bad thing. If they insist on bad interfaces, they can keep using HMP
'savevm', too...
Kevin
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 7/7] migration: introduce snapshot-{save, load, delete} QMP commands
2020-09-03 10:17 ` Kevin Wolf
@ 2020-09-03 10:21 ` Daniel P. Berrangé
2020-09-03 10:44 ` Markus Armbruster
0 siblings, 1 reply; 19+ messages in thread
From: Daniel P. Berrangé @ 2020-09-03 10:21 UTC (permalink / raw)
To: Kevin Wolf
Cc: Peter Krempa, Denis V. Lunev, qemu-block, Juan Quintela,
qemu-devel, Markus Armbruster, Pavel Dovgalyuk, Paolo Bonzini,
Max Reitz, John Snow, Dr. David Alan Gilbert
On Thu, Sep 03, 2020 at 12:17:29PM +0200, Kevin Wolf wrote:
> Am 03.09.2020 um 11:48 hat Markus Armbruster geschrieben:
> > If having to map from drive ID to node-name really is too much of a
> > burden, we can look for ways to make it easier, or we can make savem
> > optionally accept drive IDs instead of node-names, like we do in several
> > other places for backward compatibility.
>
> Yes, letting commands accept both node-names and drive IDs is trivial
> and we do it pretty much everywhere. Much better than randomly selecting
> an image to save the VM state to.
Is there anything which guarantees that node-names and drive IDs will
never clash ? I didn't look for drive IDs as I was trying to ensure
a non-ambiguous lookup in case a string was both a valid node name
and a valid drive ID
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 7/7] migration: introduce snapshot-{save, load, delete} QMP commands
2020-09-03 10:21 ` Daniel P. Berrangé
@ 2020-09-03 10:44 ` Markus Armbruster
0 siblings, 0 replies; 19+ messages in thread
From: Markus Armbruster @ 2020-09-03 10:44 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Kevin Wolf, Peter Krempa, Denis V. Lunev, qemu-block,
Juan Quintela, qemu-devel, Max Reitz, Pavel Dovgalyuk,
Paolo Bonzini, John Snow, Dr. David Alan Gilbert
Daniel P. Berrangé <berrange@redhat.com> writes:
> On Thu, Sep 03, 2020 at 12:17:29PM +0200, Kevin Wolf wrote:
>> Am 03.09.2020 um 11:48 hat Markus Armbruster geschrieben:
>> > If having to map from drive ID to node-name really is too much of a
>> > burden, we can look for ways to make it easier, or we can make savem
>> > optionally accept drive IDs instead of node-names, like we do in several
>> > other places for backward compatibility.
>>
>> Yes, letting commands accept both node-names and drive IDs is trivial
>> and we do it pretty much everywhere. Much better than randomly selecting
>> an image to save the VM state to.
>
> Is there anything which guarantees that node-names and drive IDs will
> never clash ? I didn't look for drive IDs as I was trying to ensure
> a non-ambiguous lookup in case a string was both a valid node name
> and a valid drive ID
If I remember correctly, we always use a pair of members, technically
both optional, must specify exactly one, because a single string would
not work.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 0/7] migration: bring improved savevm/loadvm/delvm to QMP
2020-08-27 11:15 [PATCH v3 0/7] migration: bring improved savevm/loadvm/delvm to QMP Daniel P. Berrangé
` (6 preceding siblings ...)
2020-08-27 11:16 ` [PATCH v3 7/7] migration: introduce snapshot-{save, load, delete} QMP commands Daniel P. Berrangé
@ 2020-09-11 11:52 ` Dr. David Alan Gilbert
2020-09-11 16:45 ` Daniel P. Berrangé
7 siblings, 1 reply; 19+ messages in thread
From: Dr. David Alan Gilbert @ 2020-09-11 11:52 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Kevin Wolf, Peter Krempa, Denis V. Lunev, qemu-block,
Juan Quintela, John Snow, qemu-devel, Markus Armbruster,
Pavel Dovgalyuk, Paolo Bonzini, Max Reitz
Kevin:
While we're still arguing about details of the last commit; can we get
the first few commits in - they seem to be generally nice cleanups/error
handling.
Dave
* Daniel P. Berrangé (berrange@redhat.com) wrote:
> v1: https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg00866.html
> v2: https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg07523.html
>
> When QMP was first introduced some 10+ years ago now, the snapshot
> related commands (savevm/loadvm/delvm) were not converted. This was
> primarily because their implementation causes blocking of the thread
> running the monitor commands. This was (and still is) considered
> undesirable behaviour both in HMP and QMP.
>
> In theory someone was supposed to fix this flaw at some point in the
> past 10 years and bring them into the QMP world. Sadly, thus far it
> hasn't happened as people always had more important things to work
> on. Enterprise apps were much more interested in external snapshots
> than internal snapshots as they have many more features.
>
> Meanwhile users still want to use internal snapshots as there is
> a certainly simplicity in having everything self-contained in one
> image, even though it has limitations. Thus the apps that end up
> executing the savevm/loadvm/delvm via the "human-monitor-command"
> QMP command.
>
> IOW, the problematic blocking behaviour that was one of the reasons
> for not having savevm/loadvm/delvm in QMP is experienced by applications
> regardless. By not portting the commands to QMP due to one design flaw,
> we've forced apps and users to suffer from other design flaws of HMP (
> bad error reporting, strong type checking of args, no introspection) for
> an additional 10 years. This feels rather sub-optimal :-(
>
> In practice users don't appear to care strongly about the fact that these
> commands block the VM while they run. I might have seen one bug report
> about it, but it certainly isn't something that comes up as a frequent
> topic except among us QEMU maintainers. Users do care about having
> access to the snapshot feature.
>
> Where I am seeing frequent complaints is wrt the use of OVMF combined
> with snapshots which has some serious pain points. This is getting worse
> as the push to ditch legacy BIOS in favour of UEFI gain momentum both
> across OS vendors and mgmt apps. Solving it requires new parameters to
> the commands, but doing this in HMP is super unappealing.
>
> After 10 years, I think it is time for us to be a little pragmatic about
> our handling of snapshots commands. My desire is that libvirt should never
> use "human-monitor-command" under any circumstances, because of the
> inherant flaws in HMP as a protocol for machine consumption.
>
> Thus in this series I'm proposing a fairly direct mapping of the existing
> HMP commands for savevm/loadvm/delvm into QMP as a first step. This does
> not solve the blocking thread problem, but it does put in a place a
> design using the jobs framework which can facilitate solving it later.
> It does also solve the error reporting, type checking and introspection
> problems inherant to HMP. So we're winning on 3 out of the 4 problems,
> and pushed apps to a QMP design that will let us solve the last
> remaining problem.
>
> With a QMP variant, we reasonably deal with the problems related to OVMF:
>
> - The logic to pick which disk to store the vmstate in is not
> satsifactory.
>
> The first block driver state cannot be assumed to be the root disk
> image, it might be OVMF varstore and we don't want to store vmstate
> in there.
>
> - The logic to decide which disks must be snapshotted is hardwired
> to all disks which are writable
>
> Again with OVMF there might be a writable varstore, but this can be
> raw rather than qcow2 format, and thus unable to be snapshotted.
> While users might wish to snapshot their varstore, in some/many/most
> cases it is entirely uneccessary. Users are blocked from snapshotting
> their VM though due to this varstore.
>
> These are solved by adding two parameters to the commands. The first is
> a block device node name that identifies the image to store vmstate in,
> and the second is a list of node names to include for the snapshots.
> If the list of nodes isn't given, it falls back to the historical
> behaviour of using all disks matching some undocumented criteria.
>
> In the block code I've only dealt with node names for block devices, as
> IIUC, this is all that libvirt should need in the -blockdev world it now
> lives in. IOW, I've made not attempt to cope with people wanting to use
> these QMP commands in combination with -drive args, as libvirt will
> never use -drive with a QEMU new enough to have these new commands.
>
> The main limitations of this current impl
>
> - The snapshot process runs serialized in the main thread. ie QEMU
> guest execution is blocked for the duration. The job framework
> lets us fix this in future without changing the QMP semantics
> exposed to the apps.
>
> - Most vmstate loading errors just go to stderr, as they are not
> using Error **errp reporting. Thus the job framework just
> reports a fairly generic message
>
> "Error -22 while loading VM state"
>
> Again this can be fixed later without changing the QMP semantics
> exposed to apps.
>
> I've done some minimal work in libvirt to start to make use of the new
> commands to validate their functionality, but this isn't finished yet.
>
> My ultimate goal is to make the GNOME Boxes maintainer happy again by
> having internal snapshots work with OVMF:
>
> https://gitlab.gnome.org/GNOME/gnome-boxes/-/commit/c486da262f6566326fbcb5e=
> f45c5f64048f16a6e
>
> Changed in v3:
>
> - Schedule a bottom half to escape from coroutine context in
> the jobs. This is needed because the locking in the snapshot
> code goes horribly wrong when run from a background coroutine
> instead of the main event thread.
>
> - Re-factor way we iterate over devices, so that we correctly
> report non-existant devices passed by the user over QMP.
>
> - Add QAPI docs notes about limitations wrt vmstate error
> reporting (it all goes to stderr not an Error **errp)
> so QMP only gets a fairly generic error message currently.
>
> - Add I/O test to validate many usage scenarios / errors
>
> - Add I/O test helpers to handle QMP events with a deterministic
> ordering
>
> - Ensure 'delete-snapshot' reports an error if requesting
> delete from devices that don't support snapshot, instead of
> silently succeeding with no erro.
>
> Changed in v2:
>
> - Use new command names "snapshot-{load,save,delete}" to make it
> clear that these are different from the "savevm|loadvm|delvm"
> as they use the Job framework
>
> - Use an include list for block devs, not an exclude list
>
> Daniel P. Berrang=C3=A9 (7):
> migration: improve error reporting of block driver state name
> block: push error reporting into bdrv_all_*_snapshot functions
> migration: stop returning errno from load_snapshot()
> block: add ability to specify list of blockdevs during snapshot
> block: allow specifying name of block device for vmstate storage
> iotests: add support for capturing and matching QMP events
> migration: introduce snapshot-{save,load,delete} QMP commands
>
> block/monitor/block-hmp-cmds.c | 7 +-
> block/snapshot.c | 233 ++++++++++++++-------
> include/block/snapshot.h | 19 +-
> include/migration/snapshot.h | 10 +-
> migration/savevm.c | 258 +++++++++++++++++++----
> monitor/hmp-cmds.c | 11 +-
> qapi/job.json | 9 +-
> qapi/migration.json | 135 ++++++++++++
> replay/replay-snapshot.c | 4 +-
> softmmu/vl.c | 2 +-
> tests/qemu-iotests/267.out | 14 +-
> tests/qemu-iotests/310 | 255 +++++++++++++++++++++++
> tests/qemu-iotests/310.out | 369 +++++++++++++++++++++++++++++++++
> tests/qemu-iotests/common.qemu | 107 +++++++++-
> tests/qemu-iotests/group | 1 +
> 15 files changed, 1289 insertions(+), 145 deletions(-)
> create mode 100755 tests/qemu-iotests/310
> create mode 100644 tests/qemu-iotests/310.out
>
> --=20
> 2.26.2
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 0/7] migration: bring improved savevm/loadvm/delvm to QMP
2020-09-11 11:52 ` [PATCH v3 0/7] migration: bring improved savevm/loadvm/delvm to QMP Dr. David Alan Gilbert
@ 2020-09-11 16:45 ` Daniel P. Berrangé
0 siblings, 0 replies; 19+ messages in thread
From: Daniel P. Berrangé @ 2020-09-11 16:45 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Kevin Wolf, Peter Krempa, Denis V. Lunev, qemu-block,
Juan Quintela, qemu-devel, Markus Armbruster, Pavel Dovgalyuk,
Paolo Bonzini, Max Reitz, John Snow
On Fri, Sep 11, 2020 at 12:52:04PM +0100, Dr. David Alan Gilbert wrote:
> Kevin:
> While we're still arguing about details of the last commit; can we get
> the first few commits in - they seem to be generally nice cleanups/error
> handling.
I'm going to try to send a update next week which just cuts out
the features that are causing disagreements. Someone else can
worry about them later...
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2020-09-11 16:46 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-27 11:15 [PATCH v3 0/7] migration: bring improved savevm/loadvm/delvm to QMP Daniel P. Berrangé
2020-08-27 11:16 ` [PATCH v3 1/7] migration: improve error reporting of block driver state name Daniel P. Berrangé
2020-08-27 17:17 ` Dr. David Alan Gilbert
2020-08-27 11:16 ` [PATCH v3 2/7] block: push error reporting into bdrv_all_*_snapshot functions Daniel P. Berrangé
2020-08-27 11:16 ` [PATCH v3 3/7] migration: stop returning errno from load_snapshot() Daniel P. Berrangé
2020-08-27 11:16 ` [PATCH v3 4/7] block: add ability to specify list of blockdevs during snapshot Daniel P. Berrangé
2020-08-27 11:16 ` [PATCH v3 5/7] block: allow specifying name of block device for vmstate storage Daniel P. Berrangé
2020-08-27 11:16 ` [PATCH v3 6/7] iotests: add support for capturing and matching QMP events Daniel P. Berrangé
2020-08-27 11:16 ` [PATCH v3 7/7] migration: introduce snapshot-{save, load, delete} QMP commands Daniel P. Berrangé
2020-09-01 14:20 ` Markus Armbruster
2020-09-01 16:47 ` Daniel P. Berrangé
2020-09-02 9:27 ` Markus Armbruster
2020-09-02 11:05 ` Daniel P. Berrangé
2020-09-03 9:48 ` Markus Armbruster
2020-09-03 10:17 ` Kevin Wolf
2020-09-03 10:21 ` Daniel P. Berrangé
2020-09-03 10:44 ` Markus Armbruster
2020-09-11 11:52 ` [PATCH v3 0/7] migration: bring improved savevm/loadvm/delvm to QMP Dr. David Alan Gilbert
2020-09-11 16:45 ` Daniel P. Berrangé
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.