On 11/20/12 01:09, Eric Blake wrote:
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.
Now when 3/n is pushed this and the prepare one will be right together.
Although the changes in 2/n are trivial I'd probably rather keep them
separate for now.
---
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;
I'd also set "tmp" to NULL here and ...
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);
move this free statement right to the cleanup section.
+ 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. */
Hm, despite this was my idea it's looking more strange the more I think
about it. If we're going to have the users to specify a new image file
now we will need to support that after we come up with a better version.
I'm going to think about this a bit more. But for now it seems to be the
laziest solution.
+ 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) {
Looks good as a starting point.
Peter
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list