[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCHv2 03/13] snapshot: use metaroot node to simplify management



On 06/15/12 06:18, Eric Blake wrote:
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;

I'm not quite sure what's the meaning of this statement. It efectively negates the VIR_DOMAIN_SNAPSHOT_LIST_ROOTS flag, but the original code filtered it out.

+    return virDomainSnapshotObjListGetNamesFrom(&snapshots->metaroot, names,
+                                                maxnames, flags);

This function calls virDomainSnapshotObjListCopyNames on each of children of the object but the VIR_DOMAIN_SNAPSHOT_LIST_ROOTS is never checked. In fact virDomainSnapshotObjListCopyNames states that:

 "/* LIST_ROOTS/LIST_DESCENDANTS was handled by caller, ..."

I assume that it has to be filtered out:
flags &= ~VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;


  }

  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);

Same comment as above.

  }

  int


ACK if the flag masking issue gets cleared. The metaroot approach is nice and consistent.

Peter


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]