snapshot: avoid crash when deleting qemu snapshots

This one's nasty.  Ever since we fixed virHashForEach to prevent
nested hash iterations for safety reasons (commit fba550f6),
virDomainSnapshotDelete with VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN
has been broken for qemu: it deletes children, while leaving
grandchildren intact but pointing to a no-longer-present parent.
But even before then, the code would often appear to succeed to
clean up grandchildren, but risked memory corruption if you have
a large and deep hierarchy of snapshots.

For acting on just children, a single virHashForEach is sufficient.
But for acting on an entire subtree, it requires iteration; and
since we declared recursion as invalid, we have to switch to a
while loop.  Doing this correctly requires quite a bit of overhaul,
so I added a new helper function to isolate the algorithm from the
actions, so that callers do not have to reinvent the iteration.

Note that this _still_ does not handle CHILDREN correctly if one
of the children is the current snapshot; that will be next.

* src/conf/domain_conf.h (_virDomainSnapshotDef): Add mark.
(virDomainSnapshotForEachDescendant): New prototype.
* src/libvirt_private.syms (domain_conf.h): Export it.
* src/conf/domain_conf.c (virDomainSnapshotMarkDescendant)
(virDomainSnapshotActOnDescendant)
(virDomainSnapshotForEachDescendant): New functions.
* src/qemu/qemu_driver.c (qemuDomainSnapshotDiscardChildren):
Replace...
(qemuDomainSnapshotDiscardDescenent): ...with callback that
doesn't nest hash traversal.
(qemuDomainSnapshotDelete): Use new function.

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e031cb4cdc7133bf1e64e5155f2f6a3a328918b8..0e21f28dab23236db23b58573ec46145da56252b 100644 (file)
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11678,6 +11678,109 @@ int virDomainSnapshotHasChildren(virDomainSnapshotObjPtr snap,
     return children.number;
 }
 
+typedef enum {
+    MARK_NONE,       /* No relation determined yet */
+    MARK_DESCENDANT, /* Descendant of target */
+    MARK_OTHER,      /* Not a descendant of target */
+} snapshot_mark;
+
+struct snapshot_mark_descendant {
+    const char *name; /* Parent's name on round 1, NULL on other rounds.  */
+    virDomainSnapshotObjListPtr snapshots;
+    bool marked; /* True if descendants were found in this round */
+};
+
+/* To be called in a loop until no more descendants are found.
+ * Additionally marking known unrelated snapshots reduces the number
+ * of total hash searches needed.  */
+static void
+virDomainSnapshotMarkDescendant(void *payload,
+                                const void *name ATTRIBUTE_UNUSED,
+                                void *data)
+{
+    virDomainSnapshotObjPtr obj = payload;
+    struct snapshot_mark_descendant *curr = data;
+    virDomainSnapshotObjPtr parent = NULL;
+
+    /* Learned on a previous pass.  */
+    if (obj->mark)
+        return;
+
+    if (curr->name) {
+        /* First round can only find root nodes and direct children.  */
+        if (!obj->def->parent) {
+            obj->mark = MARK_OTHER;
+        } else if (STREQ(obj->def->parent, curr->name)) {
+            obj->mark = MARK_DESCENDANT;
+            curr->marked = true;
+        }
+    } else {
+        /* All remaining rounds propagate marks from parents to children.  */
+        parent = virDomainSnapshotFindByName(curr->snapshots, obj->def->parent);
+        if (!parent) {
+            VIR_WARN("snapshot hash table is inconsistent!");
+            obj->mark = MARK_OTHER;
+            return;
+        }
+        if (parent->mark) {
+            obj->mark = parent->mark;
+            if (obj->mark == MARK_DESCENDANT)
+                curr->marked = true;
+        }
+    }
+}
+
+struct snapshot_act_on_descendant {
+    int number;
+    virHashIterator iter;
+    void *data;
+};
+
+static void
+virDomainSnapshotActOnDescendant(void *payload,
+                                 const void *name,
+                                 void *data)
+{
+    virDomainSnapshotObjPtr obj = payload;
+    struct snapshot_act_on_descendant *curr = data;
+
+    if (obj->mark == MARK_DESCENDANT) {
+        curr->number++;
+        (curr->iter)(payload, name, curr->data);
+    }
+    obj->mark = MARK_NONE;
+}
+
+/* Run iter(data) on all descendants of snapshot, while ignoring all
+ * other entries in snapshots.  Return the number of descendants
+ * visited.  No particular ordering is guaranteed.  */
+int
+virDomainSnapshotForEachDescendant(virDomainSnapshotObjListPtr snapshots,
+                                   virDomainSnapshotObjPtr snapshot,
+                                   virHashIterator iter,
+                                   void *data)
+{
+    struct snapshot_mark_descendant mark;
+    struct snapshot_act_on_descendant act;
+
+    /* virHashForEach does not support nested traversal, so we must
+     * instead iterate until no more snapshots get marked.  We
+     * guarantee that on exit, all marks have been cleared again.  */
+    mark.name = snapshot->def->name;
+    mark.snapshots = snapshots;
+    mark.marked = true;
+    while (mark.marked) {
+        mark.marked = false;
+        virHashForEach(snapshots->objs, virDomainSnapshotMarkDescendant, &mark);
+        mark.name = NULL;
+    }
+    act.number = 0;
+    act.iter = iter;
+    act.data = data;
+    virHashForEach(snapshots->objs, virDomainSnapshotActOnDescendant, &act);
+
+    return act.number;
+}
 
 int virDomainChrDefForeach(virDomainDefPtr def,
                            bool abortOnError,

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index ccd95479ef8d7da71939696a1ae401dcc3e7e496..46863f972e70d338851278c189d3d3d8861bb50b 100644 (file)
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1216,7 +1216,6 @@ struct _virDomainClockDef {
 
 # define VIR_DOMAIN_CPUMASK_LEN 1024
 
-
 typedef struct _virDomainVcpuPinDef virDomainVcpuPinDef;
 typedef virDomainVcpuPinDef *virDomainVcpuPinDefPtr;
 struct _virDomainVcpuPinDef {
@@ -1395,6 +1394,9 @@ typedef struct _virDomainSnapshotObj virDomainSnapshotObj;
 typedef virDomainSnapshotObj *virDomainSnapshotObjPtr;
 struct _virDomainSnapshotObj {
     virDomainSnapshotDefPtr def;
+
+    /* Internal use only */
+    int mark; /* Used in identifying descendents. */
 };
 
 typedef struct _virDomainSnapshotObjList virDomainSnapshotObjList;
@@ -1424,6 +1426,10 @@ void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots,
                                     virDomainSnapshotObjPtr snapshot);
 int virDomainSnapshotHasChildren(virDomainSnapshotObjPtr snap,
                                 virDomainSnapshotObjListPtr snapshots);
+int virDomainSnapshotForEachDescendant(virDomainSnapshotObjListPtr snapshots,
+                                       virDomainSnapshotObjPtr snapshot,
+                                       virHashIterator iter,
+                                       void *data);
 
 /* Guest VM runtime state */
 typedef struct _virDomainStateReason virDomainStateReason;

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 65d280e3eb4148c5b8b391e2d4564d912cc1a9e9..5b4664e1d9c5834e2ab345fe965560a0227897b3 100644 (file)
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -391,6 +391,7 @@ virDomainSnapshotDefFormat;
 virDomainSnapshotDefFree;
 virDomainSnapshotDefParseString;
 virDomainSnapshotFindByName;
+virDomainSnapshotForEachDescendant;
 virDomainSnapshotHasChildren;
 virDomainSnapshotObjListGetNames;
 virDomainSnapshotObjListNum;

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 43cb4c04eb14d66ddb2e0577d46761fae175c037..55cdc3f81ba163d4d1a3214132458063a6a7ed5f 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9230,31 +9230,21 @@ cleanup:
 struct snap_remove {
     struct qemud_driver *driver;
     virDomainObjPtr vm;
-    char *parent;
     int err;
 };
 
-static void qemuDomainSnapshotDiscardChildren(void *payload,
-                                              const void *name ATTRIBUTE_UNUSED,
-                                              void *data)
+static void
+qemuDomainSnapshotDiscardDescendant(void *payload,
+                                    const void *name ATTRIBUTE_UNUSED,
+                                    void *data)
 {
     virDomainSnapshotObjPtr snap = payload;
     struct snap_remove *curr = data;
-    struct snap_remove this;
-
-    if (snap->def->parent && STREQ(snap->def->parent, curr->parent)) {
-        this.driver = curr->driver;
-        this.vm = curr->vm;
-        this.parent = snap->def->name;
-        this.err = 0;
-        virHashForEach(curr->vm->snapshots.objs,
-                       qemuDomainSnapshotDiscardChildren, &this);
-
-        if (this.err)
-            curr->err = this.err;
-        else
-            this.err = qemuDomainSnapshotDiscard(curr->driver, curr->vm, snap);
-    }
+    int err;
+
+    err = qemuDomainSnapshotDiscard(curr->driver, curr->vm, snap);
+    if (err && !curr->err)
+        curr->err = err;
 }
 
 struct snap_reparent {
@@ -9330,10 +9320,11 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
     if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) {
         rem.driver = driver;
         rem.vm = vm;
-        rem.parent = snap->def->name;
         rem.err = 0;
-        virHashForEach(vm->snapshots.objs, qemuDomainSnapshotDiscardChildren,
-                       &rem);
+        virDomainSnapshotForEachDescendant(&vm->snapshots,
+                                           snap,
+                                           qemuDomainSnapshotDiscardDescendant,
+                                           &rem);
         if (rem.err < 0)
             goto endjob;
     } else {