External/libvirt
1
0
Fork 0
mirror of https://gitlab.com/libvirt/libvirt.git synced 2025-04-01 20:05:19 +00:00
Code

snapshot: avoid crash when deleting qemu snapshots

Browse Source 
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.
This commit is contained in:
Eric Blake 2011-08-12 07:05:50 -06:00
parent a31d65695d
commit cb231b4bee
4 changed files with 123 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

103
src/conf/domain_conf.c
View File

@ -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,

8
src/conf/domain_conf.h
View File

@ -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;

1
src/libvirt_private.syms
View File

@ -391,6 +391,7 @@ virDomainSnapshotDefFormat;
virDomainSnapshotDefFree;
virDomainSnapshotDefParseString;
virDomainSnapshotFindByName;
virDomainSnapshotForEachDescendant;
virDomainSnapshotHasChildren;
virDomainSnapshotObjListGetNames;
virDomainSnapshotObjListNum;

33
src/qemu/qemu_driver.c
View File

@ -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;
int err;
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);
}
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 {