12:40 a.m.
Rather than allowing a leaky abstraction where multiple drivers have
to open-code operations that update the relations in a
virDomainSnapshotObjList, it is better to add accessor functions so
that updates to relations are maintained closer to the internals. The
goal here is to avoid access to, nchildren, first_child, or sibling
outside of the lowest level functions, making it easier to refactor
later on. While many of the conversions are fairly straightforward,
the MoveChildren refactoring can be a bit interesting to follow.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/conf/virdomainsnapshotobj.h | 5 ++++
src/conf/virdomainsnapshotobjlist.h | 2 ++
src/conf/virdomainsnapshotobj.c | 42 +++++++++++++++++++++++++++++
src/conf/virdomainsnapshotobjlist.c | 42 ++++++++++++++++++-----------
src/libvirt_private.syms | 3 +++
src/qemu/qemu_driver.c | 19 +++----------
src/test/test_driver.c | 18 +++----------
7 files changed, 85 insertions(+), 46 deletions(-)
diff --git a/src/conf/virdomainsnapshotobj.h b/src/conf/virdomainsnapshotobj.h
index 957f1b2ea8..0981ea4c25 100644
--- a/src/conf/virdomainsnapshotobj.h
+++ b/src/conf/virdomainsnapshotobj.h
@@ -46,5 +46,10 @@ int virDomainSnapshotForEachDescendant(virDomainSnapshotObjPtr
snapshot,
virHashIterator iter,
void *data);
void virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot);
+void virDomainSnapshotDropChildren(virDomainSnapshotObjPtr snapshot);
+void virDomainSnapshotMoveChildren(virDomainSnapshotObjPtr from,
+ virDomainSnapshotObjPtr to);
+void virDomainSnapshotSetParent(virDomainSnapshotObjPtr snapshot,
+ virDomainSnapshotObjPtr parent);
#endif /* LIBVIRT_VIRDOMAINSNAPSHOTOBJ_H */
diff --git a/src/conf/virdomainsnapshotobjlist.h b/src/conf/virdomainsnapshotobjlist.h
index e210849441..c13a0b4026 100644
--- a/src/conf/virdomainsnapshotobjlist.h
+++ b/src/conf/virdomainsnapshotobjlist.h
@@ -55,6 +55,7 @@ int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots,
unsigned int flags);
virDomainSnapshotObjPtr virDomainSnapshotFindByName(virDomainSnapshotObjListPtr
snapshots,
const char *name);
+int virDomainSnapshotObjListSize(virDomainSnapshotObjListPtr snapshots);
virDomainSnapshotObjPtr virDomainSnapshotGetCurrent(virDomainSnapshotObjListPtr
snapshots);
const char *virDomainSnapshotGetCurrentName(virDomainSnapshotObjListPtr snapshots);
bool virDomainSnapshotIsCurrentName(virDomainSnapshotObjListPtr snapshots,
@@ -63,6 +64,7 @@ void virDomainSnapshotSetCurrent(virDomainSnapshotObjListPtr snapshots,
virDomainSnapshotObjPtr snapshot);
bool virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots,
virDomainSnapshotObjPtr snapshot);
+void virDomainSnapshotObjListRemoveAll(virDomainSnapshotObjListPtr snapshots);
int virDomainSnapshotForEach(virDomainSnapshotObjListPtr snapshots,
virHashIterator iter,
void *data);
diff --git a/src/conf/virdomainsnapshotobj.c b/src/conf/virdomainsnapshotobj.c
index 7f92ac21d9..d6b216c7b2 100644
--- a/src/conf/virdomainsnapshotobj.c
+++ b/src/conf/virdomainsnapshotobj.c
@@ -121,3 +121,45 @@ virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot)
snapshot->parent = NULL;
snapshot->sibling = NULL;
}
+
+
+/* Update @snapshot to no longer have children. */
+void
+virDomainSnapshotDropChildren(virDomainSnapshotObjPtr snapshot)
+{
+ snapshot->nchildren = 0;
+ snapshot->first_child = NULL;
+}
+
+
+/* Add @snapshot to @parent's list of children. */
+void
+virDomainSnapshotSetParent(virDomainSnapshotObjPtr snapshot,
+ virDomainSnapshotObjPtr parent)
+{
+ snapshot->parent = parent;
+ parent->nchildren++;
+ snapshot->sibling = parent->first_child;
+ parent->first_child = snapshot;
+}
+
+
+/* Take all children of @from and convert them into children of @to. */
+void
+virDomainSnapshotMoveChildren(virDomainSnapshotObjPtr from,
+ virDomainSnapshotObjPtr to)
+{
+ virDomainSnapshotObjPtr child;
+ virDomainSnapshotObjPtr last;
+
+ for (child = from->first_child; child; child = child->sibling) {
+ child->parent = to;
+ if (!child->sibling)
+ last = child;
+ }
+ to->nchildren += from->nchildren;
+ last->sibling = to->first_child;
+ to->first_child = from->first_child;
+ from->nchildren = 0;
+ from->first_child = NULL;
+}
diff --git a/src/conf/virdomainsnapshotobjlist.c b/src/conf/virdomainsnapshotobjlist.c
index 1eecb89a5d..9538521ab3 100644
--- a/src/conf/virdomainsnapshotobjlist.c
+++ b/src/conf/virdomainsnapshotobjlist.c
@@ -72,7 +72,7 @@ virDomainSnapshotObjListParse(const char *xmlStr,
_("incorrect flags for bulk parse"));
return -1;
}
- if (snapshots->metaroot.nchildren || snapshots->current) {
+ if (virDomainSnapshotObjListSize(snapshots)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("bulk define of snapshots only possible with "
"no existing snapshot"));
@@ -143,9 +143,7 @@ virDomainSnapshotObjListParse(const char *xmlStr,
if (ret < 0) {
/* There were no snapshots before this call; so on error, just
* blindly delete anything created before the failure. */
- virHashRemoveAll(snapshots->objs);
- snapshots->metaroot.nchildren = 0;
- snapshots->metaroot.first_child = NULL;
+ virDomainSnapshotObjListRemoveAll(snapshots);
}
xmlXPathFreeContext(ctxt);
xmlFreeDoc(xml);
@@ -437,6 +435,14 @@ virDomainSnapshotFindByName(virDomainSnapshotObjListPtr snapshots,
}
+/* Return the number of objects currently in the list */
+int
+virDomainSnapshotObjListSize(virDomainSnapshotObjListPtr snapshots)
+{
+ return virHashSize(snapshots->objs);
+}
+
+
/* Return the current snapshot, or NULL */
virDomainSnapshotObjPtr
virDomainSnapshotGetCurrent(virDomainSnapshotObjListPtr snapshots)
@@ -484,6 +490,15 @@ bool virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr
snapshots,
return ret;
}
+/* Remove all snapshots tracked in the list */
+void
+virDomainSnapshotObjListRemoveAll(virDomainSnapshotObjListPtr snapshots)
+{
+ virHashRemoveAll(snapshots->objs);
+ virDomainSnapshotDropChildren(&snapshots->metaroot);
+}
+
+
int
virDomainSnapshotForEach(virDomainSnapshotObjListPtr snapshots,
virHashIterator iter,
@@ -511,28 +526,26 @@ virDomainSnapshotSetRelations(void *payload,
virDomainSnapshotObjPtr obj = payload;
struct snapshot_set_relation *curr = data;
virDomainSnapshotObjPtr tmp;
+ virDomainSnapshotObjPtr parent;
- obj->parent = virDomainSnapshotFindByName(curr->snapshots,
- obj->def->parent);
- if (!obj->parent) {
+ parent = virDomainSnapshotFindByName(curr->snapshots, obj->def->parent);
+ if (!parent) {
curr->err = -1;
- obj->parent = &curr->snapshots->metaroot;
+ parent = &curr->snapshots->metaroot;
VIR_WARN("snapshot %s lacks parent", obj->def->name);
} else {
- tmp = obj->parent;
+ tmp = parent;
while (tmp && tmp->def) {
if (tmp == obj) {
curr->err = -1;
- obj->parent = &curr->snapshots->metaroot;
+ parent = &curr->snapshots->metaroot;
VIR_WARN("snapshot %s in circular chain",
obj->def->name);
break;
}
tmp = tmp->parent;
}
}
- obj->parent->nchildren++;
- obj->sibling = obj->parent->first_child;
- obj->parent->first_child = obj;
+ virDomainSnapshotSetParent(obj, parent);
return 0;
}
@@ -545,8 +558,7 @@ virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr
snapshots)
{
struct snapshot_set_relation act = { snapshots, 0 };
- snapshots->metaroot.nchildren = 0;
- snapshots->metaroot.first_child = NULL;
+ virDomainSnapshotDropChildren(&snapshots->metaroot);
virHashForEach(snapshots->objs, virDomainSnapshotSetRelations, &act);
if (act.err)
snapshots->current = NULL;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 72c5cef528..ffc1724850 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -980,9 +980,12 @@ virDomainObjListRename;
# conf/virdomainsnapshotobj.h
+virDomainSnapshotDropChildren;
virDomainSnapshotDropParent;
virDomainSnapshotForEachChild;
virDomainSnapshotForEachDescendant;
+virDomainSnapshotMoveChildren;
+virDomainSnapshotSetParent;
# conf/virdomainsnapshotobjlist.h
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6c71382b93..eb3d112b69 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15926,10 +15926,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
} else {
other = virDomainSnapshotFindByName(vm->snapshots,
snap->def->parent);
- snap->parent = other;
- other->nchildren++;
- snap->sibling = other->first_child;
- other->first_child = snap;
+ virDomainSnapshotSetParent(snap, other);
}
} else if (snap) {
virDomainSnapshotObjListRemove(vm->snapshots, snap);
@@ -16763,7 +16760,6 @@ struct _virQEMUSnapReparent {
virCapsPtr caps;
virDomainXMLOptionPtr xmlopt;
int err;
- virDomainSnapshotObjPtr last;
};
@@ -16779,7 +16775,6 @@ qemuDomainSnapshotReparentChildren(void *payload,
return 0;
VIR_FREE(snap->def->parent);
- snap->parent = rep->parent;
if (rep->parent->def &&
VIR_STRDUP(snap->def->parent, rep->parent->def->name) < 0) {
@@ -16787,9 +16782,6 @@ qemuDomainSnapshotReparentChildren(void *payload,
return 0;
}
- if (!snap->sibling)
- rep->last = snap;
-
rep->err = qemuDomainSnapshotWriteMetadata(rep->vm, snap,
rep->caps, rep->xmlopt,
rep->cfg->snapshotDir);
@@ -16877,7 +16869,6 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
rep.parent = snap->parent;
rep.vm = vm;
rep.err = 0;
- rep.last = NULL;
rep.caps = driver->caps;
rep.xmlopt = driver->xmlopt;
virDomainSnapshotForEachChild(snap,
@@ -16885,15 +16876,11 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
&rep);
if (rep.err < 0)
goto endjob;
- /* Can't modify siblings during ForEachChild, so do it now. */
- snap->parent->nchildren += snap->nchildren;
- rep.last->sibling = snap->parent->first_child;
- snap->parent->first_child = snap->first_child;
+ virDomainSnapshotMoveChildren(snap, snap->parent);
}
if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) {
- snap->nchildren = 0;
- snap->first_child = NULL;
+ virDomainSnapshotDropChildren(snap);
ret = 0;
} else {
ret = qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only);
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 9cbef70f1c..d3b76bfdbd 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -6416,10 +6416,7 @@ testDomainSnapshotCreateXML(virDomainPtr domain,
virDomainSnapshotSetCurrent(vm->snapshots, snap);
other = virDomainSnapshotFindByName(vm->snapshots,
snap->def->parent);
- snap->parent = other;
- other->nchildren++;
- snap->sibling = other->first_child;
- other->first_child = snap;
+ virDomainSnapshotSetParent(snap, other);
}
virDomainObjEndAPI(&vm);
}
@@ -6454,7 +6451,6 @@ struct _testSnapReparentData {
virDomainSnapshotObjPtr parent;
virDomainObjPtr vm;
int err;
- virDomainSnapshotObjPtr last;
};
static int
@@ -6469,7 +6465,6 @@ testDomainSnapshotReparentChildren(void *payload,
return 0;
VIR_FREE(snap->def->parent);
- snap->parent = rep->parent;
if (rep->parent->def &&
VIR_STRDUP(snap->def->parent, rep->parent->def->name) < 0) {
@@ -6477,8 +6472,6 @@ testDomainSnapshotReparentChildren(void *payload,
return 0;
}
- if (!snap->sibling)
- rep->last = snap;
return 0;
}
@@ -6515,22 +6508,17 @@ testDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
rep.parent = snap->parent;
rep.vm = vm;
rep.err = 0;
- rep.last = NULL;
virDomainSnapshotForEachChild(snap,
testDomainSnapshotReparentChildren,
&rep);
if (rep.err < 0)
goto cleanup;
- /* Can't modify siblings during ForEachChild, so do it now. */
- snap->parent->nchildren += snap->nchildren;
- rep.last->sibling = snap->parent->first_child;
- snap->parent->first_child = snap->first_child;
+ virDomainSnapshotMoveChildren(snap, snap->parent);
}
if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) {
- snap->nchildren = 0;
- snap->first_child = NULL;
+ virDomainSnapshotDropChildren(snap);
} else {
virDomainSnapshotDropParent(snap);
if (snap == virDomainSnapshotGetCurrent(vm->snapshots)) {
--
2.20.1