2:04 p.m.
John pointed out that patch 2 of my original bug fix did too
much in one patch. Now that we are in freeze for 5.1, I've
proposed two variants of the same fix: patch 1 is the bare
minimum to fix the bug and nothing else, while patches 3-7
are more in line with my v1 patch at doing other refactoring
work along the way (but now split into multiple logical steps)
that will prove useful for my other pending snapshot improvements
(https://www.redhat.com/archives/libvir-list/2019-February/msg01350.html,
but now 5.2 material).
I'm proposing both variants for comparison, although I already
suspect the answer will be 'use patch 1 for 5.1, then rebase
what remains of patches 3-7 into the other snapshot cleanups
for 5.2'.
variant2 is cleaner than my v1 patch 2/2 in that I no longer have
to use a default: label to hack around gcc's enum sanity checking
within a switch statement, but it required introducing a large
mechanical rename of all use of snapshot state values.
Eric Blake (7):
qemu: Fix snapshot redefine vs. domain state bug
Revert "qemu: Fix snapshot redefine vs. domain state bug"
qemu: Refactor check for _LIVE vs. _REDEFINE
qemu: Factor out qemuDomainSnapshotValidate() helper
snapshot: Rework virDomainSnapshotState enum
qemu: Use virDomainSnapshotState for switch statements
qemu: Fix snapshot redefine vs. domain state bug
src/conf/snapshot_conf.h | 21 ++++-
src/conf/snapshot_conf.c | 28 +++----
src/qemu/qemu_driver.c | 160 +++++++++++++++++++++++----------------
src/test/test_driver.c | 20 ++---
src/vbox/vbox_common.c | 4 +-
5 files changed, 137 insertions(+), 96 deletions(-)
--
2.20.1
2:04 p.m.
New subject: [libvirt] [PATCH variant1 v2 1/7] qemu: Fix snapshot redefine vs. domain state bug
The existing qemu snapshot code has a slight bug: if the domain
is currently pmsuspended, you can't use the _REDEFINE flag even
though the current domain state should have no bearing on being
able to recreate metadata state; and conversely, you can use the
_REDEFINE flag to create snapshot metadata claiming to be
pmsuspended as a bypass to the normal restrictions that you can't
create an original qemu snapshot in that state (the restriction
against pmsuspend is specific to qemu, rather than part of the
driver-agnostic snapshot_conf code).
Fix this by checking the snapshot state (when redefining) instead
of the domain state (which is a subset of snapshot states).
Fixes the second problem mentioned in https://bugzilla.redhat.com/1680304
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1d5b5f8653..36426cd65a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15693,6 +15693,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
virQEMUDriverConfigPtr cfg = NULL;
virCapsPtr caps = NULL;
qemuDomainObjPrivatePtr priv;
+ virDomainState state;
virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE |
VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT |
@@ -15776,7 +15777,11 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
}
/* allow snapshots only in certain states */
- switch ((virDomainState) vm->state.state) {
+ state = vm->state.state;
+ if (redefine)
+ state = def->state == VIR_DOMAIN_DISK_SNAPSHOT ? VIR_DOMAIN_SHUTOFF :
+ def->state;
+ switch (state) {
/* valid states */
case VIR_DOMAIN_RUNNING:
case VIR_DOMAIN_PAUSED:
@@ -15796,7 +15801,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
case VIR_DOMAIN_BLOCKED: /* invalid state, unused in qemu */
case VIR_DOMAIN_LAST:
virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state %s"),
- virDomainStateTypeToString(vm->state.state));
+ virDomainStateTypeToString(state));
goto cleanup;
}
--
2.20.1
2:04 p.m.
New subject: [libvirt] [PATCH v2 2/7] DO NOT MERGE: Revert "qemu: Fix snapshot redefine vs. domain state bug"
This reverts commit a6ecf44211ec62786dc1dc6ff30f1c7ea1a18f8b.
---
src/qemu/qemu_driver.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 36426cd65a..1d5b5f8653 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15693,7 +15693,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
virQEMUDriverConfigPtr cfg = NULL;
virCapsPtr caps = NULL;
qemuDomainObjPrivatePtr priv;
- virDomainState state;
virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE |
VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT |
@@ -15777,11 +15776,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
}
/* allow snapshots only in certain states */
- state = vm->state.state;
- if (redefine)
- state = def->state == VIR_DOMAIN_DISK_SNAPSHOT ? VIR_DOMAIN_SHUTOFF :
- def->state;
- switch (state) {
+ switch ((virDomainState) vm->state.state) {
/* valid states */
case VIR_DOMAIN_RUNNING:
case VIR_DOMAIN_PAUSED:
@@ -15801,7 +15796,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
case VIR_DOMAIN_BLOCKED: /* invalid state, unused in qemu */
case VIR_DOMAIN_LAST:
virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state %s"),
- virDomainStateTypeToString(state));
+ virDomainStateTypeToString(vm->state.state));
goto cleanup;
}
--
2.20.1
2:04 p.m.
New subject: [libvirt] [PATCH v2 3/7] qemu: Refactor check for _LIVE vs. _REDEFINE
The current qemu code rejects the combination of the two flags
VIR_DOMAIN_SNAPSHOT_CREATE_LIVE in tandem with
VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, but rather late in the cycle
(after the snapshot was already parsed), and with a rather confusing
message (complaining that live snapshots require external storage,
even if the redefined snapshot already declares external storage).
Hoist the rejection message to occur earlier (before parsing any
XML, which also aids upcoming patches that will implement bulk
redefine), and with a more typical error message about mutually
exclusive flags.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1d5b5f8653..4869096273 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15707,6 +15707,9 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
VIR_REQUIRE_FLAG_RET(VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE,
VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY,
NULL);
+ VIR_EXCLUSIVE_FLAGS_RET(VIR_DOMAIN_SNAPSHOT_CREATE_LIVE,
+ VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE,
+ NULL);
if ((redefine && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT)) ||
(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA))
@@ -15767,8 +15770,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
/* reject the VIR_DOMAIN_SNAPSHOT_CREATE_LIVE flag where not supported */
if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_LIVE &&
(!virDomainObjIsActive(vm) ||
- def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL ||
- redefine)) {
+ def->memory != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)) {
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("live snapshot creation is supported only "
"during full system snapshots"));
--
2.20.1
2:04 p.m.
New subject: [libvirt] [PATCH v2 4/7] qemu: Factor out qemuDomainSnapshotValidate() helper
Straight code motion, coupled with changing goto into return -1
as needed. This change will be important to later patches adding
bulk redefinition (where each snapshot in a list has to meet the
same constraints), but moving it out now also makes it easier
for a more immediate bug-fix related to which states are being
validated in the next patch.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 99 +++++++++++++++++++++++-------------------
1 file changed, 55 insertions(+), 44 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4869096273..a8ac9b59ee 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15673,6 +15673,59 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver,
}
+/* Validate that a snapshot object does not violate any qemu-specific
+ * constraints. */
+static int
+qemuDomainSnapshotValidate(virDomainSnapshotDefPtr def, int state,
+ unsigned int flags)
+{
+ /* reject snapshot names containing slashes or starting with dot as
+ * snapshot definitions are saved in files named by the snapshot name */
+ if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) {
+ if (strchr(def->name, '/')) {
+ virReportError(VIR_ERR_XML_DETAIL,
+ _("invalid snapshot name '%s': "
+ "name can't contain '/'"),
+ def->name);
+ return -1;
+ }
+
+ if (def->name[0] == '.') {
+ virReportError(VIR_ERR_XML_DETAIL,
+ _("invalid snapshot name '%s': "
+ "name can't start with '.'"),
+ def->name);
+ return -1;
+ }
+ }
+
+ /* allow snapshots only in certain states */
+ switch ((virDomainState) state) {
+ /* valid states */
+ case VIR_DOMAIN_RUNNING:
+ case VIR_DOMAIN_PAUSED:
+ case VIR_DOMAIN_SHUTDOWN:
+ case VIR_DOMAIN_SHUTOFF:
+ case VIR_DOMAIN_CRASHED:
+ break;
+
+ case VIR_DOMAIN_PMSUSPENDED:
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("qemu doesn't support taking snapshots of "
+ "PMSUSPENDED guests"));
+ return -1;
+
+ /* invalid states */
+ case VIR_DOMAIN_NOSTATE:
+ case VIR_DOMAIN_BLOCKED: /* invalid state, unused in qemu */
+ case VIR_DOMAIN_LAST:
+ virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state %s"),
+ virDomainStateTypeToString(state));
+ return -1;
+ }
+ return 0;
+}
+
static virDomainSnapshotPtr
qemuDomainSnapshotCreateXML(virDomainPtr domain,
const char *xmlDesc,
@@ -15747,25 +15800,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
parse_flags)))
goto cleanup;
- /* reject snapshot names containing slashes or starting with dot as
- * snapshot definitions are saved in files named by the snapshot name */
- if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) {
- if (strchr(def->name, '/')) {
- virReportError(VIR_ERR_XML_DETAIL,
- _("invalid snapshot name '%s': "
- "name can't contain '/'"),
- def->name);
- goto cleanup;
- }
-
- if (def->name[0] == '.') {
- virReportError(VIR_ERR_XML_DETAIL,
- _("invalid snapshot name '%s': "
- "name can't start with '.'"),
- def->name);
- goto cleanup;
- }
- }
+ if (qemuDomainSnapshotValidate(def, vm->state.state, flags) < 0)
+ goto cleanup;
/* reject the VIR_DOMAIN_SNAPSHOT_CREATE_LIVE flag where not supported */
if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_LIVE &&
@@ -15777,31 +15813,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
goto cleanup;
}
- /* allow snapshots only in certain states */
- switch ((virDomainState) vm->state.state) {
- /* valid states */
- case VIR_DOMAIN_RUNNING:
- case VIR_DOMAIN_PAUSED:
- case VIR_DOMAIN_SHUTDOWN:
- case VIR_DOMAIN_SHUTOFF:
- case VIR_DOMAIN_CRASHED:
- break;
-
- case VIR_DOMAIN_PMSUSPENDED:
- virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
- _("qemu doesn't support taking snapshots of "
- "PMSUSPENDED guests"));
- goto cleanup;
-
- /* invalid states */
- case VIR_DOMAIN_NOSTATE:
- case VIR_DOMAIN_BLOCKED: /* invalid state, unused in qemu */
- case VIR_DOMAIN_LAST:
- virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state %s"),
- virDomainStateTypeToString(vm->state.state));
- goto cleanup;
- }
-
/* We are going to modify the domain below. Internal snapshots would use
* a regular job, so we need to set the job mask to disallow query as
* 'savevm' blocks the monitor. External snapshot will then modify the
--
2.20.1
2:04 p.m.
New subject: [libvirt] [PATCH v2 5/7] snapshot: Rework virDomainSnapshotState enum
The existing virDomainSnapshotState is a superset of virDomainState,
adding one more state (disk-snapshot) on top of valid domain states.
But as written, the enum cannot be used for gcc validation that all
enum values are covered in a strongly-typed switch condition, because
the enum does not explicitly include the values it is adding to.
Copy the style used in qemu_blockjob.h of creating new enum names
for every inherited value, and update all clients to use the new
enum names anywhere snapshot state is referenced. Qemu code gains
a fixme comment about some odd casts (which will be cleaned up in
separate patches); the rest of the patch is mechanical.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/conf/snapshot_conf.h | 21 ++++++++++++++++++---
src/conf/snapshot_conf.c | 28 ++++++++++++++--------------
src/qemu/qemu_driver.c | 27 ++++++++++++++++-----------
src/test/test_driver.c | 20 ++++++++++----------
src/vbox/vbox_common.c | 4 ++--
5 files changed, 60 insertions(+), 40 deletions(-)
diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
index 7a175dfc96..9084f5fb8b 100644
--- a/src/conf/snapshot_conf.h
+++ b/src/conf/snapshot_conf.h
@@ -36,11 +36,26 @@ typedef enum {
VIR_DOMAIN_SNAPSHOT_LOCATION_LAST
} virDomainSnapshotLocation;
+/**
+ * This enum has to map all known domain states from the public enum
+ * virDomainState, before adding one additional state possible only
+ * for snapshots.
+ */
typedef enum {
- /* Inherit the VIR_DOMAIN_* states from virDomainState. */
- VIR_DOMAIN_DISK_SNAPSHOT = VIR_DOMAIN_LAST,
- VIR_DOMAIN_SNAPSHOT_STATE_LAST
+ /* Mapped to public enum */
+ VIR_SNAP_STATE_NOSTATE = VIR_DOMAIN_NOSTATE,
+ VIR_SNAP_STATE_RUNNING = VIR_DOMAIN_RUNNING,
+ VIR_SNAP_STATE_BLOCKED = VIR_DOMAIN_BLOCKED,
+ VIR_SNAP_STATE_PAUSED = VIR_DOMAIN_PAUSED,
+ VIR_SNAP_STATE_SHUTDOWN = VIR_DOMAIN_SHUTDOWN,
+ VIR_SNAP_STATE_SHUTOFF = VIR_DOMAIN_SHUTOFF,
+ VIR_SNAP_STATE_CRASHED = VIR_DOMAIN_CRASHED,
+ VIR_SNAP_STATE_PMSUSPENDED = VIR_DOMAIN_PMSUSPENDED,
+ /* Additional enum values local to qemu */
+ VIR_SNAP_STATE_DISK_SNAPSHOT,
+ VIR_SNAP_STATE_LAST
} virDomainSnapshotState;
+verify((int)VIR_SNAP_STATE_DISK_SNAPSHOT == VIR_DOMAIN_LAST);
/* Stores disk-snapshot information */
typedef struct _virDomainSnapshotDiskDef virDomainSnapshotDiskDef;
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 41236d9932..299fc2101b 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -58,7 +58,7 @@ VIR_ENUM_IMPL(virDomainSnapshotLocation,
VIR_DOMAIN_SNAPSHOT_LOCATION_LAST,
);
/* virDomainSnapshotState is really virDomainState plus one extra state */
-VIR_ENUM_IMPL(virDomainSnapshotState, VIR_DOMAIN_SNAPSHOT_STATE_LAST,
+VIR_ENUM_IMPL(virDomainSnapshotState, VIR_SNAP_STATE_LAST,
"nostate",
"running",
"blocked",
@@ -257,8 +257,8 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
state);
goto cleanup;
}
- offline = (def->state == VIR_DOMAIN_SHUTOFF ||
- def->state == VIR_DOMAIN_DISK_SNAPSHOT);
+ offline = (def->state == VIR_SNAP_STATE_SHUTOFF ||
+ def->state == VIR_SNAP_STATE_DISK_SNAPSHOT);
/* Older snapshots were created with just <domain>/<uuid>, and
* lack domain/@type. In that case, leave dom NULL, and
@@ -879,14 +879,14 @@ static int virDomainSnapshotObjListCopyNames(void *payload,
if (data->flags & VIR_DOMAIN_SNAPSHOT_FILTERS_STATUS) {
if (!(data->flags & VIR_DOMAIN_SNAPSHOT_LIST_INACTIVE) &&
- obj->def->state == VIR_DOMAIN_SHUTOFF)
+ obj->def->state == VIR_SNAP_STATE_SHUTOFF)
return 0;
if (!(data->flags & VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY) &&
- obj->def->state == VIR_DOMAIN_DISK_SNAPSHOT)
+ obj->def->state == VIR_SNAP_STATE_DISK_SNAPSHOT)
return 0;
if (!(data->flags & VIR_DOMAIN_SNAPSHOT_LIST_ACTIVE) &&
- obj->def->state != VIR_DOMAIN_SHUTOFF &&
- obj->def->state != VIR_DOMAIN_DISK_SNAPSHOT)
+ obj->def->state != VIR_SNAP_STATE_SHUTOFF &&
+ obj->def->state != VIR_SNAP_STATE_DISK_SNAPSHOT)
return 0;
}
@@ -1225,7 +1225,7 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
bool align_match = true;
virDomainSnapshotObjPtr other;
- bool external = def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
+ bool external = def->state == VIR_SNAP_STATE_DISK_SNAPSHOT ||
virDomainSnapshotDefIsExternal(def);
/* Prevent circular chains */
@@ -1282,10 +1282,10 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
other = virDomainSnapshotFindByName(vm->snapshots, def->name);
if (other) {
- if ((other->def->state == VIR_DOMAIN_RUNNING ||
- other->def->state == VIR_DOMAIN_PAUSED) !=
- (def->state == VIR_DOMAIN_RUNNING ||
- def->state == VIR_DOMAIN_PAUSED)) {
+ if ((other->def->state == VIR_SNAP_STATE_RUNNING ||
+ other->def->state == VIR_SNAP_STATE_PAUSED) !=
+ (def->state == VIR_SNAP_STATE_RUNNING ||
+ def->state == VIR_SNAP_STATE_PAUSED)) {
virReportError(VIR_ERR_INVALID_ARG,
_("cannot change between online and offline "
"snapshot state in snapshot %s"),
@@ -1293,8 +1293,8 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
goto cleanup;
}
- if ((other->def->state == VIR_DOMAIN_DISK_SNAPSHOT) !=
- (def->state == VIR_DOMAIN_DISK_SNAPSHOT)) {
+ if ((other->def->state == VIR_SNAP_STATE_DISK_SNAPSHOT) !=
+ (def->state == VIR_SNAP_STATE_DISK_SNAPSHOT)) {
virReportError(VIR_ERR_INVALID_ARG,
_("cannot change between disk only and "
"full system in snapshot %s"),
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a8ac9b59ee..34014ba9c7 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15022,7 +15022,7 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm,
case VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL:
found_internal = true;
- if (def->state == VIR_DOMAIN_DISK_SNAPSHOT && active) {
+ if (def->state == VIR_SNAP_STATE_DISK_SNAPSHOT && active) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("active qemu domains require external disk "
"snapshots; disk %s requested internal"),
@@ -15099,7 +15099,7 @@ qemuDomainSnapshotPrepare(virDomainObjPtr vm,
}
/* disk snapshot requires at least one disk */
- if (def->state == VIR_DOMAIN_DISK_SNAPSHOT && !external) {
+ if (def->state == VIR_SNAP_STATE_DISK_SNAPSHOT && !external) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("disk-only snapshots require at least "
"one disk to be selected for snapshot"));
@@ -15844,9 +15844,9 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
align_match = false;
if (virDomainObjIsActive(vm))
- def->state = VIR_DOMAIN_DISK_SNAPSHOT;
+ def->state = VIR_SNAP_STATE_DISK_SNAPSHOT;
else
- def->state = VIR_DOMAIN_SHUTOFF;
+ def->state = VIR_SNAP_STATE_SHUTOFF;
def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
} else if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
def->state = virDomainObjGetState(vm, NULL);
@@ -15863,7 +15863,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
goto endjob;
}
- def->memory = (def->state == VIR_DOMAIN_SHUTOFF ?
+ def->memory = (def->state == VIR_SNAP_STATE_SHUTOFF ?
VIR_DOMAIN_SNAPSHOT_LOCATION_NONE :
VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL);
}
@@ -16426,8 +16426,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
goto endjob;
if (!vm->persistent &&
- snap->def->state != VIR_DOMAIN_RUNNING &&
- snap->def->state != VIR_DOMAIN_PAUSED &&
+ snap->def->state != VIR_SNAP_STATE_RUNNING &&
+ snap->def->state != VIR_SNAP_STATE_PAUSED &&
(flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) == 0) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -16450,8 +16450,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
goto endjob;
}
if (virDomainObjIsActive(vm) &&
- !(snap->def->state == VIR_DOMAIN_RUNNING
- || snap->def->state == VIR_DOMAIN_PAUSED) &&
+ !(snap->def->state == VIR_SNAP_STATE_RUNNING ||
+ snap->def->state == VIR_SNAP_STATE_PAUSED) &&
(flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) {
virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s",
@@ -16487,6 +16487,11 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
cookie = (qemuDomainSaveCookiePtr) snap->def->cookie;
+ /* FIXME: This cast should be to virDomainSnapshotState, with
+ * better handling of VIR_SNAP_STATE_DISK_SNAPSHOT (the only enum
+ * value added beyond what virDomainState supports). But for now
+ * it doesn't matter, because of the above rejection of revert to
+ * external snapshots. */
switch ((virDomainState) snap->def->state) {
case VIR_DOMAIN_RUNNING:
case VIR_DOMAIN_PAUSED:
@@ -16628,7 +16633,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
/* Touch up domain state. */
if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) &&
- (snap->def->state == VIR_DOMAIN_PAUSED ||
+ (snap->def->state == VIR_SNAP_STATE_PAUSED ||
(flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) {
/* Transitions 3, 6, 9 */
virDomainObjSetState(vm, VIR_DOMAIN_PAUSED,
@@ -16735,7 +16740,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Invalid target domain state '%s'. Refusing "
"snapshot reversion"),
- virDomainStateTypeToString(snap->def->state));
+ virDomainSnapshotStateTypeToString(snap->def->state));
goto endjob;
}
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index ce0df1f8e3..4a6555e0ea 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -6271,9 +6271,9 @@ testDomainSnapshotAlignDisks(virDomainObjPtr vm,
align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
align_match = false;
if (virDomainObjIsActive(vm))
- def->state = VIR_DOMAIN_DISK_SNAPSHOT;
+ def->state = VIR_SNAP_STATE_DISK_SNAPSHOT;
else
- def->state = VIR_DOMAIN_SHUTOFF;
+ def->state = VIR_SNAP_STATE_SHUTOFF;
def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
} else if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
def->state = virDomainObjGetState(vm, NULL);
@@ -6281,7 +6281,7 @@ testDomainSnapshotAlignDisks(virDomainObjPtr vm,
align_match = false;
} else {
def->state = virDomainObjGetState(vm, NULL);
- def->memory = def->state == VIR_DOMAIN_SHUTOFF ?
+ def->memory = def->state == VIR_SNAP_STATE_SHUTOFF ?
VIR_DOMAIN_SNAPSHOT_LOCATION_NONE :
VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
}
@@ -6572,8 +6572,8 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
goto cleanup;
if (!vm->persistent &&
- snap->def->state != VIR_DOMAIN_RUNNING &&
- snap->def->state != VIR_DOMAIN_PAUSED &&
+ snap->def->state != VIR_SNAP_STATE_RUNNING &&
+ snap->def->state != VIR_SNAP_STATE_PAUSED &&
(flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) == 0) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
@@ -6590,8 +6590,8 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
goto cleanup;
}
if (virDomainObjIsActive(vm) &&
- !(snap->def->state == VIR_DOMAIN_RUNNING
- || snap->def->state == VIR_DOMAIN_PAUSED) &&
+ !(snap->def->state == VIR_SNAP_STATE_RUNNING ||
+ snap->def->state == VIR_SNAP_STATE_PAUSED) &&
(flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) {
virReportError(VIR_ERR_SNAPSHOT_REVERT_RISKY, "%s",
@@ -6612,8 +6612,8 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
if (!config)
goto cleanup;
- if (snap->def->state == VIR_DOMAIN_RUNNING ||
- snap->def->state == VIR_DOMAIN_PAUSED) {
+ if (snap->def->state == VIR_SNAP_STATE_RUNNING ||
+ snap->def->state == VIR_SNAP_STATE_PAUSED) {
/* Transitions 2, 3, 5, 6, 8, 9 */
bool was_running = false;
bool was_stopped = false;
@@ -6672,7 +6672,7 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
/* Touch up domain state. */
if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING) &&
- (snap->def->state == VIR_DOMAIN_PAUSED ||
+ (snap->def->state == VIR_SNAP_STATE_PAUSED ||
(flags & VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED))) {
/* Transitions 3, 6, 9 */
virDomainObjSetState(vm, VIR_DOMAIN_PAUSED,
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index d95fe7c7ae..407c932019 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -6314,9 +6314,9 @@ static char *vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr
snapshot,
goto cleanup;
}
if (online)
- def->state = VIR_DOMAIN_RUNNING;
+ def->state = VIR_SNAP_STATE_RUNNING;
else
- def->state = VIR_DOMAIN_SHUTOFF;
+ def->state = VIR_SNAP_STATE_SHUTOFF;
virUUIDFormat(dom->uuid, uuidstr);
memcpy(def->dom->uuid, dom->uuid, VIR_UUID_BUFLEN);
--
2.20.1
2:13 p.m.
New subject: [libvirt] [PATCH v2 5/7] snapshot: Rework virDomainSnapshotState enum
On 2/27/19 2:04 PM, Eric Blake wrote:
The existing virDomainSnapshotState is a superset of virDomainState,
adding one more state (disk-snapshot) on top of valid domain states.
But as written, the enum cannot be used for gcc validation that all
enum values are covered in a strongly-typed switch condition, because
the enum does not explicitly include the values it is adding to.
Copy the style used in qemu_blockjob.h of creating new enum names
for every inherited value, and update all clients to use the new
enum names anywhere snapshot state is referenced. Qemu code gains
a fixme comment about some odd casts (which will be cleaned up in
separate patches); the rest of the patch is mechanical.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
+/**
+ * This enum has to map all known domain states from the public enum
+ * virDomainState, before adding one additional state possible only
+ * for snapshots.
+ */
typedef enum {
- /* Inherit the VIR_DOMAIN_* states from virDomainState. */
- VIR_DOMAIN_DISK_SNAPSHOT = VIR_DOMAIN_LAST,
- VIR_DOMAIN_SNAPSHOT_STATE_LAST
+ /* Mapped to public enum */
+ VIR_SNAP_STATE_NOSTATE = VIR_DOMAIN_NOSTATE,
+ VIR_SNAP_STATE_RUNNING = VIR_DOMAIN_RUNNING,
+ VIR_SNAP_STATE_BLOCKED = VIR_DOMAIN_BLOCKED,
+ VIR_SNAP_STATE_PAUSED = VIR_DOMAIN_PAUSED,
+ VIR_SNAP_STATE_SHUTDOWN = VIR_DOMAIN_SHUTDOWN,
+ VIR_SNAP_STATE_SHUTOFF = VIR_DOMAIN_SHUTOFF,
+ VIR_SNAP_STATE_CRASHED = VIR_DOMAIN_CRASHED,
+ VIR_SNAP_STATE_PMSUSPENDED = VIR_DOMAIN_PMSUSPENDED,
+ /* Additional enum values local to qemu */
Too much copy-and-paste from qemu_blockjob.h; this comment should
mention 'local to snapshots'
+ VIR_SNAP_STATE_DISK_SNAPSHOT,
+ VIR_SNAP_STATE_LAST
} virDomainSnapshotState;
+verify((int)VIR_SNAP_STATE_DISK_SNAPSHOT == VIR_DOMAIN_LAST);
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
2:04 p.m.
New subject: [libvirt] [PATCH v2 6/7] qemu: Use virDomainSnapshotState for switch statements
Clean up the previous patch which abused a switch on virDomainState
when contrasted to a variable containing virDomainSnapshotState, by
converting the two affected switch statements to now use the right
enum. No semantic changes now (the caller to qemuDomainSnapshotValidate
passes in a domain state rather than a snapshot state, so the new
branch is currently unreachable [will change in next patch], while
qemuDomainRevertToSnapshot can't reach the new state because of the
earlier rejection of external images).
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 57 +++++++++++++++++++++++-------------------
1 file changed, 31 insertions(+), 26 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 34014ba9c7..06bc1893ad 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15700,27 +15700,35 @@ qemuDomainSnapshotValidate(virDomainSnapshotDefPtr def, int
state,
}
/* allow snapshots only in certain states */
- switch ((virDomainState) state) {
+ switch ((virDomainSnapshotState) state) {
/* valid states */
- case VIR_DOMAIN_RUNNING:
- case VIR_DOMAIN_PAUSED:
- case VIR_DOMAIN_SHUTDOWN:
- case VIR_DOMAIN_SHUTOFF:
- case VIR_DOMAIN_CRASHED:
+ case VIR_SNAP_STATE_RUNNING:
+ case VIR_SNAP_STATE_PAUSED:
+ case VIR_SNAP_STATE_SHUTDOWN:
+ case VIR_SNAP_STATE_SHUTOFF:
+ case VIR_SNAP_STATE_CRASHED:
break;
- case VIR_DOMAIN_PMSUSPENDED:
+ case VIR_SNAP_STATE_DISK_SNAPSHOT:
+ if (!(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE)) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state
%s"),
+ virDomainSnapshotStateTypeToString(state));
+ return -1;
+ }
+ break;
+
+ case VIR_SNAP_STATE_PMSUSPENDED:
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("qemu doesn't support taking snapshots of "
"PMSUSPENDED guests"));
return -1;
/* invalid states */
- case VIR_DOMAIN_NOSTATE:
- case VIR_DOMAIN_BLOCKED: /* invalid state, unused in qemu */
- case VIR_DOMAIN_LAST:
+ case VIR_SNAP_STATE_NOSTATE:
+ case VIR_SNAP_STATE_BLOCKED: /* invalid state, unused in qemu */
+ case VIR_SNAP_STATE_LAST:
virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid domain state %s"),
- virDomainStateTypeToString(state));
+ virDomainSnapshotStateTypeToString(state));
return -1;
}
return 0;
@@ -16487,14 +16495,9 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
cookie = (qemuDomainSaveCookiePtr) snap->def->cookie;
- /* FIXME: This cast should be to virDomainSnapshotState, with
- * better handling of VIR_SNAP_STATE_DISK_SNAPSHOT (the only enum
- * value added beyond what virDomainState supports). But for now
- * it doesn't matter, because of the above rejection of revert to
- * external snapshots. */
- switch ((virDomainState) snap->def->state) {
- case VIR_DOMAIN_RUNNING:
- case VIR_DOMAIN_PAUSED:
+ switch ((virDomainSnapshotState) snap->def->state) {
+ case VIR_SNAP_STATE_RUNNING:
+ case VIR_SNAP_STATE_PAUSED:
start_flags |= VIR_QEMU_PROCESS_START_PAUSED;
@@ -16669,9 +16672,9 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
}
break;
- case VIR_DOMAIN_SHUTDOWN:
- case VIR_DOMAIN_SHUTOFF:
- case VIR_DOMAIN_CRASHED:
+ case VIR_SNAP_STATE_SHUTDOWN:
+ case VIR_SNAP_STATE_SHUTOFF:
+ case VIR_SNAP_STATE_CRASHED:
/* Transitions 1, 4, 7 */
/* Newer qemu -loadvm refuses to revert to the state of a snapshot
* created by qemu-img snapshot -c. If the domain is running, we
@@ -16728,15 +16731,17 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
}
break;
- case VIR_DOMAIN_PMSUSPENDED:
+ case VIR_SNAP_STATE_PMSUSPENDED:
virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
_("qemu doesn't support reversion of snapshot taken in
"
"PMSUSPENDED state"));
goto endjob;
- case VIR_DOMAIN_NOSTATE:
- case VIR_DOMAIN_BLOCKED:
- case VIR_DOMAIN_LAST:
+ case VIR_SNAP_STATE_DISK_SNAPSHOT:
+ /* Rejected earlier as an external snapshot */
+ case VIR_SNAP_STATE_NOSTATE:
+ case VIR_SNAP_STATE_BLOCKED:
+ case VIR_SNAP_STATE_LAST:
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Invalid target domain state '%s'. Refusing "
"snapshot reversion"),
--
2.20.1
2:04 p.m.
New subject: [libvirt] [PATCH variant2 v2 7/7] qemu: Fix snapshot redefine vs. domain state bug
The existing qemu snapshot code has a slight bug: if the domain
is currently pmsuspended, you can't use the _REDEFINE flag even
though the current domain state should have no bearing on being
able to recreate metadata state; and conversely, you can use the
_REDEFINE flag to create snapshot metadata claiming to be
pmsuspended as a bypass to the normal restrictions that you can't
create an original qemu snapshot in that state (the restriction
against pmsuspend is specific to qemu, rather than part of the
driver-agnostic snapshot_conf code).
Fix this by checking the snapshot state (when redefining) instead
of the domain state (which is a subset of snapshot states).
Fixes the second problem mentioned in https://bugzilla.redhat.com/1680304
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/qemu/qemu_driver.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 06bc1893ad..18acdd9816 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15674,7 +15674,9 @@ qemuDomainSnapshotCreateActiveExternal(virQEMUDriverPtr driver,
/* Validate that a snapshot object does not violate any qemu-specific
- * constraints. */
+ * constraints. @state is virDomainState if flags implies creation, or
+ * virDomainSnapshotState if flags includes _REDEFINE (the latter
+ * enum is a superset of the former). */
static int
qemuDomainSnapshotValidate(virDomainSnapshotDefPtr def, int state,
unsigned int flags)
@@ -15808,7 +15810,8 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
parse_flags)))
goto cleanup;
- if (qemuDomainSnapshotValidate(def, vm->state.state, flags) < 0)
+ if (qemuDomainSnapshotValidate(def, redefine ? def->state : vm->state.state,
+ flags) < 0)
goto cleanup;
/* reject the VIR_DOMAIN_SNAPSHOT_CREATE_LIVE flag where not supported */
--
2.20.1
9:43 a.m.
New subject: [libvirt] [PATCH v2 8/7] news: More 5.1 updates
Mention my snapshot bug fixes, and the corresponding virsh command-line
parse tweak I added while working on the snapshot bug fixes.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
This should be valid whether we go with variant1 or variant2
from the rest of the series.
docs/news.xml | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/docs/news.xml b/docs/news.xml
index b86943ab47..0ecafffd30 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -157,6 +157,17 @@
version.
</description>
</change>
+ <change>
+ <summary>
+ Batch mode virsh and virt-admin parsing improvements
+ </summary>
+ <description>
+ When parsing a single-argument command_string in batch mode,
+ virsh and virt-admin now permit newlines in addition to
+ semicolons for splitting commands, and backslash-newline for
+ splitting long lines, to be more like shell parsing.
+ </description>
+ </change>
</section>
<section title="Bug fixes">
<change>
@@ -315,6 +326,16 @@
attributes for devices without an image.
</description>
</change>
+ <change>
+ <summary>
+ External snapshot metadata redefinition is fixed
+ </summary>
+ <description>
+ Attempting to use VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE to
+ reinstate the metadata describing an external snapshot
+ created earlier for an offline domain no longer fails.
+ </description>
+ </change>
</section>
</release>
<release version="v5.0.0" date="2019-01-15">
--
2.20.1
5:21 p.m.
On 2/27/19 3:04 PM, Eric Blake wrote:
John pointed out that patch 2 of my original bug fix did too
much in one patch. Now that we are in freeze for 5.1, I've
proposed two variants of the same fix: patch 1 is the bare
minimum to fix the bug and nothing else, while patches 3-7
are more in line with my v1 patch at doing other refactoring
work along the way (but now split into multiple logical steps)
that will prove useful for my other pending snapshot improvements
(https://www.redhat.com/archives/libvir-list/2019-February/msg01350.html,
but now 5.2 material).
I'm proposing both variants for comparison, although I already
suspect the answer will be 'use patch 1 for 5.1, then rebase
what remains of patches 3-7 into the other snapshot cleanups
for 5.2'.
variant2 is cleaner than my v1 patch 2/2 in that I no longer have
to use a default: label to hack around gcc's enum sanity checking
within a switch statement, but it required introducing a large
mechanical rename of all use of snapshot state values.
I was hoping someone else would take a bite before me, but I guess not...
So my choices are let's do some refactoring, add/modify some enums, and
make a cleaner fix, but have a longer series of changes or do a quick
short term hack for 5.1.0 and then as soon as 5.2.0 opens, revert that,
and do things properly. More less - a fair assessment?
There's also the has anyone outside of you noticed up to this point.
I vote for doing it right, but if there's strong opinions to hack and
replace, then I won't complain either.
You have my -
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
Eric Blake (7):
qemu: Fix snapshot redefine vs. domain state bug
Revert "qemu: Fix snapshot redefine vs. domain state bug"
qemu: Refactor check for _LIVE vs. _REDEFINE
qemu: Factor out qemuDomainSnapshotValidate() helper
snapshot: Rework virDomainSnapshotState enum
qemu: Use virDomainSnapshotState for switch statements
qemu: Fix snapshot redefine vs. domain state bug
src/conf/snapshot_conf.h | 21 ++++-
src/conf/snapshot_conf.c | 28 +++----
src/qemu/qemu_driver.c | 160 +++++++++++++++++++++++----------------
src/test/test_driver.c | 20 ++---
src/vbox/vbox_common.c | 4 +-
5 files changed, 137 insertions(+), 96 deletions(-)
2094
days inactive
2095
days old
10 comments
2 participants
participants (2)
-
Eric Blake -
John Ferlan