It is easier to track the current snapshot as part of the list of
snapshots. In particular, doing so lets us guarantee that the current
snapshot is cleared if that snapshot is removed from the list (rather
than depending on the caller to do so, and risking a use-after-free
problem). This requires the addition of several new accessor
functions.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
src/conf/domain_conf.h | 1 -
src/conf/virdomainsnapshotobjlist.h | 10 +++--
src/conf/snapshot_conf.c | 4 +-
src/conf/virdomainsnapshotobjlist.c | 63 ++++++++++++++++++++++++-----
src/libvirt_private.syms | 4 ++
src/qemu/qemu_domain.c | 14 +++----
src/qemu/qemu_driver.c | 47 +++++++++------------
src/test/test_driver.c | 38 ++++++++---------
8 files changed, 109 insertions(+), 72 deletions(-)
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 538fb50b9e..e608440444 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2517,7 +2517,6 @@ struct _virDomainObj {
virDomainDefPtr newDef; /* New definition to activate at shutdown */
virDomainSnapshotObjListPtr snapshots;
- virDomainSnapshotObjPtr current_snapshot;
bool hasManagedSave;
diff --git a/src/conf/virdomainsnapshotobjlist.h b/src/conf/virdomainsnapshotobjlist.h
index 2a1ee86586..e210849441 100644
--- a/src/conf/virdomainsnapshotobjlist.h
+++ b/src/conf/virdomainsnapshotobjlist.h
@@ -33,14 +33,12 @@ void virDomainSnapshotObjListFree(virDomainSnapshotObjListPtr snapshots);
int virDomainSnapshotObjListParse(const char *xmlStr,
const unsigned char *domain_uuid,
virDomainSnapshotObjListPtr snapshots,
- virDomainSnapshotObjPtr *current_snap,
virCapsPtr caps,
virDomainXMLOptionPtr xmlopt,
unsigned int flags);
int virDomainSnapshotObjListFormat(virBufferPtr buf,
const char *uuidstr,
virDomainSnapshotObjListPtr snapshots,
- virDomainSnapshotObjPtr current_snapshot,
virCapsPtr caps,
virDomainXMLOptionPtr xmlopt,
unsigned int flags);
@@ -57,7 +55,13 @@ int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots,
unsigned int flags);
virDomainSnapshotObjPtr virDomainSnapshotFindByName(virDomainSnapshotObjListPtr snapshots,
const char *name);
-void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots,
+virDomainSnapshotObjPtr virDomainSnapshotGetCurrent(virDomainSnapshotObjListPtr snapshots);
+const char *virDomainSnapshotGetCurrentName(virDomainSnapshotObjListPtr snapshots);
+bool virDomainSnapshotIsCurrentName(virDomainSnapshotObjListPtr snapshots,
+ const char *name);
+void virDomainSnapshotSetCurrent(virDomainSnapshotObjListPtr snapshots,
+ virDomainSnapshotObjPtr snapshot);
+bool virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots,
virDomainSnapshotObjPtr snapshot);
int virDomainSnapshotForEach(virDomainSnapshotObjListPtr snapshots,
virHashIterator iter,
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index bf5fdc0647..c692d36bd1 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -969,9 +969,9 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
return -1;
}
if (other) {
- if (other == vm->current_snapshot) {
+ if (other == virDomainSnapshotGetCurrent(vm->snapshots)) {
*update_current = true;
- vm->current_snapshot = NULL;
+ virDomainSnapshotSetCurrent(vm->snapshots, NULL);
}
/* Drop and rebuild the parent relationship, but keep all
diff --git a/src/conf/virdomainsnapshotobjlist.c b/src/conf/virdomainsnapshotobjlist.c
index be44bdde71..1eecb89a5d 100644
--- a/src/conf/virdomainsnapshotobjlist.c
+++ b/src/conf/virdomainsnapshotobjlist.c
@@ -40,6 +40,7 @@ struct _virDomainSnapshotObjList {
virHashTable *objs;
virDomainSnapshotObj metaroot; /* Special parent of all root snapshots */
+ virDomainSnapshotObjPtr current; /* The current snapshot, if any */
};
@@ -50,7 +51,6 @@ int
virDomainSnapshotObjListParse(const char *xmlStr,
const unsigned char *domain_uuid,
virDomainSnapshotObjListPtr snapshots,
- virDomainSnapshotObjPtr *current_snap,
virCapsPtr caps,
virDomainXMLOptionPtr xmlopt,
unsigned int flags)
@@ -62,6 +62,7 @@ virDomainSnapshotObjListParse(const char *xmlStr,
int n;
size_t i;
int keepBlanksDefault = xmlKeepBlanksDefault(0);
+ virDomainSnapshotObjPtr snap;
VIR_AUTOFREE(xmlNodePtr *) nodes = NULL;
VIR_AUTOFREE(char *) current = NULL;
@@ -71,7 +72,7 @@ virDomainSnapshotObjListParse(const char *xmlStr,
_("incorrect flags for bulk parse"));
return -1;
}
- if (snapshots->metaroot.nchildren || *current_snap) {
+ if (snapshots->metaroot.nchildren || snapshots->current) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("bulk define of snapshots only possible with "
"no existing snapshot"));
@@ -106,7 +107,6 @@ virDomainSnapshotObjListParse(const char *xmlStr,
for (i = 0; i < n; i++) {
virDomainSnapshotDefPtr def;
- virDomainSnapshotObjPtr snap;
def = virDomainSnapshotDefParseNode(xml, nodes[i], caps, xmlopt, NULL,
flags);
@@ -129,12 +129,13 @@ virDomainSnapshotObjListParse(const char *xmlStr,
}
if (current) {
- if (!(*current_snap = virDomainSnapshotFindByName(snapshots,
- current))) {
+ snap = virDomainSnapshotFindByName(snapshots, current);
+ if (!snap) {
virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT,
_("no snapshot matching current='%s'"), current);
goto cleanup;
}
+ virDomainSnapshotSetCurrent(snapshots, snap);
}
ret = 0;
@@ -183,7 +184,6 @@ int
virDomainSnapshotObjListFormat(virBufferPtr buf,
const char *uuidstr,
virDomainSnapshotObjListPtr snapshots,
- virDomainSnapshotObjPtr current_snapshot,
virCapsPtr caps,
virDomainXMLOptionPtr xmlopt,
unsigned int flags)
@@ -197,9 +197,8 @@ virDomainSnapshotObjListFormat(virBufferPtr buf,
};
virBufferAddLit(buf, "<snapshots");
- if (current_snapshot)
- virBufferEscapeString(buf, " current='%s'",
- current_snapshot->def->name);
+ virBufferEscapeString(buf, " current='%s'",
+ virDomainSnapshotGetCurrentName(snapshots));
virBufferAddLit(buf, ">\n");
virBufferAdjustIndent(buf, 2);
if (virDomainSnapshotForEach(snapshots, virDomainSnapshotFormatOne,
@@ -437,10 +436,52 @@ virDomainSnapshotFindByName(virDomainSnapshotObjListPtr snapshots,
return name ? virHashLookup(snapshots->objs, name) : &snapshots->metaroot;
}
-void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots,
+
+/* Return the current snapshot, or NULL */
+virDomainSnapshotObjPtr
+virDomainSnapshotGetCurrent(virDomainSnapshotObjListPtr snapshots)
+{
+ return snapshots->current;
+}
+
+
+/* Return the current snapshot's name, or NULL */
+const char *
+virDomainSnapshotGetCurrentName(virDomainSnapshotObjListPtr snapshots)
+{
+ if (snapshots->current)
+ return snapshots->current->def->name;
+ return NULL;
+}
+
+
+/* Return true if name matches the current snapshot */
+bool
+virDomainSnapshotIsCurrentName(virDomainSnapshotObjListPtr snapshots,
+ const char *name)
+{
+ return snapshots->current && STREQ(snapshots->current->def->name, name);
+}
+
+
+/* Update the current snapshot, using NULL if no current remains */
+void
+virDomainSnapshotSetCurrent(virDomainSnapshotObjListPtr snapshots,
+ virDomainSnapshotObjPtr snapshot)
+{
+ snapshots->current = snapshot;
+}
+
+
+/* Remove snapshot from the list; return true if it was current */
+bool virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots,
virDomainSnapshotObjPtr snapshot)
{
+ bool ret = snapshots->current == snapshot;
virHashRemoveEntry(snapshots->objs, snapshot->def->name);
+ if (ret)
+ snapshots->current = NULL;
+ return ret;
}
int
@@ -507,6 +548,8 @@ virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots)
snapshots->metaroot.nchildren = 0;
snapshots->metaroot.first_child = NULL;
virHashForEach(snapshots->objs, virDomainSnapshotSetRelations, &act);
+ if (act.err)
+ snapshots->current = NULL;
return act.err;
}
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 26f10bd47f..72c5cef528 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -990,6 +990,9 @@ virDomainListSnapshots;
virDomainSnapshotAssignDef;
virDomainSnapshotFindByName;
virDomainSnapshotForEach;
+virDomainSnapshotGetCurrent;
+virDomainSnapshotGetCurrentName;
+virDomainSnapshotIsCurrentName;
virDomainSnapshotObjListFormat;
virDomainSnapshotObjListFree;
virDomainSnapshotObjListGetNames;
@@ -997,6 +1000,7 @@ virDomainSnapshotObjListNew;
virDomainSnapshotObjListNum;
virDomainSnapshotObjListParse;
virDomainSnapshotObjListRemove;
+virDomainSnapshotSetCurrent;
virDomainSnapshotUpdateRelations;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index f8387339f0..b1a84d3914 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8461,7 +8461,7 @@ qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm,
unsigned int flags = VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE |
VIR_DOMAIN_SNAPSHOT_FORMAT_INTERNAL;
- if (vm->current_snapshot == snapshot)
+ if (virDomainSnapshotGetCurrent(vm->snapshots) == snapshot)
flags |= VIR_DOMAIN_SNAPSHOT_FORMAT_CURRENT;
virUUIDFormat(vm->def->uuid, uuidstr);
newxml = virDomainSnapshotDefFormat(uuidstr, snapshot->def, caps, xmlopt,
@@ -8614,8 +8614,8 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
vm->def->name, snap->def->name) < 0)
goto cleanup;
- if (snap == vm->current_snapshot) {
- vm->current_snapshot = NULL;
+ if (snap == virDomainSnapshotGetCurrent(vm->snapshots)) {
+ virDomainSnapshotSetCurrent(vm->snapshots, NULL);
if (update_parent && snap->def->parent) {
parentsnap = virDomainSnapshotFindByName(vm->snapshots,
snap->def->parent);
@@ -8623,13 +8623,13 @@ qemuDomainSnapshotDiscard(virQEMUDriverPtr driver,
VIR_WARN("missing parent snapshot matching name '%s'",
snap->def->parent);
} else {
- vm->current_snapshot = parentsnap;
+ virDomainSnapshotSetCurrent(vm->snapshots, parentsnap);
if (qemuDomainSnapshotWriteMetadata(vm, parentsnap, driver->caps,
driver->xmlopt,
cfg->snapshotDir) < 0) {
VIR_WARN("failed to set parent snapshot '%s' as current",
snap->def->parent);
- vm->current_snapshot = NULL;
+ virDomainSnapshotSetCurrent(vm->snapshots, NULL);
}
}
}
@@ -8658,7 +8658,7 @@ int qemuDomainSnapshotDiscardAll(void *payload,
virQEMUSnapRemovePtr curr = data;
int err;
- if (curr->vm->current_snapshot == snap)
+ if (virDomainSnapshotGetCurrent(curr->vm->snapshots) == snap)
curr->current = true;
err = qemuDomainSnapshotDiscard(curr->driver, curr->vm, snap, false,
curr->metadata_only);
@@ -8679,8 +8679,6 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr driver,
rem.err = 0;
virDomainSnapshotForEach(vm->snapshots, qemuDomainSnapshotDiscardAll,
&rem);
- if (rem.current)
- vm->current_snapshot = NULL;
if (virDomainSnapshotUpdateRelations(vm->snapshots) < 0 && !rem.err)
rem.err = -1;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7e0e76a31a..6c71382b93 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -482,9 +482,11 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
if (snap == NULL) {
virDomainSnapshotDefFree(def);
} else if (cur) {
+ if (current)
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Too many snapshots claiming to be current for domain %s"),
+ vm->def->name);
current = snap;
- if (!vm->current_snapshot)
- vm->current_snapshot = snap;
}
VIR_FREE(fullpath);
@@ -495,13 +497,7 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
_("Failed to fully read directory %s"),
snapDir);
- if (vm->current_snapshot != current) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Too many snapshots claiming to be current for domain %s"),
- vm->def->name);
- vm->current_snapshot = NULL;
- }
-
+ virDomainSnapshotSetCurrent(vm->snapshots, current);
if (virDomainSnapshotUpdateRelations(vm->snapshots) < 0)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Snapshots have inconsistent relations for domain %s"),
@@ -15858,13 +15854,13 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
def = NULL;
}
- current = vm->current_snapshot;
+ current = virDomainSnapshotGetCurrent(vm->snapshots);
if (current) {
if (!redefine &&
VIR_STRDUP(snap->def->parent, current->def->name) < 0)
goto endjob;
if (update_current) {
- vm->current_snapshot = NULL;
+ virDomainSnapshotSetCurrent(vm->snapshots, NULL);
if (qemuDomainSnapshotWriteMetadata(vm, current,
driver->caps, driver->xmlopt,
cfg->snapshotDir) < 0)
@@ -15915,7 +15911,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
endjob:
if (snapshot && !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA)) {
if (update_current)
- vm->current_snapshot = snap;
+ virDomainSnapshotSetCurrent(vm->snapshots, snap);
if (qemuDomainSnapshotWriteMetadata(vm, snap, driver->caps,
driver->xmlopt,
cfg->snapshotDir) < 0) {
@@ -15927,7 +15923,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
_("unable to save metadata for snapshot %s"),
snap->def->name);
virDomainSnapshotObjListRemove(vm->snapshots, snap);
- vm->current_snapshot = NULL;
} else {
other = virDomainSnapshotFindByName(vm->snapshots,
snap->def->parent);
@@ -16166,7 +16161,7 @@ qemuDomainHasCurrentSnapshot(virDomainPtr domain,
if (virDomainHasCurrentSnapshotEnsureACL(domain->conn, vm->def) < 0)
goto cleanup;
- ret = (vm->current_snapshot != NULL);
+ ret = (virDomainSnapshotGetCurrent(vm->snapshots) != NULL);
cleanup:
virDomainObjEndAPI(&vm);
@@ -16223,13 +16218,13 @@ qemuDomainSnapshotCurrent(virDomainPtr domain,
if (virDomainSnapshotCurrentEnsureACL(domain->conn, vm->def) < 0)
goto cleanup;
- if (!vm->current_snapshot) {
+ if (!virDomainSnapshotGetCurrent(vm->snapshots)) {
virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, "%s",
_("the domain does not have a current snapshot"));
goto cleanup;
}
- snapshot = virGetDomainSnapshot(domain, vm->current_snapshot->def->name);
+ snapshot = virGetDomainSnapshot(domain, virDomainSnapshotGetCurrentName(vm->snapshots));
cleanup:
virDomainObjEndAPI(&vm);
@@ -16289,8 +16284,7 @@ qemuDomainSnapshotIsCurrent(virDomainSnapshotPtr snapshot,
if (!(snap = qemuSnapObjFromSnapshot(vm, snapshot)))
goto cleanup;
- ret = (vm->current_snapshot &&
- STREQ(virSnapName(snapshot), vm->current_snapshot->def->name));
+ ret = virDomainSnapshotIsCurrentName(vm->snapshots, virSnapName(snapshot));
cleanup:
virDomainObjEndAPI(&vm);
@@ -16443,14 +16437,14 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
}
}
- current = vm->current_snapshot;
+ current = virDomainSnapshotGetCurrent(vm->snapshots);
if (current) {
- vm->current_snapshot = NULL;
+ virDomainSnapshotSetCurrent(vm->snapshots, NULL);
if (qemuDomainSnapshotWriteMetadata(vm, current,
driver->caps, driver->xmlopt,
cfg->snapshotDir) < 0)
goto endjob;
- /* XXX Should we restore vm->current_snapshot after this point
+ /* XXX Should we restore the current snapshot after this point
* in the failure cases where we know there was no change? */
}
@@ -16459,7 +16453,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
*
* XXX Should domain snapshots track live xml rather
* than inactive xml? */
- vm->current_snapshot = snap;
if (snap->def->dom) {
config = virDomainDefCopy(snap->def->dom, caps,
driver->xmlopt, NULL, true);
@@ -16730,15 +16723,15 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
cleanup:
if (ret == 0) {
- vm->current_snapshot = snap;
+ virDomainSnapshotSetCurrent(vm->snapshots, snap);
if (qemuDomainSnapshotWriteMetadata(vm, snap, driver->caps,
driver->xmlopt,
cfg->snapshotDir) < 0) {
- vm->current_snapshot = NULL;
+ virDomainSnapshotSetCurrent(vm->snapshots, NULL);
ret = -1;
}
} else if (snap) {
- vm->current_snapshot = NULL;
+ virDomainSnapshotSetCurrent(vm->snapshots, NULL);
}
if (ret == 0 && config && vm->persistent &&
!(ret = virDomainSaveConfig(cfg->configDir, driver->caps,
@@ -16866,7 +16859,7 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
if (rem.err < 0)
goto endjob;
if (rem.current) {
- vm->current_snapshot = snap;
+ virDomainSnapshotSetCurrent(vm->snapshots, snap);
if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) {
if (qemuDomainSnapshotWriteMetadata(vm, snap, driver->caps,
driver->xmlopt,
@@ -16874,7 +16867,7 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to set snapshot '%s' as current"),
snap->def->name);
- vm->current_snapshot = NULL;
+ virDomainSnapshotSetCurrent(vm->snapshots, NULL);
goto endjob;
}
}
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index ca480c1b21..9cbef70f1c 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -845,13 +845,13 @@ testParseDomainSnapshots(testDriverPtr privconn,
}
if (cur) {
- if (domobj->current_snapshot) {
+ if (virDomainSnapshotGetCurrent(domobj->snapshots)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("more than one snapshot claims to be active"));
goto error;
}
- domobj->current_snapshot = snap;
+ virDomainSnapshotSetCurrent(domobj->snapshots, snap);
}
}
@@ -6151,7 +6151,7 @@ testDomainHasCurrentSnapshot(virDomainPtr domain,
if (!(vm = testDomObjFromDomain(domain)))
return -1;
- ret = (vm->current_snapshot != NULL);
+ ret = (virDomainSnapshotGetCurrent(vm->snapshots) != NULL);
virDomainObjEndAPI(&vm);
return ret;
@@ -6193,19 +6193,21 @@ testDomainSnapshotCurrent(virDomainPtr domain,
{
virDomainObjPtr vm;
virDomainSnapshotPtr snapshot = NULL;
+ virDomainSnapshotObjPtr current;
virCheckFlags(0, NULL);
if (!(vm = testDomObjFromDomain(domain)))
return NULL;
- if (!vm->current_snapshot) {
+ current = virDomainSnapshotGetCurrent(vm->snapshots);
+ if (!current) {
virReportError(VIR_ERR_NO_DOMAIN_SNAPSHOT, "%s",
_("the domain does not have a current snapshot"));
goto cleanup;
}
- snapshot = virGetDomainSnapshot(domain, vm->current_snapshot->def->name);
+ snapshot = virGetDomainSnapshot(domain, current->def->name);
cleanup:
virDomainObjEndAPI(&vm);
@@ -6253,8 +6255,7 @@ testDomainSnapshotIsCurrent(virDomainSnapshotPtr snapshot,
if (!(vm = testDomObjFromSnapshot(snapshot)))
return -1;
- ret = (vm->current_snapshot &&
- STREQ(virSnapName(snapshot), vm->current_snapshot->def->name));
+ ret = virDomainSnapshotIsCurrentName(vm->snapshots, virSnapName(snapshot));
virDomainObjEndAPI(&vm);
return ret;
@@ -6393,9 +6394,8 @@ testDomainSnapshotCreateXML(virDomainPtr domain,
}
if (!redefine) {
- if (vm->current_snapshot &&
- (VIR_STRDUP(snap->def->parent,
- vm->current_snapshot->def->name) < 0))
+ if (VIR_STRDUP(snap->def->parent,
+ virDomainSnapshotGetCurrentName(vm->snapshots)) < 0)
goto cleanup;
if ((flags & VIR_DOMAIN_SNAPSHOT_CREATE_HALT) &&
@@ -6413,7 +6413,7 @@ testDomainSnapshotCreateXML(virDomainPtr domain,
if (snapshot) {
virDomainSnapshotObjPtr other;
if (update_current)
- vm->current_snapshot = snap;
+ virDomainSnapshotSetCurrent(vm->snapshots, snap);
other = virDomainSnapshotFindByName(vm->snapshots,
snap->def->parent);
snap->parent = other;
@@ -6444,9 +6444,7 @@ testDomainSnapshotDiscardAll(void *payload,
virDomainSnapshotObjPtr snap = payload;
testSnapRemoveDataPtr curr = data;
- if (curr->vm->current_snapshot == snap)
- curr->current = true;
- virDomainSnapshotObjListRemove(curr->vm->snapshots, snap);
+ curr->current |= virDomainSnapshotObjListRemove(curr->vm->snapshots, snap);
return 0;
}
@@ -6511,7 +6509,7 @@ testDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
testDomainSnapshotDiscardAll,
&rem);
if (rem.current)
- vm->current_snapshot = snap;
+ virDomainSnapshotSetCurrent(vm->snapshots, snap);
} else if (snap->nchildren) {
testSnapReparentData rep;
rep.parent = snap->parent;
@@ -6535,7 +6533,7 @@ testDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
snap->first_child = NULL;
} else {
virDomainSnapshotDropParent(snap);
- if (snap == vm->current_snapshot) {
+ if (snap == virDomainSnapshotGetCurrent(vm->snapshots)) {
if (snap->def->parent) {
parentsnap = virDomainSnapshotFindByName(vm->snapshots,
snap->def->parent);
@@ -6543,7 +6541,7 @@ testDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
VIR_WARN("missing parent snapshot matching name '%s'",
snap->def->parent);
}
- vm->current_snapshot = parentsnap;
+ virDomainSnapshotSetCurrent(vm->snapshots, parentsnap);
}
virDomainSnapshotObjListRemove(vm->snapshots, snap);
}
@@ -6619,9 +6617,7 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
}
}
-
- if (vm->current_snapshot)
- vm->current_snapshot = NULL;
+ virDomainSnapshotSetCurrent(vm->snapshots, NULL);
config = virDomainDefCopy(snap->def->dom, privconn->caps,
privconn->xmlopt, NULL, true);
@@ -6746,7 +6742,7 @@ testDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
}
}
- vm->current_snapshot = snap;
+ virDomainSnapshotSetCurrent(vm->snapshots, snap);
ret = 0;
cleanup:
if (event) {
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 3/20/19 1:40 AM, Eric Blake wrote:
> It is easier to track the current snapshot as part of the list of
> snapshots. In particular, doing so lets us guarantee that the current
> snapshot is cleared if that snapshot is removed from the list (rather
> than depending on the caller to do so, and risking a use-after-free
> problem). This requires the addition of several new accessor
> functions.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> src/conf/domain_conf.h | 1 -
> src/conf/virdomainsnapshotobjlist.h | 10 +++--
> src/conf/snapshot_conf.c | 4 +-
> src/conf/virdomainsnapshotobjlist.c | 63 ++++++++++++++++++++++++-----
> src/libvirt_private.syms | 4 ++
> src/qemu/qemu_domain.c | 14 +++----
> src/qemu/qemu_driver.c | 47 +++++++++------------
> src/test/test_driver.c | 38 ++++++++---------
> 8 files changed, 109 insertions(+), 72 deletions(-)
>
[...]
> diff --git a/src/conf/virdomainsnapshotobjlist.c b/src/conf/virdomainsnapshotobjlist.c
> index be44bdde71..1eecb89a5d 100644
> --- a/src/conf/virdomainsnapshotobjlist.c
> +++ b/src/conf/virdomainsnapshotobjlist.c
[...]
> +
> +
> +/* Remove snapshot from the list; return true if it was current */
> +bool virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots,
> virDomainSnapshotObjPtr snapshot)
NIT: One of these things is not like the others ;-)
bool
virDomain...
> {
> + bool ret = snapshots->current == snapshot;
> virHashRemoveEntry(snapshots->objs, snapshot->def->name);
> + if (ret)
> + snapshots->current = NULL;
Slick, this is how testDomainSnapshotDiscardAll can alter it's logic.
Took me until the end of the patch to find ;-)... and coverity didn't
whine about one function checking return while the others don't.
> + return ret;
> }
>
[...]
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 7e0e76a31a..6c71382b93 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -482,9 +482,11 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
> if (snap == NULL) {
> virDomainSnapshotDefFree(def);
> } else if (cur) {
> + if (current)
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Too many snapshots claiming to be current for domain %s"),
> + vm->def->name);
Even though we generate this message we go ahead and update @current to
@snap. Should this be an "if (current) ... else ... " ?
Additionally if someone's really AFU'd, they could get more than one
message; whereas, previously they'd only get one such message.
> current = snap;
> - if (!vm->current_snapshot)
> - vm->current_snapshot = snap;
> }
>
> VIR_FREE(fullpath);
> @@ -495,13 +497,7 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
> _("Failed to fully read directory %s"),
> snapDir);
>
> - if (vm->current_snapshot != current) {
> - virReportError(VIR_ERR_INTERNAL_ERROR,
> - _("Too many snapshots claiming to be current for domain %s"),
> - vm->def->name);
> - vm->current_snapshot = NULL;
Previously if this happened, then current was set to NULL, but the
following will set it to the last snap declared to be current.
Is that expected? If not, then perhaps the if (current) above needs to
add a "current = NULL;" along with the error message. Of course that
leads to the possibility of others declaring themselves current and
possibly having multiple errors splatted. Only seems to matter for
someone running debug or looking at debug logs since we don't fail.
BTW: This is one of those current gray areas of making two changes in
one patch. One change being the usage of the accessors and the other
being the alteration of when this message gets splatted.
> - }
> -
> + virDomainSnapshotSetCurrent(vm->snapshots, current);
> if (virDomainSnapshotUpdateRelations(vm->snapshots) < 0)
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Snapshots have inconsistent relations for domain %s"),
[...]
> @@ -15927,7 +15923,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
> _("unable to save metadata for snapshot %s"),
> snap->def->name);
> virDomainSnapshotObjListRemove(vm->snapshots, snap);
> - vm->current_snapshot = NULL;
virDomainSnapshotSetCurrent(vm->snapshots, NULL);
right?
> } else {
> other = virDomainSnapshotFindByName(vm->snapshots,
> snap->def->parent);
[...]
>
> @@ -16459,7 +16453,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
> *
> * XXX Should domain snapshots track live xml rather
> * than inactive xml? */
> - vm->current_snapshot = snap;
virDomainSnapshotSetCurrent(vm->snapshots, snap); ?
> if (snap->def->dom) {
> config = virDomainDefCopy(snap->def->dom, caps,
> driver->xmlopt, NULL, true);
[...]
The comments in qemuDomainSnapshotLoad aren't showstoppers. I assume you
can answer and things will be fine.
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 3/20/19 3:39 PM, John Ferlan wrote:
>
>
> On 3/20/19 1:40 AM, Eric Blake wrote:
>> It is easier to track the current snapshot as part of the list of
>> snapshots. In particular, doing so lets us guarantee that the current
>> snapshot is cleared if that snapshot is removed from the list (rather
>> than depending on the caller to do so, and risking a use-after-free
>> problem). This requires the addition of several new accessor
>> functions.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>> +
>> +/* Remove snapshot from the list; return true if it was current */
>> +bool virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots,
>> virDomainSnapshotObjPtr snapshot)
>
> NIT: One of these things is not like the others ;-)
>
> bool
> virDomain...
I need to quit sending patches at midnight.
>
>> {
>> + bool ret = snapshots->current == snapshot;
>> virHashRemoveEntry(snapshots->objs, snapshot->def->name);
>> + if (ret)
>> + snapshots->current = NULL;
>
> Slick, this is how testDomainSnapshotDiscardAll can alter it's logic.
> Took me until the end of the patch to find ;-)... and coverity didn't
> whine about one function checking return while the others don't.
>
>> + return ret;
You noticed that this clears the current snapshot... [1]
>> }
>>
>
> [...]
>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 7e0e76a31a..6c71382b93 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -482,9 +482,11 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
>> if (snap == NULL) {
>> virDomainSnapshotDefFree(def);
>> } else if (cur) {
>> + if (current)
>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>> + _("Too many snapshots claiming to be current for domain %s"),
>> + vm->def->name);
>
> Even though we generate this message we go ahead and update @current to
> @snap. Should this be an "if (current) ... else ... " ?
>
> Additionally if someone's really AFU'd, they could get more than one
> message; whereas, previously they'd only get one such message.
It's a tough call. Anyone messing around with
/var/lib/libvrt/qemu/snapshot/$dom to trigger the problem in the first
place is already unsupported territory, where who knows what libvirt
will do with their garbage. Maybe I should just do a standalone patch
that quits trying to play nice (by merely setting no current snapshot
after a warning) and instead hard-fail the loading of any more
snapshots. (After all, I have a patch in the pipeline that does a bulk
load, and THAT patch refuses to load ANY snapshots if the xml has been
modified incorrectly behind libvirt's back, rather than trying to play
nice and still load as many snapshots as possible).
>> - if (vm->current_snapshot != current) {
>> - virReportError(VIR_ERR_INTERNAL_ERROR,
>> - _("Too many snapshots claiming to be current for domain %s"),
>> - vm->def->name);
>> - vm->current_snapshot = NULL;
>
> Previously if this happened, then current was set to NULL, but the
> following will set it to the last snap declared to be current.
>
> Is that expected? If not, then perhaps the if (current) above needs to
> add a "current = NULL;" along with the error message. Of course that
> leads to the possibility of others declaring themselves current and
> possibly having multiple errors splatted.
And getting different behavior depending on whether the user had an even
or odd number of domains claiming to be current.
> Only seems to matter for
> someone running debug or looking at debug logs since we don't fail.
>
> BTW: This is one of those current gray areas of making two changes in
> one patch. One change being the usage of the accessors and the other
> being the alteration of when this message gets splatted.
Okay, you've convinced me to try and split it.
>> @@ -15927,7 +15923,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
>> _("unable to save metadata for snapshot %s"),
>> snap->def->name);
>> virDomainSnapshotObjListRemove(vm->snapshots, snap);
>> - vm->current_snapshot = NULL;
>
> virDomainSnapshotSetCurrent(vm->snapshots, NULL);
>
> right?
Not needed - virDomainSnapshotObjListRemove() takes care of it, back at [1].
>
>> } else {
>> other = virDomainSnapshotFindByName(vm->snapshots,
>> snap->def->parent);
>
> [...]
>
>>
>> @@ -16459,7 +16453,6 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
>> *
>> * XXX Should domain snapshots track live xml rather
>> * than inactive xml? */
>> - vm->current_snapshot = snap;
>
> virDomainSnapshotSetCurrent(vm->snapshots, snap); ?
This one's trickier, but still, not needed: look at the cleanup: label,
which calls virDomainSnapshotSetCurrent(vm->snapshots, snap); on success.
>
>> if (snap->def->dom) {
>> config = virDomainDefCopy(snap->def->dom, caps,
>> driver->xmlopt, NULL, true);
>
> [...]
>
> The comments in qemuDomainSnapshotLoad aren't showstoppers. I assume you
> can answer and things will be fine.
>
> Reviewed-by: John Ferlan <jferlan@redhat.com>
>
> John
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 3/20/19 4:03 PM, Eric Blake wrote:
> On 3/20/19 3:39 PM, John Ferlan wrote:
>>
>>
>> On 3/20/19 1:40 AM, Eric Blake wrote:
>>> It is easier to track the current snapshot as part of the list of
>>> snapshots. In particular, doing so lets us guarantee that the current
>>> snapshot is cleared if that snapshot is removed from the list (rather
>>> than depending on the caller to do so, and risking a use-after-free
>>> problem). This requires the addition of several new accessor
>>> functions.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>> ---
>
>>> +++ b/src/qemu/qemu_driver.c
>>> @@ -482,9 +482,11 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
>>> if (snap == NULL) {
>>> virDomainSnapshotDefFree(def);
>>> } else if (cur) {
>>> + if (current)
>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>> + _("Too many snapshots claiming to be current for domain %s"),
>>> + vm->def->name);
>>
>> Even though we generate this message we go ahead and update @current to
>> @snap. Should this be an "if (current) ... else ... " ?
>>
>> Additionally if someone's really AFU'd, they could get more than one
>> message; whereas, previously they'd only get one such message.
>
> It's a tough call. Anyone messing around with
> /var/lib/libvrt/qemu/snapshot/$dom to trigger the problem in the first
> place is already unsupported territory, where who knows what libvirt
> will do with their garbage. Maybe I should just do a standalone patch
> that quits trying to play nice (by merely setting no current snapshot
> after a warning) and instead hard-fail the loading of any more
> snapshots. (After all, I have a patch in the pipeline that does a bulk
> load, and THAT patch refuses to load ANY snapshots if the xml has been
> modified incorrectly behind libvirt's back, rather than trying to play
> nice and still load as many snapshots as possible).
>> BTW: This is one of those current gray areas of making two changes in
>> one patch. One change being the usage of the accessors and the other
>> being the alteration of when this message gets splatted.
>
> Okay, you've convinced me to try and split it.
I'm posting what I split out for the logic change (where I had fun
writing the commit message);
>> The comments in qemuDomainSnapshotLoad aren't showstoppers. I assume you
>> can answer and things will be fine.
>>
>> Reviewed-by: John Ferlan <jferlan@redhat.com>
and I'm going to be bold and apply your R-b to both halves of the split
rather than make you re-read it in a v2.
From 1a54a57381704f09491c80a0ef54b43b7b582bb7 Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Thu, 21 Mar 2019 12:16:03 -0500
Subject: [PATCH] snapshot: Rework parse logic during libvirt startup
Rework the logic in qemuDomainSnapshotLoad() to set
vm->current_snapshot only once at the end of the loop, rather than
repeatedly querying it during the loop, to make it easier for the next
patch to use accessor functions rather than direct manipulation of
vm->current_snapshot. When encountering multiple snapshots claiming
to be current (based on the presence of an <active>1</active> element
in the XML, which libvirt only outputs for internal use and not for
any public API), this changes behavior from warning only once and
running with no current snapshot, to instead warning on each duplicate
and selecting the last one encountered (which is arbitrary based on
readdir() ordering, but actually stands a fair chance of being the
most-recently created snapshot whether by timestamp or by the
propensity of humans to name things in ascending order).
Note that the code in question is only run by libvirtd when it first
starts, reading state from disk from the previous run into memory for
this run. Since the data resides somewhere that only libvirt should be
touching (typically /var/lib/libvirt/qemu/snapshot/*), it should be
clean. So in the common case, the code touched here is unreachable.
But if someone is actually messing with files behind libvirt's back,
they deserve the change in behavior.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
---
src/qemu/qemu_driver.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 13a34ef860..e49924ce1a 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1,7 +1,7 @@
/*
* qemu_driver.c: core driver methods for managing qemu guests
*
- * Copyright (C) 2006-2016 Red Hat, Inc.
+ * Copyright (C) 2006-2019 Red Hat, Inc.
* Copyright (C) 2006 Daniel P. Berrange
*
* This library is free software; you can redistribute it and/or
@@ -482,9 +482,11 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
if (snap == NULL) {
virDomainSnapshotDefFree(def);
} else if (cur) {
+ if (current)
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Too many snapshots claiming to be
current for domain %s"),
+ vm->def->name);
current = snap;
- if (!vm->current_snapshot)
- vm->current_snapshot = snap;
}
VIR_FREE(fullpath);
@@ -495,13 +497,7 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
_("Failed to fully read directory %s"),
snapDir);
- if (vm->current_snapshot != current) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Too many snapshots claiming to be current for
domain %s"),
- vm->def->name);
- vm->current_snapshot = NULL;
- }
-
+ vm->current_snapshot = current;
if (virDomainSnapshotUpdateRelations(vm->snapshots) < 0)
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Snapshots have inconsistent relations for
domain %s"),
--
2.20.1
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 3/21/19 3:58 PM, Eric Blake wrote:
> On 3/20/19 4:03 PM, Eric Blake wrote:
>> On 3/20/19 3:39 PM, John Ferlan wrote:
>>>
>>>
>>> On 3/20/19 1:40 AM, Eric Blake wrote:
>>>> It is easier to track the current snapshot as part of the list of
>>>> snapshots. In particular, doing so lets us guarantee that the current
>>>> snapshot is cleared if that snapshot is removed from the list (rather
>>>> than depending on the caller to do so, and risking a use-after-free
>>>> problem). This requires the addition of several new accessor
>>>> functions.
>>>>
>>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>>> ---
>>
>
>>>> +++ b/src/qemu/qemu_driver.c
>>>> @@ -482,9 +482,11 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm,
>>>> if (snap == NULL) {
>>>> virDomainSnapshotDefFree(def);
>>>> } else if (cur) {
>>>> + if (current)
>>>> + virReportError(VIR_ERR_INTERNAL_ERROR,
>>>> + _("Too many snapshots claiming to be current for domain %s"),
>>>> + vm->def->name);
>>>
>>> Even though we generate this message we go ahead and update @current to
>>> @snap. Should this be an "if (current) ... else ... " ?
>>>
>>> Additionally if someone's really AFU'd, they could get more than one
>>> message; whereas, previously they'd only get one such message.
>>
>> It's a tough call. Anyone messing around with
>> /var/lib/libvrt/qemu/snapshot/$dom to trigger the problem in the first
>> place is already unsupported territory, where who knows what libvirt
>> will do with their garbage. Maybe I should just do a standalone patch
>> that quits trying to play nice (by merely setting no current snapshot
>> after a warning) and instead hard-fail the loading of any more
>> snapshots. (After all, I have a patch in the pipeline that does a bulk
>> load, and THAT patch refuses to load ANY snapshots if the xml has been
>> modified incorrectly behind libvirt's back, rather than trying to play
>> nice and still load as many snapshots as possible).
>
>
>>> BTW: This is one of those current gray areas of making two changes in
>>> one patch. One change being the usage of the accessors and the other
>>> being the alteration of when this message gets splatted.
>>
>> Okay, you've convinced me to try and split it.
>
> I'm posting what I split out for the logic change (where I had fun
> writing the commit message);
>
>>> The comments in qemuDomainSnapshotLoad aren't showstoppers. I assume you
>>> can answer and things will be fine.
>>>
>>> Reviewed-by: John Ferlan <jferlan@redhat.com>
>
> and I'm going to be bold and apply your R-b to both halves of the split
> rather than make you re-read it in a v2.
That's fine.
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.