Re: [libvirt] [RFC PATCHv2 4/n] snapshot: actually compute branch definition from XML

When asked to parse a branch snapshot XML definition, we have to piece together the definition of the new snapshot from parts of the branch point, as well as run some sanity checks that the branches are compatible. This patch is rather restrictive in what it allows; depending on effort and future needs, we may be able to relax some of those restrictions and allow more cases. * src/conf/snapshot_conf.c (virDomainSnapshotDefParseString): Honor new flag.

--- src/conf/snapshot_conf.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 89 insertions(+), 2 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 10aa5e5..901705e 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -171,7 +171,7 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char *xmlStr, virCapsPtr caps, unsigned int expectedVirtTypes, - virDomainSnapshotObjListPtr snapshots ATTRIBUTE_UNUSED, + virDomainSnapshotObjListPtr snapshots, unsigned int flags) { xmlXPathContextPtr ctxt = NULL; @@ -188,6 +188,7 @@ virDomainSnapshotDefParseString(const char *xmlStr, char *memorySnapshot = NULL; char *memoryFile = NULL; bool offline = !!(flags & VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE); + virDomainSnapshotObjPtr other = NULL;

xml = virXMLParseCtxt(NULL, xmlStr, _("(domain_snapshot)"), &ctxt); if (!xml) { @@ -223,7 +224,61 @@ virDomainSnapshotDefParseString(const char *xmlStr, def->description = virXPathString("string(./description)", ctxt); - if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { + if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_BRANCH) { + if ((flags & (VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE | + VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL)) || + !snapshots) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unexpected parse request")); + goto cleanup; + } + + /* In order to form a branch, we must find the existing + * snapshot, and it must be external. */ + tmp = virXPathString("string(./branch)", ctxt); + if (!tmp) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("use of branch flag requires <branch> element")); + goto cleanup; + } + other = virDomainSnapshotFindByName(snapshots, tmp); + if (!other) { + virReportError(VIR_ERR_XML_ERROR, _("could not find branch '%s'"), + tmp); + VIR_FREE(tmp);

+ goto cleanup; + } + + if (!virDomainSnapshotIsExternal(other)) { + virReportError(VIR_ERR_XML_ERROR, + _("branch '%s' is not an external snapshot"), tmp); + VIR_FREE(tmp); + goto cleanup; + } + if (!other->def->dom) { + virReportError(VIR_ERR_XML_ERROR, + _("branch '%s' lacks corresponding domain details"), + tmp); + VIR_FREE(tmp); + goto cleanup; + } + VIR_FREE(tmp); + + /* The new definition shares the same <parent>, <state>, and + * <domain> as what it is branching from. */ + def->creationTime = tv.tv_sec; + if (other->def->parent && + !(def->parent = strdup(other->def->parent))) { + virReportOOMError(); + goto cleanup; + } + def->state = other->def->state; + + /* Any <memory> or <disk> in the XML must be consistent with + * the branch point, and any <disk> not in the XML will be + * populated to match the branch; checked below. */ + + } else if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE) { if (virXPathLongLong("string(./creationTime)", ctxt, &def->creationTime) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -314,6 +369,24 @@ virDomainSnapshotDefParseString(const char *xmlStr, _("memory state cannot be saved with offline snapshot")); goto cleanup; } + if (other) { + /* XXX It would be nice to allow automatic reuse of existing + * memory files, with bookkeeping that prevents deleting a + * file if some other branch is still using it. But for a + * first implementation, it is easier to enforce that the user + * always matches (disk-only branching to disk-only; + * checkpoint branching to checkpoint and giving us a new name + * which we set up as a copy). There is also a question of + * whether attempting a hard link rather than always copying + * to a new inode makes sense, if the original is a regular + * file and not a block device. */

+ if (other->def->memory != def->memory) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("cannot convert between disk-only and checkpoint; " + "<memory> element must match across branch")); + goto cleanup; + } + } def->file = memoryFile; memoryFile = NULL; @@ -335,6 +408,20 @@ virDomainSnapshotDefParseString(const char *xmlStr, _("unable to handle disk requests in snapshot")); goto cleanup; } + if (other) { + /* For now, we only allow branch snapshots of external snapshots. */ + if (virDomainSnapshotAlignDisks(def, + VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL, + true) < 0) + goto cleanup; + for (i = 0; i < def->ndisks; def++) + if (def->disks[i].snapshot != other->def->disks[i].snapshot) { + virReportError(VIR_ERR_XML_ERROR, + _("mismatch in snapshot mode for disk '%s'"), + def->disks[i].name); + goto cleanup; + } + } if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL) { if (virXPathInt("string(./active)", ctxt, &active) < 0) {