snapshot: Add accessor for reparenting snapshot children

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.
This patch starts the task with a single new function:
virDomainSnapshotMoveChildren(). The logic might not be immediately
obvious [okay, that's an understatement - the existing code uses black
magic ;-)], so here's an overview: The old code has an implicit for
loop around each call to qemuDomainSnapshotReparentChildren() by using
virDomainSnapshotForEachChild() (you'll need a wider context than
git's default of 3 lines to see that); the new code has a more visible
for loop. Then it helps if you realize that the code is making two
separate changes to each child object: STRDUP of the new parent name
prior to writing XML files (unchanged), and touching up the pointer to
the parent object (refactored); the end result is the same whether a
single pass made both changes (both in driver code), or whether it is
split into two passes making one change each (one in driver code, the
other in the new accessor).

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>

diff --git a/src/conf/virdomainsnapshotobj.c b/src/conf/virdomainsnapshotobj.c
index 7f92ac21d949d905fc46118197aab7a6920c59d2..271cd344ffe131f6dde80a2b3df8f95816720c63 100644 (file)
--- a/src/conf/virdomainsnapshotobj.c
+++ b/src/conf/virdomainsnapshotobj.c
@@ -121,3 +121,26 @@ virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot)
     snapshot->parent = NULL;
     snapshot->sibling = NULL;
 }
+
+
+/* Take all children of @from and convert them into children of @to. */
+void
+virDomainSnapshotMoveChildren(virDomainSnapshotObjPtr from,
+                              virDomainSnapshotObjPtr to)
+{
+    virDomainSnapshotObjPtr child;
+    virDomainSnapshotObjPtr last;
+
+    if (!from->first_child)
+        return;
+    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/virdomainsnapshotobj.h b/src/conf/virdomainsnapshotobj.h
index 957f1b2ea84ea1cee6effdb31745dd58a0ac01f8..22961729e84377cc7d9ba2001f17c441d2bd714f 100644 (file)
--- a/src/conf/virdomainsnapshotobj.h
+++ b/src/conf/virdomainsnapshotobj.h
@@ -46,5 +46,7 @@ int virDomainSnapshotForEachDescendant(virDomainSnapshotObjPtr snapshot,
                                        virHashIterator iter,
                                        void *data);
 void virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot);
+void virDomainSnapshotMoveChildren(virDomainSnapshotObjPtr from,
+                                   virDomainSnapshotObjPtr to);
 
 #endif /* LIBVIRT_VIRDOMAINSNAPSHOTOBJ_H */

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 41d17226f95895e7e4269df49b78162f5ea37aae..22f13ce64ddfd9baff79c7b5ff8e1cb84b1050c1 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -983,6 +983,7 @@ virDomainObjListRename;
 virDomainSnapshotDropParent;
 virDomainSnapshotForEachChild;
 virDomainSnapshotForEachDescendant;
+virDomainSnapshotMoveChildren;
 
 
 # conf/virdomainsnapshotobjlist.h

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 3479098e9cb6cac8e5593b69445f3892cffab0c7..bc9b6d2b80712d2cbede66f956c1b5d5e6d790f1 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16763,7 +16763,6 @@ struct _virQEMUSnapReparent {
     virCapsPtr caps;
     virDomainXMLOptionPtr xmlopt;
     int err;
-    virDomainSnapshotObjPtr last;
 };
 
 
@@ -16779,7 +16778,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 +16785,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 +16872,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,10 +16879,7 @@ 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) {

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index f73648fbe1ba0389981eca7ff77d03b9750c28b9..d68a5ce1b11689a15bf80386b26a1aa2ce93e9ba 100644 (file)
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -6454,7 +6454,6 @@ struct _testSnapReparentData {
     virDomainSnapshotObjPtr parent;
     virDomainObjPtr vm;
     int err;
-    virDomainSnapshotObjPtr last;
 };
 
 static int
@@ -6469,7 +6468,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 +6475,6 @@ testDomainSnapshotReparentChildren(void *payload,
         return 0;
     }
 
-    if (!snap->sibling)
-        rep->last = snap;
     return 0;
 }
 
@@ -6515,17 +6511,13 @@ 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) {