11:18 p.m.
This idea was first suggested by Daniel Veillard here:
https://www.redhat.com/archives/libvir-list/2011-October/msg00353.html
Now that I am about to add more complexity to snapshot listing, it
makes sense to avoid code duplication and special casing for domain
listing (all snapshots) vs. snapshot listing (descendants); adding
a metaroot reduces the number of code lines by having the domain
listing turn into a descendant listing of the metaroot.
Note that this has one minor pessimization - if we are going to list
ALL snapshots without filtering, then virHashForeach is more efficient
than recursing through the child relationships; restoring that minor
optimization will occur in the next patch.
* src/conf/domain_conf.h (_virDomainSnapshotObj)
(_virDomainSnapshotObjList): Repurpose some fields.
(virDomainSnapshotDropParent): Drop unused parameter.
* src/conf/domain_conf.c (virDomainSnapshotObjListGetNames)
(virDomainSnapshotObjListCount): Simplify.
(virDomainSnapshotFindByName, virDomainSnapshotSetRelations)
(virDomainSnapshotDropParent): Match new field semantics.
* src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML)
(qemuDomainSnapshotReparentChildren, qemuDomainSnapshotDelete):
Adjust clients.
---
v2: fix bugs in virDomainSnapshotSetRelations, rebase earlier in series
src/conf/domain_conf.c | 116 ++++++++++++------------------------------------
src/conf/domain_conf.h | 12 ++---
src/qemu/qemu_driver.c | 36 +++++----------
3 files changed, 46 insertions(+), 118 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8d80f3b..c7437af 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14289,32 +14289,9 @@ int virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr
snapshots,
char **const names, int maxnames,
unsigned int flags)
{
- struct virDomainSnapshotNameData data = { 0, 0, maxnames, names, 0 };
- int i;
-
- data.flags = flags & ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;
-
- if (!(flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) {
- virHashForEach(snapshots->objs, virDomainSnapshotObjListCopyNames,
- &data);
- } else {
- virDomainSnapshotObjPtr root = snapshots->first_root;
- while (root) {
- virDomainSnapshotObjListCopyNames(root, root->def->name, &data);
- root = root->sibling;
- }
- }
- if (data.oom) {
- virReportOOMError();
- goto cleanup;
- }
-
- return data.numnames;
-
-cleanup:
- for (i = 0; i < data.numnames; i++)
- VIR_FREE(data.names[i]);
- return -1;
+ flags ^= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;
+ return virDomainSnapshotObjListGetNamesFrom(&snapshots->metaroot, names,
+ maxnames, flags);
}
int virDomainSnapshotObjListGetNamesFrom(virDomainSnapshotObjPtr snapshot,
@@ -14369,23 +14346,8 @@ static void virDomainSnapshotObjListCount(void *payload,
int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots,
unsigned int flags)
{
- struct virDomainSnapshotNumData data = { 0, 0 };
-
- data.flags = flags & ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;
-
- if (!(flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) {
- virHashForEach(snapshots->objs, virDomainSnapshotObjListCount, &data);
- } else if (data.flags) {
- virDomainSnapshotObjPtr root = snapshots->first_root;
- while (root) {
- virDomainSnapshotObjListCount(root, root->def->name, &data);
- root = root->sibling;
- }
- } else {
- data.count = snapshots->nroots;
- }
-
- return data.count;
+ flags ^= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;
+ return virDomainSnapshotObjListNumFrom(&snapshots->metaroot, flags);
}
int
@@ -14413,7 +14375,7 @@ virDomainSnapshotObjPtr
virDomainSnapshotFindByName(const virDomainSnapshotObjListPtr snapshots,
const char *name)
{
- return virHashLookup(snapshots->objs, name);
+ return name ? virHashLookup(snapshots->objs, name) : &snapshots->metaroot;
}
void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots,
@@ -14499,34 +14461,27 @@ virDomainSnapshotSetRelations(void *payload,
struct snapshot_set_relation *curr = data;
virDomainSnapshotObjPtr tmp;
- if (obj->def->parent) {
- obj->parent = virDomainSnapshotFindByName(curr->snapshots,
- obj->def->parent);
- if (!obj->parent) {
- curr->err = -1;
- VIR_WARN("snapshot %s lacks parent", obj->def->name);
- } else {
- tmp = obj->parent;
- while (tmp) {
- if (tmp == obj) {
- curr->err = -1;
- obj->parent = NULL;
- VIR_WARN("snapshot %s in circular chain",
obj->def->name);
- break;
- }
- tmp = tmp->parent;
- }
- if (!tmp) {
- obj->parent->nchildren++;
- obj->sibling = obj->parent->first_child;
- obj->parent->first_child = obj;
+ obj->parent = virDomainSnapshotFindByName(curr->snapshots,
+ obj->def->parent);
+ if (!obj->parent) {
+ curr->err = -1;
+ obj->parent = &curr->snapshots->metaroot;
+ VIR_WARN("snapshot %s lacks parent", obj->def->name);
+ } else {
+ tmp = obj->parent;
+ while (tmp && tmp->def) {
+ if (tmp == obj) {
+ curr->err = -1;
+ obj->parent = &curr->snapshots->metaroot;
+ VIR_WARN("snapshot %s in circular chain",
obj->def->name);
+ break;
}
+ tmp = tmp->parent;
}
- } else {
- curr->snapshots->nroots++;
- obj->sibling = curr->snapshots->first_root;
- curr->snapshots->first_root = obj;
}
+ obj->parent->nchildren++;
+ obj->sibling = obj->parent->first_child;
+ obj->parent->first_child = obj;
}
/* Populate parent link and child count of all snapshots, with all
@@ -14546,28 +14501,13 @@ virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr
snapshots)
* of a parent, it is faster to just 0 the count rather than calling
* this function on each child. */
void
-virDomainSnapshotDropParent(virDomainSnapshotObjListPtr snapshots,
- virDomainSnapshotObjPtr snapshot)
+virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot)
{
virDomainSnapshotObjPtr prev = NULL;
virDomainSnapshotObjPtr curr = NULL;
- size_t *count;
- virDomainSnapshotObjPtr *first;
- if (snapshot->parent) {
- count = &snapshot->parent->nchildren;
- first = &snapshot->parent->first_child;
- } else {
- count = &snapshots->nroots;
- first = &snapshots->first_root;
- }
-
- if (!*count || !*first) {
- VIR_WARN("inconsistent snapshot relations");
- return;
- }
- (*count)--;
- curr = *first;
+ snapshot->parent->nchildren--;
+ curr = snapshot->parent->first_child;
while (curr != snapshot) {
if (!curr) {
VIR_WARN("inconsistent snapshot relations");
@@ -14579,7 +14519,7 @@ virDomainSnapshotDropParent(virDomainSnapshotObjListPtr
snapshots,
if (prev)
prev->sibling = snapshot->sibling;
else
- *first = snapshot->sibling;
+ snapshot->parent->first_child = snapshot->sibling;
snapshot->parent = NULL;
snapshot->sibling = NULL;
}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 88674eb..e3a3679 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1726,9 +1726,11 @@ struct _virDomainSnapshotDef {
typedef struct _virDomainSnapshotObj virDomainSnapshotObj;
typedef virDomainSnapshotObj *virDomainSnapshotObjPtr;
struct _virDomainSnapshotObj {
- virDomainSnapshotDefPtr def;
+ virDomainSnapshotDefPtr def; /* non-NULL except for metaroot */
- virDomainSnapshotObjPtr parent; /* NULL if root */
+ virDomainSnapshotObjPtr parent; /* non-NULL except for metaroot, before
+ virDomainSnapshotUpdateRelations, or
+ after virDomainSnapshotDropParent */
virDomainSnapshotObjPtr sibling; /* NULL if last child of parent */
size_t nchildren;
virDomainSnapshotObjPtr first_child; /* NULL if no children */
@@ -1741,8 +1743,7 @@ struct _virDomainSnapshotObjList {
* for O(1), lockless lookup-by-name */
virHashTable *objs;
- size_t nroots;
- virDomainSnapshotObjPtr first_root;
+ virDomainSnapshotObj metaroot; /* Special parent of all root snapshots */
};
typedef enum {
@@ -1788,8 +1789,7 @@ int virDomainSnapshotForEachDescendant(virDomainSnapshotObjPtr
snapshot,
virHashIterator iter,
void *data);
int virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots);
-void virDomainSnapshotDropParent(virDomainSnapshotObjListPtr snapshots,
- virDomainSnapshotObjPtr snapshot);
+void virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot);
/* Guest VM runtime state */
typedef struct _virDomainStateReason virDomainStateReason;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a1b76c2..7038a4c 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10480,7 +10480,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
}
/* Drop and rebuild the parent relationship, but keep all
* child relations by reusing snap. */
- virDomainSnapshotDropParent(&vm->snapshots, other);
+ virDomainSnapshotDropParent(other);
virDomainSnapshotDefFree(other->def);
other->def = NULL;
snap = other;
@@ -10589,18 +10589,12 @@ cleanup:
} else {
if (update_current)
vm->current_snapshot = snap;
- if (snap->def->parent) {
- other = virDomainSnapshotFindByName(&vm->snapshots,
- snap->def->parent);
- snap->parent = other;
- other->nchildren++;
- snap->sibling = other->first_child;
- other->first_child = snap;
- } else {
- vm->snapshots.nroots++;
- snap->sibling = vm->snapshots.first_root;
- vm->snapshots.first_root = snap;
- }
+ other = virDomainSnapshotFindByName(&vm->snapshots,
+ snap->def->parent);
+ snap->parent = other;
+ other->nchildren++;
+ snap->sibling = other->first_child;
+ other->first_child = snap;
}
} else if (snap) {
virDomainSnapshotObjListRemove(&vm->snapshots, snap);
@@ -11405,7 +11399,7 @@ qemuDomainSnapshotReparentChildren(void *payload,
VIR_FREE(snap->def->parent);
snap->parent = rep->parent;
- if (rep->parent) {
+ if (rep->parent->def) {
snap->def->parent = strdup(rep->parent->def->name);
if (snap->def->parent == NULL) {
@@ -11513,15 +11507,9 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr
snapshot,
if (rep.err < 0)
goto endjob;
/* Can't modify siblings during ForEachChild, so do it now. */
- if (snap->parent) {
- snap->parent->nchildren += snap->nchildren;
- rep.last->sibling = snap->parent->first_child;
- snap->parent->first_child = snap->first_child;
- } else {
- vm->snapshots.nroots += snap->nchildren;
- rep.last->sibling = vm->snapshots.first_root;
- vm->snapshots.first_root = snap->first_child;
- }
+ snap->parent->nchildren += snap->nchildren;
+ rep.last->sibling = snap->parent->first_child;
+ snap->parent->first_child = snap->first_child;
}
if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) {
@@ -11529,7 +11517,7 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr
snapshot,
snap->first_child = NULL;
ret = 0;
} else {
- virDomainSnapshotDropParent(&vm->snapshots, snap);
+ virDomainSnapshotDropParent(snap);
ret = qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only);
}
--
1.7.10.2