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

[libvirt] [PATCH 2/3] snapshot: avoid crash when deleting qemu snapshots



This one's nasty.  Ever since we fixed virHashForEach to prevent
nested hash iterations for safety reasons, 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.

* 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.
---
 src/conf/domain_conf.c   |  103 ++++++++++++++++++++++++++++++++++++++++++++++
 src/conf/domain_conf.h   |    9 ++++-
 src/libvirt_private.syms |    1 +
 src/qemu/qemu_driver.c   |   35 ++++++----------
 4 files changed, 125 insertions(+), 23 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 010ce57..b3770c4 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11233,6 +11233,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 abf9cbd..373a88a 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1140,13 +1140,16 @@ struct _virDomainSnapshotDef {
     long long creationTime; /* in seconds */
     int state;

-    long active;
+    long active; /* XXX make this internal use only to identify current snap */
 };

 typedef struct _virDomainSnapshotObj virDomainSnapshotObj;
 typedef virDomainSnapshotObj *virDomainSnapshotObjPtr;
 struct _virDomainSnapshotObj {
     virDomainSnapshotDefPtr def;
+
+    /* Internal use only */
+    int mark; /* Used in identifying descendents. */
 };

 typedef struct _virDomainSnapshotObjList virDomainSnapshotObjList;
@@ -1176,6 +1179,10 @@ void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots,
                                     virDomainSnapshotObjPtr snapshot);
 int virDomainSnapshotHasChildren(virDomainSnapshotObjPtr snap,
                                 virDomainSnapshotObjListPtr snapshots);
+int virDomainSnapshotForEachDescendant(virDomainSnapshotObjListPtr snapshots,
+                                       virDomainSnapshotObjPtr snapshot,
+                                       virHashIterator iter,
+                                       void *data);

 typedef struct _virDomainVcpuPinDef virDomainVcpuPinDef;
 typedef virDomainVcpuPinDef *virDomainVcpuPinDefPtr;
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 830222b..b8ea369 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -384,6 +384,7 @@ virDomainSnapshotDefFormat;
 virDomainSnapshotDefFree;
 virDomainSnapshotDefParseString;
 virDomainSnapshotFindByName;
+virDomainSnapshotForEachDescendant;
 virDomainSnapshotHasChildren;
 virDomainSnapshotObjListGetNames;
 virDomainSnapshotObjListNum;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a1e89ba..eeee9dd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9008,31 +9008,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 {
@@ -9108,10 +9098,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 {
-- 
1.7.4.4


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