snapshot: Don't leak moment obj list metaroot to callers

virDomainSnapshotFindByName(list, NULL) should return NULL, rather
than the internal-use-only metaroot.  Most existing callers pass in a
non-NULL name; the few external callers that don't are immediately
calling virDomainMomentSetParent (which indeed needs the metaroot
rather than NULL if the parent name is NULL); but as the leaky
abstraction is ugly, it is worth instead making
virDomainMomentSetParent static and adding a new function for
resolving the parent link of a brand new moment within its list.  The
existing external uses of virDomainMomentSetParent always succeed
(either the new moment has parent_name of NULL to become a new root,
or has parent_name set to a strdup of the previous current moment);
hence, our new function does not need a return value (but it still has
a VIR_WARN in case future uses break our assumptions about failure
being impossible).

Missed when commit 02c4e24d refactored things to attempt to remove
direct metaroot manipulations out of the qemu and test drivers into
internal-only details, and made more obvious when commit dc8d3dc6
factored it out into a separate file.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

diff --git a/src/conf/virdomainmomentobjlist.c b/src/conf/virdomainmomentobjlist.c
index 7cb1745525fe11a447aed90abf8a1bee51a692b2..5c147bdbbf87d1bc266061e2ff2784b3a46f092d 100644 (file)
--- a/src/conf/virdomainmomentobjlist.c
+++ b/src/conf/virdomainmomentobjlist.c
@@ -151,7 +151,7 @@ virDomainMomentDropChildren(virDomainMomentObjPtr moment)
 
 
 /* Add @moment to @parent's list of children. */
-void
+static void
 virDomainMomentSetParent(virDomainMomentObjPtr moment,
                          virDomainMomentObjPtr parent)
 {
@@ -162,6 +162,27 @@ virDomainMomentSetParent(virDomainMomentObjPtr moment,
 }
 
 
+/* Add @moment to the appropriate parent's list of children. The
+ * caller must ensure that moment->def->parent_name is either NULL
+ * (for a new root) or set to an existing moment already in the
+ * list. */
+void
+virDomainMomentLinkParent(virDomainMomentObjListPtr moments,
+                          virDomainMomentObjPtr moment)
+{
+    virDomainMomentObjPtr parent;
+
+    parent = virDomainMomentFindByName(moments, moment->def->parent_name);
+    if (!parent) {
+        parent = &moments->metaroot;
+        if (moment->def->parent_name)
+            VIR_WARN("moment %s lacks parent %s", moment->def->name,
+                     moment->def->parent_name);
+    }
+    virDomainMomentSetParent(moment, parent);
+}
+
+
 /* Take all children of @from and convert them into children of @to. */
 void
 virDomainMomentMoveChildren(virDomainMomentObjPtr from,
@@ -390,7 +411,9 @@ virDomainMomentObjPtr
 virDomainMomentFindByName(virDomainMomentObjListPtr moments,
                           const char *name)
 {
-    return name ? virHashLookup(moments->objs, name) : &moments->metaroot;
+    if (name)
+        return virHashLookup(moments->objs, name);
+    return NULL;
 }
 
 
@@ -484,9 +507,12 @@ virDomainMomentSetRelations(void *payload,
 
     parent = virDomainMomentFindByName(curr->moments, obj->def->parent_name);
     if (!parent) {
-        curr->err = -1;
         parent = &curr->moments->metaroot;
-        VIR_WARN("moment %s lacks parent", obj->def->name);
+        if (obj->def->parent_name) {
+            curr->err = -1;
+            VIR_WARN("moment %s lacks parent %s", obj->def->name,
+                     obj->def->parent_name);
+        }
     } else {
         tmp = parent;
         while (tmp && tmp->def) {

diff --git a/src/conf/virdomainmomentobjlist.h b/src/conf/virdomainmomentobjlist.h
index 4067e928f4656253d0d847577bf5867e331a9a84..897d8d54d1ba8ee9ba6a41022c2fbf7b74953719 100644 (file)
--- a/src/conf/virdomainmomentobjlist.h
+++ b/src/conf/virdomainmomentobjlist.h
@@ -60,8 +60,8 @@ void virDomainMomentDropParent(virDomainMomentObjPtr moment);
 void virDomainMomentDropChildren(virDomainMomentObjPtr moment);
 void virDomainMomentMoveChildren(virDomainMomentObjPtr from,
                                  virDomainMomentObjPtr to);
-void virDomainMomentSetParent(virDomainMomentObjPtr moment,
-                              virDomainMomentObjPtr parent);
+void virDomainMomentLinkParent(virDomainMomentObjListPtr moments,
+                               virDomainMomentObjPtr moment);
 
 virDomainMomentObjListPtr virDomainMomentObjListNew(void);
 void virDomainMomentObjListFree(virDomainMomentObjListPtr moments);

diff --git a/src/conf/virdomainsnapshotobjlist.c b/src/conf/virdomainsnapshotobjlist.c
index 99bc4bb0c521b65a8a4be801ab667f57ad135588..95622f0ba7fc9fcfcca3e7b4668f0de9aae1befc 100644 (file)
--- a/src/conf/virdomainsnapshotobjlist.c
+++ b/src/conf/virdomainsnapshotobjlist.c
@@ -223,6 +223,15 @@ virDomainSnapshotForEach(virDomainSnapshotObjListPtr snapshots,
 }
 
 
+/* Populate parent link of a given snapshot. */
+void
+virDomainSnapshotLinkParent(virDomainSnapshotObjListPtr snapshots,
+                            virDomainMomentObjPtr snap)
+{
+    return virDomainMomentLinkParent(snapshots->base, snap);
+}
+
+
 /* Populate parent link and child count of all snapshots, with all
  * assigned defs having relations starting as 0/NULL. Return 0 on
  * success, -1 if a parent is missing or if a circular relationship

diff --git a/src/conf/virdomainsnapshotobjlist.h b/src/conf/virdomainsnapshotobjlist.h
index fed8d22bc869eb50fe85609502b131f2225f170e..b8c80b39eda2d83f6509aef546b2f21da7d50575 100644 (file)
--- a/src/conf/virdomainsnapshotobjlist.h
+++ b/src/conf/virdomainsnapshotobjlist.h
@@ -51,6 +51,8 @@ void virDomainSnapshotObjListRemoveAll(virDomainSnapshotObjListPtr snapshots);
 int virDomainSnapshotForEach(virDomainSnapshotObjListPtr snapshots,
                              virHashIterator iter,
                              void *data);
+void virDomainSnapshotLinkParent(virDomainSnapshotObjListPtr snapshots,
+                                 virDomainMomentObjPtr moment);
 int virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots);
 int virDomainSnapshotCheckCycles(virDomainSnapshotObjListPtr snapshots,
                                  virDomainSnapshotDefPtr def,

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index dff97bd82a7c4008182d8debf808a82b4fa5d760..ff5a77b0e27dfa936eae2ce83719365155bb5521 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -978,7 +978,6 @@ virDomainMomentDropParent;
 virDomainMomentForEachChild;
 virDomainMomentForEachDescendant;
 virDomainMomentMoveChildren;
-virDomainMomentSetParent;
 
 
 # conf/virdomainobjlist.h
@@ -1007,6 +1006,7 @@ virDomainSnapshotFindByName;
 virDomainSnapshotForEach;
 virDomainSnapshotGetCurrent;
 virDomainSnapshotGetCurrentName;
+virDomainSnapshotLinkParent;
 virDomainSnapshotObjListFree;
 virDomainSnapshotObjListGetNames;
 virDomainSnapshotObjListNew;

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 482f915b67ec46e8f8da5b2de2cb2c15e5adb0d3..065e0a1bd8836caf424ea200eeac2720879bdf13 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15540,7 +15540,6 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
     bool update_current = true;
     bool redefine = flags & VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE;
     unsigned int parse_flags = VIR_DOMAIN_SNAPSHOT_PARSE_DISKS;
-    virDomainMomentObjPtr other = NULL;
     int align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_INTERNAL;
     bool align_match = true;
     virQEMUDriverConfigPtr cfg = NULL;
@@ -15807,9 +15806,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
                            snap->def->name);
             virDomainSnapshotObjListRemove(vm->snapshots, snap);
         } else {
-            other = virDomainSnapshotFindByName(vm->snapshots,
-                                                snap->def->parent_name);
-            virDomainMomentSetParent(snap, other);
+            virDomainSnapshotLinkParent(vm->snapshots, snap);
         }
     } else if (snap) {
         virDomainSnapshotObjListRemove(vm->snapshots, snap);

diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 22b96d435baf52d025c9257886d9e2ee42349688..56f6b78ecc906c9d2855159401d4a1f80d0012ab 100755 (executable)
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -7663,12 +7663,9 @@ testDomainSnapshotCreateXML(virDomainPtr domain,
  cleanup:
     if (vm) {
         if (snapshot) {
-            virDomainMomentObjPtr other;
             if (update_current)
                 virDomainSnapshotSetCurrent(vm->snapshots, snap);
-            other = virDomainSnapshotFindByName(vm->snapshots,
-                                                snap->def->parent_name);
-            virDomainMomentSetParent(snap, other);
+            virDomainSnapshotLinkParent(vm->snapshots, snap);
         }
         virDomainObjEndAPI(&vm);
     }