6:33 p.m.
Audit all changes to the qemu vm->current_snapshot, and make them
update the saved xml file for both the previous and the new
snapshot, so that there is always at most one snapshot with
<active>1</active> in the xml, and that snapshot is used as the
current snapshot even across libvirtd restarts.
* src/conf/domain_conf.h (_virDomainSnapshotDef): Alter member
type and name.
* src/conf/domain_conf.c (virDomainSnapshotDefParseString)
(virDomainSnapshotDefFormat): Update clients.
* docs/schemas/domainsnapshot.rng: Tighten rng.
* src/qemu/qemu_driver.c (qemuDomainSnapshotLoad): Reload current
snapshot.
(qemuDomainSnapshotCreateXML, qemuDomainRevertToSnapshot)
(qemuDomainSnapshotDiscard): Track current snapshot.
---
docs/schemas/domainsnapshot.rng | 5 ++-
src/conf/domain_conf.c | 6 ++-
src/conf/domain_conf.h | 4 +-
src/qemu/qemu_driver.c | 79 +++++++++++++++++++++++++++++---------
4 files changed, 71 insertions(+), 23 deletions(-)
diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng
index 86bab0b..410833f 100644
--- a/docs/schemas/domainsnapshot.rng
+++ b/docs/schemas/domainsnapshot.rng
@@ -29,7 +29,10 @@
</optional>
<optional>
<element name='active'>
- <text/>
+ <choice>
+ <value>0</value>
+ <value>1</value>
+ </choice>
</element>
</optional>
<optional>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index ce1f3c5..7793a13 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -10967,6 +10967,7 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const char
*xmlStr,
virDomainSnapshotDefPtr ret = NULL;
char *creation = NULL, *state = NULL;
struct timeval tv;
+ int active;
xml = virXMLParse(NULL, xmlStr, "domainsnapshot.xml");
if (!xml) {
@@ -11030,11 +11031,12 @@ virDomainSnapshotDefPtr virDomainSnapshotDefParseString(const
char *xmlStr,
goto cleanup;
}
- if (virXPathLong("string(./active)", ctxt, &def->active) < 0)
{
+ if (virXPathInt("string(./active)", ctxt, &active) < 0) {
virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Could not find 'active'
element"));
goto cleanup;
}
+ def->current = active != 0;
}
else
def->creationTime = tv.tv_sec;
@@ -11076,7 +11078,7 @@ char *virDomainSnapshotDefFormat(char *domain_uuid,
virBufferAsprintf(&buf, " <uuid>%s</uuid>\n",
domain_uuid);
virBufferAddLit(&buf, " </domain>\n");
if (internal)
- virBufferAsprintf(&buf, " <active>%ld</active>\n",
def->active);
+ virBufferAsprintf(&buf, " <active>%d</active>\n",
def->current);
virBufferAddLit(&buf, "</domainsnapshot>\n");
if (virBufferError(&buf)) {
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 2cc9b06..8382d28 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1297,13 +1297,15 @@ enum virDomainTaintFlags {
typedef struct _virDomainSnapshotDef virDomainSnapshotDef;
typedef virDomainSnapshotDef *virDomainSnapshotDefPtr;
struct _virDomainSnapshotDef {
+ /* Public XML. */
char *name;
char *description;
char *parent;
long long creationTime; /* in seconds */
int state;
- long active;
+ /* Internal use. */
+ bool current; /* At most one snapshot in the list should have this set */
};
typedef struct _virDomainSnapshotObj virDomainSnapshotObj;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7426690..76c5549 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -294,6 +294,7 @@ static void qemuDomainSnapshotLoad(void *payload,
char *fullpath;
virDomainSnapshotDefPtr def = NULL;
virDomainSnapshotObjPtr snap = NULL;
+ virDomainSnapshotObjPtr current = NULL;
char ebuf[1024];
virDomainObjLock(vm);
@@ -339,7 +340,8 @@ static void qemuDomainSnapshotLoad(void *payload,
def = virDomainSnapshotDefParseString(xmlStr, 0);
if (def == NULL) {
/* Nothing we can do here, skip this one */
- VIR_ERROR(_("Failed to parse snapshot XML from file '%s'"),
fullpath);
+ VIR_ERROR(_("Failed to parse snapshot XML from file
'%s'"),
+ fullpath);
VIR_FREE(fullpath);
VIR_FREE(xmlStr);
continue;
@@ -348,12 +350,22 @@ static void qemuDomainSnapshotLoad(void *payload,
snap = virDomainSnapshotAssignDef(&vm->snapshots, def);
if (snap == NULL) {
virDomainSnapshotDefFree(def);
+ } else if (snap->def->current) {
+ current = snap;
+ if (!vm->current_snapshot)
+ vm->current_snapshot = snap;
}
VIR_FREE(fullpath);
VIR_FREE(xmlStr);
}
+ if (vm->current_snapshot != current) {
+ VIR_ERROR(_("Too many snapshots claiming to be current for domain
%s"),
+ vm->def->name);
+ vm->current_snapshot = NULL;
+ }
+
/* FIXME: qemu keeps internal track of snapshots. We can get access
* to this info via the "info snapshots" monitor command for running
* domains, or via "qemu-img snapshot -l" for shutoff domains. It would
@@ -8468,12 +8480,17 @@ static virDomainSnapshotPtr
qemuDomainSnapshotCreateXML(virDomainPtr domain,
goto cleanup;
snap->def->state = virDomainObjGetState(vm, NULL);
+ snap->def->current = true;
if (vm->current_snapshot) {
def->parent = strdup(vm->current_snapshot->def->name);
if (def->parent == NULL) {
virReportOOMError();
goto cleanup;
}
+ vm->current_snapshot->def->current = false;
+ if (qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot,
+ driver->snapshotDir) < 0)
+ goto cleanup;
vm->current_snapshot = NULL;
}
@@ -8493,6 +8510,7 @@ static virDomainSnapshotPtr qemuDomainSnapshotCreateXML(virDomainPtr
domain,
*/
if (qemuDomainSnapshotWriteMetadata(vm, snap, driver->snapshotDir) < 0)
goto cleanup;
+ vm->current_snapshot = snap;
snapshot = virGetDomainSnapshot(domain, snap->def->name);
@@ -8780,7 +8798,17 @@ static int qemuDomainRevertToSnapshot(virDomainSnapshotPtr
snapshot,
goto cleanup;
}
- vm->current_snapshot = snap;
+ if (vm->current_snapshot) {
+ vm->current_snapshot->def->current = false;
+ if (qemuDomainSnapshotWriteMetadata(vm, vm->current_snapshot,
+ driver->snapshotDir) < 0)
+ goto cleanup;
+ vm->current_snapshot = NULL;
+ /* XXX Should we restore vm->current_snapshot after this point
+ * in the failure cases where we know there was no change? */
+ }
+
+ snap->def->current = true;
if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0)
goto cleanup;
@@ -8886,6 +8914,15 @@ endjob:
vm = NULL;
cleanup:
+ if (vm && ret == 0) {
+ if (qemuDomainSnapshotWriteMetadata(vm, snap,
+ driver->snapshotDir) < 0)
+ ret = -1;
+ else
+ vm->current_snapshot = snap;
+ } else if (snap) {
+ snap->def->current = false;
+ }
if (event)
qemuDomainEventQueue(driver, event);
if (vm)
@@ -8904,7 +8941,7 @@ static int qemuDomainSnapshotDiscard(struct qemud_driver *driver,
int ret = -1;
int i;
qemuDomainObjPrivatePtr priv;
- virDomainSnapshotObjPtr parentsnap;
+ virDomainSnapshotObjPtr parentsnap = NULL;
if (!virDomainObjIsActive(vm)) {
qemuimgarg[0] = qemuFindQemuImgBinary();
@@ -8943,31 +8980,35 @@ static int qemuDomainSnapshotDiscard(struct qemud_driver *driver,
qemuDomainObjExitMonitorWithDriver(driver, vm);
}
+ if (virAsprintf(&snapFile, "%s/%s/%s.xml", driver->snapshotDir,
+ vm->def->name, snap->def->name) < 0) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
if (snap == vm->current_snapshot) {
if (snap->def->parent) {
parentsnap = virDomainSnapshotFindByName(&vm->snapshots,
snap->def->parent);
if (!parentsnap) {
- qemuReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT,
- _("no domain snapshot parent with matching name
'%s'"),
- snap->def->parent);
- goto cleanup;
+ VIR_WARN("missing parent snapshot matching name '%s'",
+ snap->def->parent);
+ } else {
+ parentsnap->def->current = true;
+ if (qemuDomainSnapshotWriteMetadata(vm, snap,
+ driver->snapshotDir) < 0) {
+ VIR_WARN("failed to set parent snapshot '%s' as
current",
+ snap->def->parent);
+ parentsnap->def->current = false;
+ parentsnap = NULL;
+ }
}
-
- /* Now we set the new current_snapshot for the domain */
- vm->current_snapshot = parentsnap;
- } else {
- vm->current_snapshot = NULL;
}
+ vm->current_snapshot = parentsnap;
}
- if (virAsprintf(&snapFile, "%s/%s/%s.xml", driver->snapshotDir,
- vm->def->name, snap->def->name) < 0) {
- virReportOOMError();
- goto cleanup;
- }
- unlink(snapFile);
-
+ if (unlink(snapFile) < 0)
+ VIR_WARN("Failed to unlink %s", snapFile);
virDomainSnapshotObjListRemove(&vm->snapshots, snap);
ret = 0;
--
1.7.4.4