[libvirt] [PATCH] snapshot: conf: Forbid using same file as external snapshot for disks and external snapshot for memory
When create external system checkpoint snapshot, snapshot file for disks should not be same with snapshot file for memory Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1148932 Signed-off-by: Shanzhi Yu <shyu@redhat.com> --- src/conf/snapshot_conf.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 1f83b2c..25af914 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -221,6 +221,7 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, char *tmp; char *memorySnapshot = NULL; char *memoryFile = NULL; + char *diskFile = NULL; bool offline = !!(flags & VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE); if (VIR_ALLOC(def) < 0) @@ -299,6 +300,7 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, memorySnapshot = virXPathString("string(./memory/@snapshot)", ctxt); memoryFile = virXPathString("string(./memory/@file)", ctxt); + diskFile = virXPathString("string(./disks/disk/source/@file)", ctxt); if (memorySnapshot) { def->memory = virDomainSnapshotLocationTypeFromString(memorySnapshot); if (def->memory <= 0) { @@ -322,6 +324,12 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, } } else if (memoryFile) { def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; + if (diskFile != NULL && STREQ(memoryFile,diskFile)){ + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("external snapshot file for disks should not be same" + " with external snapshot file for memory")); + goto cleanup; + } } else if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { def->memory = (offline ? VIR_DOMAIN_SNAPSHOT_LOCATION_NONE : @@ -336,6 +344,7 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, } def->file = memoryFile; memoryFile = NULL; + diskFile = NULL; /* verify that memory path is absolute */ if (def->file && def->file[0] != '/') { @@ -379,6 +388,7 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, VIR_FREE(nodes); VIR_FREE(memorySnapshot); VIR_FREE(memoryFile); + VIR_FREE(diskFile); if (ret == NULL) virDomainSnapshotDefFree(def); -- 1.9.3
On 10/02/2014 11:17 AM, Shanzhi Yu wrote:
When create external system checkpoint snapshot, snapshot file for disks should not be same with snapshot file for memory
This is a case of user stupidity. We can't always protect the user from shooting themselves in the foot. If it is easy, it may be worth adding, but I'm not convinced it is easy. A more foolproof (but still not bulletproof) approach would be to track the memspec via the lease manager, since the lease manager already avoids collisions even across situations that aren't easy to spot by mere string equality. But that has it's own set of design questions (to date, the lease manager covers only disk images, not saved state images).
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1148932 Signed-off-by: Shanzhi Yu <shyu@redhat.com> --- src/conf/snapshot_conf.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
} else if (memoryFile) { def->memory = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; + if (diskFile != NULL && STREQ(memoryFile,diskFile)){
STREQ_NULLABLE is a little more compact. However, this check is very weak - it does not detect things like 'a/b' and 'a//b' being the same file. I also think it is better to detect this via stat() at the point later on in the process where we sanitize the XML, rather than just here at parse time (where a user can leave the disk destination blank and rely on later code supplying a sane default, which would miss a collision with the memspec also using what would be the generated disk name). Thus, I think that if we are going to do this at all, it should be done in virDomainSnapshotAlignDisks(). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Shanzhi Yu