4:20 a.m.
The patch series aims to uniformly handle snapshot attribute for disks from
multiple places - snapshot definitions, domain definitions, snapshot operation
and defaults which is done in [1] utimately. Patch [2] makes a step towards
this goal. Other patches are fixes along the way that disables some invalid
cases on conversely enable previously disable valid cases.
The patches with most reasoning ([2] and [3]) are effectively revert Eric's
patch f9670bf thus I hope Eric will take a look.
Nikolay Shirokovskiy (10):
conf: snapshot: fix comment in _virDomainSnapshotDef
conf: snapshot: don't pass flags from different family
conf: snapshot: check disk with path on parse
conf: snapshot: align exernal/internal snapshot the same way [2]
conf: snapshot: remove snapshot mode checking from disk align [3]
conf: virDomainSnapshotAlignDisks: use convinient variable
qemu: disable internal snapshot of readonly disk
qemu: snapshot: fix for case of disk with empty source
qemu: snapshot: align disks consistently [1]
conf: snapshot: make disk aligns same on redefinition
src/conf/domain_conf.c | 6 +--
src/conf/snapshot_conf.c | 103 ++++++++++++++++++++++-------------------------
src/conf/snapshot_conf.h | 5 +--
src/qemu/qemu_driver.c | 27 ++++++++++---
src/test/test_driver.c | 5 +--
5 files changed, 74 insertions(+), 72 deletions(-)
--
1.8.3.1
4:20 a.m.
New subject: [libvirt] [PATCH 01/10] conf: snapshot: fix comment in _virDomainSnapshotDef
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/conf/snapshot_conf.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
index 5ac1ba7..7c15d15 100644
--- a/src/conf/snapshot_conf.h
+++ b/src/conf/snapshot_conf.h
@@ -66,7 +66,7 @@ struct _virDomainSnapshotDef {
long long creationTime; /* in seconds */
int state; /* virDomainSnapshotState */
- int memory; /* virDomainMemorySnapshot */
+ int memory; /* virDomainSnapshotLocation */
char *file; /* memory state file when snapshot is external */
size_t ndisks; /* should not exceed dom->ndisks */
--
1.8.3.1
8:46 a.m.
New subject: [libvirt] [PATCH 01/10] conf: snapshot: fix comment in _virDomainSnapshotDef
On Thu, Dec 27, 2018 at 01:20:43PM +0300, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy
<nshirokovskiy(a)virtuozzo.com>
---
src/conf/snapshot_conf.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
4:20 a.m.
New subject: [libvirt] [PATCH 02/10] conf: snapshot: don't pass flags from different family
virDomainDiskSourceParse expects VIR_DOMAIN_DEF_PARSE_* values
in @flags and we pass VIR_DOMAIN_SNAPSHOT_PARSE_* values. Fortunately
sources of type 'file' and 'block' do not take flags into account
and for source of type 'network' flags only make difference for
tlsFromConfig atribute which is never passed I guess and never
used.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/conf/snapshot_conf.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 7386b4a..db55b4f 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -108,7 +108,6 @@ static int
virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
xmlXPathContextPtr ctxt,
virDomainSnapshotDiskDefPtr def,
- unsigned int flags,
virDomainXMLOptionPtr xmlopt)
{
int ret = -1;
@@ -154,7 +153,7 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
}
if ((cur = virXPathNode("./source", ctxt)) &&
- virDomainDiskSourceParse(cur, ctxt, def->src, flags, xmlopt) < 0)
+ virDomainDiskSourceParse(cur, ctxt, def->src, 0, xmlopt) < 0)
goto cleanup;
if ((driver = virXPathString("string(./driver/@type)", ctxt)) &&
@@ -339,7 +338,7 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
def->ndisks = n;
for (i = 0; i < def->ndisks; i++) {
if (virDomainSnapshotDiskDefParseXML(nodes[i], ctxt, &def->disks[i],
- flags, xmlopt) < 0)
+ xmlopt) < 0)
goto cleanup;
}
VIR_FREE(nodes);
--
1.8.3.1
6:47 a.m.
New subject: [libvirt] [PATCH 02/10] conf: snapshot: don't pass flags from different family
On Thu, Dec 27, 2018 at 01:20:44PM +0300, Nikolay Shirokovskiy wrote:
virDomainDiskSourceParse expects VIR_DOMAIN_DEF_PARSE_* values
in @flags and we pass VIR_DOMAIN_SNAPSHOT_PARSE_* values. Fortunately
sources of type 'file' and 'block' do not take flags into account
and for source of type 'network' flags only make difference for
tlsFromConfig atribute which is never passed I guess and never
"I guess" does not sound comforting.
How about:
which is only formatted for the status XML (with the
VIR_DOMAIN_DEF_PARSE_STATUS flag), not the domain definition inside
the snapshot XML, which uses VIR_DOMAIN_DEF_FORMAT_INACTIVE.
used.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/conf/snapshot_conf.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
4:20 a.m.
New subject: [libvirt] [PATCH 03/10] conf: snapshot: check disk with path on parse
We can move the hunk to the parsing stage. Because if disk->src->path
is set and and disk->snapshot is not then it will be set to external
on parsing stage by the above hunk in virDomainSnapshotDiskDefParseXML.
Disk aligning will not have chance to set disk->snapshot.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/conf/snapshot_conf.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index db55b4f..bd125dc 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -174,6 +174,15 @@ virDomainSnapshotDiskDefParseXML(xmlNodePtr node,
if (!def->snapshot && (def->src->path || def->src->format))
def->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
+ if (def->src->path &&
+ def->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("file '%s' for disk '%s' requires "
+ "use of external snapshot mode"),
+ def->src->path, def->name);
+ goto cleanup;
+ }
+
ret = 0;
cleanup:
ctxt->node = saved;
@@ -591,14 +600,6 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
disk->name, tmp);
goto cleanup;
}
- if (disk->src->path &&
- disk->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("file '%s' for disk '%s' requires
"
- "use of external snapshot mode"),
- disk->src->path, disk->name);
- goto cleanup;
- }
if (STRNEQ(disk->name, def->dom->disks[idx]->dst)) {
VIR_FREE(disk->name);
if (VIR_STRDUP(disk->name, def->dom->disks[idx]->dst) < 0)
--
1.8.3.1
4:20 a.m.
New subject: [libvirt] [PATCH 04/10] conf: snapshot: align exernal/internal snapshot the same way
In case !require_match (disk snapshots or snapshot with external memory)
setting disk->snapshot if it is not set in snapshot definition is
straigtforward. First check default value for the disk from domain definition
(disk_snapshot) and use it if it is set, second if it is not set then use
default value provided as function argument. But require_match case is
trickier. For some reason it reverts this logic except for the case when
disk_snapshot is none. This logic added in commit [1]. AFAIU this is done
because mixing internal and external disks snapshots is not supported (and
still is). But then it is not clear why in case of disks snapshots
(!require_match) the logic is not the same as for internal snapshots because
mixing is not supported in both cases. Also it is seems very surprising that
for some snapshots domain disks settings are respected and for others are not.
Making exception for none seems complicating things further. AFAIU this
exception intention was to disable snapshots for readonly disks because for
these disks disk_snapshot is set to none on parsing stage.
I suggest to use same logic for require_match as for !require_match. This makes
things graspable in respect to settings priorities coming from different places.
And this breaks clients that set disks_snapshot to external and then makes
internal snapshots. I hope this was not used by anyone. There are no other
changes.
This function is used on snapshot redefine too. I guess users are not
supposed to remove snapshot attributes for disks from previously
dumped snapshot xmls.
[1] f9670b - snapshot: improve disk align checking
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/conf/snapshot_conf.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index bd125dc..7d5367f 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -582,9 +582,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
disk_snapshot = def->dom->disks[idx]->snapshot;
if (!disk->snapshot) {
- if (disk_snapshot &&
- (!require_match ||
- disk_snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE))
+ if (disk_snapshot)
disk->snapshot = disk_snapshot;
else
disk->snapshot = default_snapshot;
--
1.8.3.1
4:20 a.m.
New subject: [libvirt] [PATCH 05/10] conf: snapshot: remove snapshot mode checking from disk align
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(a)virtuozzo.com>
---
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
4:20 a.m.
New subject: [libvirt] [PATCH 06/10] conf: virDomainSnapshotAlignDisks: use convinient variable
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/conf/snapshot_conf.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 2a35b25..002cd45 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -560,6 +560,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
/* Double check requested disks. */
for (i = 0; i < def->ndisks; i++) {
virDomainSnapshotDiskDefPtr disk = &def->disks[i];
+ virDomainDiskDefPtr dom_disk;
int idx = virDomainDiskIndexByName(def->dom, disk->name, false);
int disk_snapshot;
@@ -577,17 +578,18 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
}
ignore_value(virBitmapSetBit(map, idx));
disk->idx = idx;
+ dom_disk = def->dom->disks[idx];
- disk_snapshot = def->dom->disks[idx]->snapshot;
+ disk_snapshot = dom_disk->snapshot;
if (!disk->snapshot) {
if (disk_snapshot)
disk->snapshot = disk_snapshot;
else
disk->snapshot = default_snapshot;
}
- if (STRNEQ(disk->name, def->dom->disks[idx]->dst)) {
+ if (STRNEQ(disk->name, dom_disk->dst)) {
VIR_FREE(disk->name);
- if (VIR_STRDUP(disk->name, def->dom->disks[idx]->dst) < 0)
+ if (VIR_STRDUP(disk->name, dom_disk->dst) < 0)
goto cleanup;
}
}
--
1.8.3.1
4:20 a.m.
New subject: [libvirt] [PATCH 07/10] qemu: disable internal snapshot of readonly disk
Now in case of active domain such a snapshot will complete
successfully but disk will not have a correspondent internal snapshot
as qemu will not make one. In case of inactive domain operation
will finish successfully too but disk snapshot will be done.
Let's disable such a snapshot because as described above in case
of active domain it is not possible now and in both cases it does
not make much sense. We recently disable external snapshot of
readonly disk too in [1].
[1] 067aad26b - qemu: disable external snapshot of readonly disk
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/qemu/qemu_driver.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index c6fba1a..506d8ab 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14811,6 +14811,13 @@ qemuDomainSnapshotPrepareDiskInternal(virDomainDiskDefPtr disk,
{
int actualType;
+ if (disk->src->readonly) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("internal snapshot for readonly disk %s "
+ "is not supported"), disk->dst);
+ return -1;
+ }
+
/* active disks are handled by qemu itself so no need to worry about those */
if (active)
return 0;
--
1.8.3.1
4:20 a.m.
New subject: [libvirt] [PATCH 08/10] qemu: snapshot: fix for case of disk with empty source
It is possible to specify any snapshot option for disk with empty
source. Only cdrom and floppy can have empty source but cdrom
is out of considering because it is always readonly (set on parse
stage) and snapshotting readonly disks is now completely prohibited.
Let's check floppies.
In case of internal snapshot of such a disk for active or inactive domain
snapshot returns success but actual snapshot is not possible of course.
We need to prohibit this cases explicitly.
In case of external snapshot of such a disk for active or inactive domain
we get errors [1] and [2] respectively. It is better to have more
user-frieldly messages.
This patch prohibits explicitly all the above cases.
[1] error in case of external disk snapshot of active domain
error: internal error: unable to execute QEMU command 'transaction': Device
'drive-fdc0-0-0' has no medium
[2] error in case of external disk snapshot of inactive domain
error: internal error: Child process (/usr/local/bin/qemu-img create -f qcow2 \
-o 'backing_file=(null),backing_fmt=qcow2' /path/file.qcow2) \
unexpected exit status 1: qemu-img: /path/file.qcow2: \
Could not open '/path/(null)': No such file or directory
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/qemu/qemu_driver.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 506d8ab..bd3f00b 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14760,6 +14760,13 @@ qemuDomainSnapshotPrepareDiskExternal(virDomainDiskDefPtr disk,
return -1;
}
+ if (virStorageSourceIsEmpty(disk->src)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("snapshot for disk %s with empty source "
+ "is not possible"), disk->dst);
+ return -1;
+ }
+
if (qemuTranslateSnapshotDiskSourcePool(snapdisk) < 0)
return -1;
@@ -14818,6 +14825,13 @@ qemuDomainSnapshotPrepareDiskInternal(virDomainDiskDefPtr disk,
return -1;
}
+ if (virStorageSourceIsEmpty(disk->src)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("snapshot for disk %s with empty source "
+ "is not possible"), disk->dst);
+ return -1;
+ }
+
/* active disks are handled by qemu itself so no need to worry about those */
if (active)
return 0;
--
1.8.3.1
4:20 a.m.
New subject: [libvirt] [PATCH 09/10] qemu: snapshot: align disks consistently
There is inconsitency in how we set disk snapshot attribute for missing disk
and for disk explicitly present in snapshot definition in virDomainSnapshotAlignDisks.
First for explicit disk we do not check if disk source is present. After the
previous patch this will cause snapshot failures so we'd better disable
snapshotting of such disk at this place. (For the record: we could have
failures before previous patch too, it just does not makes sense to snapshot
disk without source).
Second for missing disks with empty source disabling snapshot take precedence
over user settings. This does not feel right. It is better report to user that
option he wanted in not supported/possible rather then ignoring it. This
can break things a bit. Hopefully nobody really uses domain disk snapshot
setting.
Next let's remove setting disk snapshot on parse stage and move it altogether
to one place - disk align function. Other hypervisors does not check this
attribute in domain disk config so we are free here.
Now we can place logic for setting disk snapshot value in one function
- virDomainSnapshotSetDiskSnapshot.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/conf/domain_conf.c | 6 +-----
src/conf/snapshot_conf.c | 38 +++++++++++++++++++++++---------------
2 files changed, 24 insertions(+), 20 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d8dfd16..0fbd739 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -9759,8 +9759,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
snapshot);
goto error;
}
- } else if (def->src->readonly) {
- def->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
}
if (rawio) {
@@ -24285,9 +24283,7 @@ virDomainDiskDefFormat(virBufferPtr buf,
if (def->sgio)
virBufferAsprintf(buf, " sgio='%s'", sgio);
- if (def->snapshot &&
- !(def->snapshot == VIR_DOMAIN_SNAPSHOT_LOCATION_NONE &&
- def->src->readonly))
+ if (def->snapshot)
virBufferAsprintf(buf, " snapshot='%s'",
virDomainSnapshotLocationTypeToString(def->snapshot));
virBufferAddLit(buf, ">\n");
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 002cd45..172dff8 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -521,6 +521,25 @@ virDomainSnapshotCompareDiskIndex(const void *a, const void *b)
return diska->idx - diskb->idx;
}
+
+static void
+virDomainSnapshotSetDiskSnapshot(virDomainSnapshotDiskDefPtr disk,
+ virDomainDiskDefPtr dom_disk,
+ int default_snapshot)
+{
+ if (disk->snapshot)
+ return;
+
+ if (dom_disk->snapshot)
+ disk->snapshot = dom_disk->snapshot;
+ else if (dom_disk->src->readonly ||
+ virStorageSourceIsEmpty(dom_disk->src))
+ disk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
+ else
+ disk->snapshot = default_snapshot;
+}
+
+
/* Align def->disks to def->domain. Sort the list of def->disks,
* filling in any missing disks or snapshot state defaults given by
* the domain, with a fallback to a passed in default. Convert paths
@@ -562,7 +581,6 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
virDomainSnapshotDiskDefPtr disk = &def->disks[i];
virDomainDiskDefPtr dom_disk;
int idx = virDomainDiskIndexByName(def->dom, disk->name, false);
- int disk_snapshot;
if (idx < 0) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -580,13 +598,8 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
disk->idx = idx;
dom_disk = def->dom->disks[idx];
- disk_snapshot = dom_disk->snapshot;
- if (!disk->snapshot) {
- if (disk_snapshot)
- disk->snapshot = disk_snapshot;
- else
- disk->snapshot = default_snapshot;
- }
+ virDomainSnapshotSetDiskSnapshot(disk, dom_disk, default_snapshot);
+
if (STRNEQ(disk->name, dom_disk->dst)) {
VIR_FREE(disk->name);
if (VIR_STRDUP(disk->name, dom_disk->dst) < 0)
@@ -612,15 +625,10 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def,
goto cleanup;
disk->idx = i;
- /* Don't snapshot empty drives */
- if (virStorageSourceIsEmpty(def->dom->disks[i]->src))
- disk->snapshot = VIR_DOMAIN_SNAPSHOT_LOCATION_NONE;
- else
- disk->snapshot = def->dom->disks[i]->snapshot;
+ virDomainSnapshotSetDiskSnapshot(disk, def->dom->disks[i],
+ default_snapshot);
disk->src->type = VIR_STORAGE_TYPE_FILE;
- if (!disk->snapshot)
- disk->snapshot = default_snapshot;
}
qsort(&def->disks[0], def->ndisks, sizeof(def->disks[0]),
--
1.8.3.1
4:20 a.m.
New subject: [libvirt] [PATCH 10/10] conf: snapshot: make disk aligns same on redefinition
This patch is similar to [1] but reason is different. At time [1] was written
there is a check in virDomainSnapshotAlignDisks (it is removed in [2]) that
won't allow external disks of external snapshot because of wrong align_location
VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL. Now this check is gone.
In case of disk snapshot and external snapshots we always dumped disks (see [3]
were at that time libvirt only makes active disk only snapshots, active
internal snaphots and inactive internal snapshots. Then after the patch we
start to dump disks snapshot section for all snapshots and before we do only
active disk only snapshots). So if user's edits to dumped snapshots before
redefine is limited then we don't need actually to call align for external
cases. But's lets leave it for the case and then make disk align same whether
snapshot is redefined first time or not.
[1] 731a5a4 - snapshot: qemu: Allow redefinition of external snapshots
[2] --- * - conf: snapshot: remove snapshot mode checking from disk align
[3] 4201a7e - snapshot: new XML for external system checkpoint
* not yet pushed upstream (part of current patch series)
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com>
---
src/conf/snapshot_conf.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index 172dff8..3033827 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -1307,6 +1307,8 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
}
if (def->dom) {
+ /* we can skip align in this cases as well as we always
+ * dumped disks for these cases */
if (def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
virDomainSnapshotDefIsExternal(def))
align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
@@ -1335,8 +1337,10 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
*snap = other;
} else {
if (def->dom) {
+ /* we can skip align in this cases as well as we always
+ * dumped disks for these cases */
if (def->state == VIR_DOMAIN_DISK_SNAPSHOT ||
- def->memory == VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL)
+ virDomainSnapshotDefIsExternal(def))
align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
if (virDomainSnapshotAlignDisks(def, align_location) < 0)
--
1.8.3.1
1930
days inactive
1970
days old
12 comments
2 participants
participants (2)
-
Ján Tomko -
Nikolay Shirokovskiy