The removed hunk of code added in [1] in virDomainSnapshotAlignDisks
either duplicates qemuDomainSnapshotPrepare checks or prohibit some
meaningful cases and overall is complicated to think about (check next analysis).
It prohibit next cases:
A. external disk snapshot, which fall in 3 cases
1. there are other disks with internal snapshot
2. all other disks snapshots are external and domain is active
3. the same as above but domain is inactive
Cases 1 and 2 are disabled by qemuDomainSnapshotPrepare too and case 3 need not
to be prohibited. This way one can make external disk snapshot of inactive
domain without disk only flag. One may argue why we need such a possibility
- then why we have code at the bottom of qemuDomainSnapshotPrepare that adds
disk only to @flags. This @flags mutating code starts to function only in case 3.
B. disabling disk snapshot if it is not disabled in domain disk config too.
This actually does not make sense. Say snapshot is not disabled in disk config
and this check gives failure. Then I can disable it in disk config and pass
this check and after that disk config is never checked so it is not clear why
fail first case and pass second. I guess the intention was to prohibit
disabling non readonly disks as for readonly disk snapshot attribute is
set to none on parse. So we'd better analyze these possibilites which fall into
4 cases (all non-readonly):
1. empty disk and active domain
2. empty disk and inactive domain
3. non empty disk and there are some disks with internal snapshots
4. non empty disk and all other disks snapshots are external
Cases 1, 2 and 4 are handled valid cases. The reasoning for case 2 is
a bit tricky - it is hanled because only floppies are possible as
empty non readonly disks and those are skipped by qemuDomainSnapshotForEachQcow2Raw.
Case 3 is prohibited by qemuDomainSnapshotPrepare.
Now @require_match in virDomainSnapshotAlignDisks is not used and can be
removed.
The above analysis only covers actually creating snapshot but not redefining.
Reasoning here is just as in the previous patch - I guess users are not
supposed to edit disk's snapshot attributes.
[1] f9670b - snapshot: improve disk align checking
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@xxxxxxxxxxxxx>
---
src/conf/snapshot_conf.c | 33 +++++++--------------------------
src/conf/snapshot_conf.h | 3 +--
src/qemu/qemu_driver.c | 6 +-----
src/test/test_driver.c | 5 +----
4 files changed, 10 insertions(+), 37 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 7d5367f..2a35b25 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -526,12 +526,10 @@ virDomainSnapshotCompareDiskIndex(const void *a, const void *b)
* the domain, with a fallback to a passed in default. Convert paths
* to disk targets for uniformity. Issue an error and return -1 if
* any def->disks[n]->name appears more than once or does not map to
- * dom->disks. If require_match, also ensure that there is no
- * conflicting requests for both internal and external snapshots. */
+ * dom->disks. */
int
virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
- int default_snapshot,
- bool require_match)
+ int default_snapshot)
{
int ret = -1;
virBitmapPtr map = NULL;
@@ -586,17 +584,6 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
disk->snapshot = disk_snapshot;
else
disk->snapshot = default_snapshot;
- } else if (require_match &&
- disk->snapshot != default_snapshot &&
- !(disk->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE &&
- disk_snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE)) {
- const char *tmp;
-
- tmp = virDomainSnapshotLocationTypeToString(default_snapshot);
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("disk '%s' must use snapshot mode '%s'"),
- disk->name, tmp);
- goto cleanup;
}
if (STRNEQ(disk->name, def->dom->disks[idx]->dst)) {
VIR_FREE(disk->name);
@@ -1219,7 +1206,6 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
virDomainSnapshotDefPtr def = *defptr;
int ret = -1;
int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
- bool align_match = true;
char uuidstr[VIR_UUID_STRING_BUFLEN];
virDomainSnapshotObjPtr other;
@@ -1312,13 +1298,10 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
if (def->dom) {
if (def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
- virDomainSnapshotDefIsExternal(def)) {
+ virDomainSnapshotDefIsExternal(def))
align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
- align_match = false;
- }
- if (virDomainSnapshotAlignDisks(def, align_location,
- align_match) < 0) {
+ if (virDomainSnapshotAlignDisks(def, align_location) < 0) {
/* revert stealing of the snapshot domain definition */
if (def->dom && !other->def->dom) {
other->def->dom = def->dom;
@@ -1343,12 +1326,10 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
} else {
if (def->dom) {
if (def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
- def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
+ def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
- align_match = false;
- }
- if (virDomainSnapshotAlignDisks(def, align_location,
- align_match) < 0)
+
+ if (virDomainSnapshotAlignDisks(def, align_location) < 0)
goto cleanup;
}
}
diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
index 7c15d15..3333d2e 100644
--- a/src/conf/snapshot_conf.h
+++ b/src/conf/snapshot_conf.h
@@ -118,8 +118,7 @@ char *virDomainSnapshotDefFormat(const char *domain_uuid,
unsigned int flags,
int internal);
int virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr snapshot,
- int default_snapshot,
- bool require_match);
+ int default_snapshot);
virDomainSnapshotObjPtr virDomainSnapshotAssignDef(virDomainSnapshotObjListPtr snapshots,
virDomainSnapshotDefPtr def);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index ea316f6..c6fba1a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15555,7 +15555,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
unsigned int parse_flags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS;
virDomainSnapshotObjPtr other = NULL;
int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
- bool align_match = true;
virQEMUDriverConfigPtr cfg = NULL;
virCapsPtr caps = NULL;
qemuDomainObjPrivatePtr priv;
@@ -15695,7 +15694,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) {
align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
- align_match = false;
if (virDomainObjIsActive(vm))
def->state = VIR_DOMAIN_DISK_SNAPSHOT;
else
@@ -15704,7 +15702,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
} else if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
def->state = virDomainObjGetState(vm, NULL);
align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
- align_match = false;
} else {
def->state = virDomainObjGetState(vm, NULL);
@@ -15720,8 +15717,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
VIR_DOMAIN_SNAPSHOT_LOCATION_NONE :
VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL);
}
- if (virDomainSnapshotAlignDisks(def, align_location,
- align_match) < 0 ||
+ if (virDomainSnapshotAlignDisks(def, align_location) < 0 ||
qemuDomainSnapshotPrepare(vm, def, &flags) < 0)
goto endjob;
}
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index b76f0b7..dbbc77d 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -6314,11 +6314,9 @@ testDomainSnapshotAlignDisks(virDomainObjPtr vm,
unsigned int flags)
{
int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
- bool align_match = true;
if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) {
align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
- align_match = false;
if (virDomainObjIsActive(vm))
def->state = VIR_DOMAIN_DISK_SNAPSHOT;
else
@@ -6327,7 +6325,6 @@ testDomainSnapshotAlignDisks(virDomainObjPtr vm,
} else if (def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
def->state = virDomainObjGetState(vm, NULL);
align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
- align_match = false;
} else {
def->state = virDomainObjGetState(vm, NULL);
def->memory = def->state == VIR_DOMAIN_SHUTOFF ?
@@ -6335,7 +6332,7 @@ testDomainSnapshotAlignDisks(virDomainObjPtr vm,
VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
}
- return virDomainSnapshotAlignDisks(def, align_location, align_match);
+ return virDomainSnapshotAlignDisks(def, align_location);
}
static virDomainSnapshotPtr
--
1.8.3.1
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
