snapshot: use metaroot node to simplify management

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.

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 8d80f3b01710d64087c94ed53e4a5628a7408ac7..43872d1c7370ebac1a1ab858a87f4d9c7406c2a8 100644 (file)
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -14272,8 +14272,9 @@ static void virDomainSnapshotObjListCopyNames(void *payload,
 
     if (data->oom)
         return;
-    /* LIST_ROOTS/LIST_DESCENDANTS was handled by caller,
-     * LIST_METADATA is a no-op if we get this far.  */
+    /* LIST_ROOTS/LIST_DESCENDANTS was handled by the choice of
+     * iteration made in the caller, and LIST_METADATA is a no-op if
+     * we get this far.  */
     if ((data->flags & VIR_DOMAIN_SNAPSHOT_LIST_LEAVES) && obj->nchildren)
         return;
 
@@ -14289,32 +14290,12 @@ 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;
+    /* LIST_ROOTS and LIST_DESCENDANTS have the same bit value, but
+     * opposite semantics.  Toggle here to get the correct traversal
+     * on the metaroot.  */
+    flags ^= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;
+    return virDomainSnapshotObjListGetNamesFrom(&snapshots->metaroot, names,
+                                                maxnames, flags);
 }
 
 int virDomainSnapshotObjListGetNamesFrom(virDomainSnapshotObjPtr snapshot,
@@ -14359,8 +14340,9 @@ static void virDomainSnapshotObjListCount(void *payload,
     virDomainSnapshotObjPtr obj = payload;
     struct virDomainSnapshotNumData *data = opaque;
 
-    /* LIST_ROOTS/LIST_DESCENDANTS was handled by caller,
-     * LIST_METADATA is a no-op if we get this far.  */
+    /* LIST_ROOTS/LIST_DESCENDANTS was handled by the choice of
+     * iteration made in the caller, and LIST_METADATA is a no-op if
+     * we get this far.  */
     if ((data->flags & VIR_DOMAIN_SNAPSHOT_LIST_LEAVES) && obj->nchildren)
         return;
     data->count++;
@@ -14369,23 +14351,11 @@ 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;
+    /* LIST_ROOTS and LIST_DESCENDANTS have the same bit value, but
+     * opposite semantics.  Toggle here to get the correct traversal
+     * on the metaroot.  */
+    flags ^= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;
+    return virDomainSnapshotObjListNumFrom(&snapshots->metaroot, flags);
 }
 
 int
@@ -14413,7 +14383,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 +14469,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 +14509,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 +14527,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 88674eb8308de5ecd6c21b5653bbdee9dc9d7c6b..e3a36792680055a4d9eca6cfa78183531190fd50 100644 (file)
--- 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 3fd9803812aeffdc2547a41c4948dcb485e43cae..1ff2af4887082d08a4ac31013df7b5a298b86e8d 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10514,7 +10514,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;
@@ -10623,18 +10623,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);
@@ -11439,7 +11433,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) {
@@ -11547,15 +11541,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) {
@@ -11563,7 +11551,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);
     }