[libvirt] [PATCH 00/10] snapshot: misc fixes
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
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@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
On Thu, Dec 27, 2018 at 01:20:43PM +0300, Nikolay Shirokovskiy wrote:
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/conf/snapshot_conf.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@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
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@virtuozzo.com> --- src/conf/snapshot_conf.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@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
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@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
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@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
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@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
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@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
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@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
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@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
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@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
participants (2)
-
Ján Tomko -
Nikolay Shirokovskiy