10:22 a.m.
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 | 8 +++-
src/libvirt_private.syms | 1 +
src/qemu/qemu_driver.c | 35 ++++++----------
4 files changed, 124 insertions(+), 23 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 44212cf..75fa7f7 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -11266,6 +11266,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 8382d28..d266605 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1139,7 +1139,6 @@ struct _virDomainClockDef {
# define VIR_DOMAIN_CPUMASK_LEN 1024
-
typedef struct _virDomainVcpuPinDef virDomainVcpuPinDef;
typedef virDomainVcpuPinDef *virDomainVcpuPinDefPtr;
struct _virDomainVcpuPinDef {
@@ -1312,6 +1311,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;
@@ -1341,6 +1343,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 0618b49..fcce810 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 b492c97..1405b71 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -9031,31 +9031,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 {
@@ -9131,10 +9121,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