[libvirt] [PATCH 7/8] snapshot: use metaroot node to simplify management

This idea was first suggested by Daniel Veillard here: https://www.redhat.com/archives/libvir-list/2011-October/msg00353.html Now that I am about to add more complexity to snapshot listing, it makes sense to avoid code duplication and special casing for domain listing (all snapshots) vs. snapshot listing (descendants); adding a metaroot reduces the number of code lines by having the domain listing turn into a descendant listing of the metaroot. Note that this has one minor pessimization - if we are going to list ALL snapshots without filtering, then virHashForeach is more efficient than recursing through the child relationships; restoring that minor optimization will occur in the next patch. * src/conf/domain_conf.h (_virDomainSnapshotObj) (_virDomainSnapshotObjList): Repurpose some fields. (virDomainSnapshotDropParent): Drop unused parameter. * src/conf/domain_conf.c (virDomainSnapshotObjListGetNames) (virDomainSnapshotObjListCount): Simplify. (virDomainSnapshotFindByName, virDomainSnapshotSetRelations) (virDomainSnapshotDropParent): Match new field semantics. * src/qemu/qemu_driver.c (qemuDomainSnapshotCreateXML) (qemuDomainSnapshotReparentChildren, qemuDomainSnapshotDelete): Adjust clients. --- Turns out I had 4 instead of 2 more patches to go. Oops, just noticed an #if 0 leftover in this patch. I'll be respinning a v2 anyways, as I hope to rebase this patch to occur earlier in the series. src/conf/domain_conf.c | 119 +++++++++++++----------------------------------- src/conf/domain_conf.h | 12 ++--- src/qemu/qemu_driver.c | 36 +++++---------- 3 files changed, 49 insertions(+), 118 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 44343ee..a653af5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14321,33 +14321,14 @@ int virDomainSnapshotObjListGetNames(virDomainSnapshotObjListPtr snapshots, char **const names, int maxnames, unsigned int flags) { - struct virDomainSnapshotNameData data = { 0, 0, maxnames, names, 0 }; - int i; - - if (virDomainSnapshotObjListPrepFlags(&data.flags, flags) < 0) - return 0; - - if (!(flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) { + flags ^= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; + return virDomainSnapshotObjListGetNamesFrom(&snapshots->metaroot, names, + maxnames, flags); +#if 0 + if (!(flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) virHashForEach(snapshots->objs, virDomainSnapshotObjListCopyNames, &data); - } else { - virDomainSnapshotObjPtr root = snapshots->first_root; - while (root) { - virDomainSnapshotObjListCopyNames(root, root->def->name, &data); - root = root->sibling; - } - } - if (data.oom) { - virReportOOMError(); - goto cleanup; - } - - return data.numnames; - -cleanup: - for (i = 0; i < data.numnames; i++) - VIR_FREE(data.names[i]); - return -1; +#endif } int virDomainSnapshotObjListGetNamesFrom(virDomainSnapshotObjPtr snapshot, @@ -14404,24 +14385,8 @@ static void virDomainSnapshotObjListCount(void *payload, int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots, unsigned int flags) { - struct virDomainSnapshotNumData data = { 0, 0 }; - - if (virDomainSnapshotObjListPrepFlags(&data.flags, flags) < 0) - return 0; - - if (!(flags & VIR_DOMAIN_SNAPSHOT_LIST_ROOTS)) { - virHashForEach(snapshots->objs, virDomainSnapshotObjListCount, &data); - } else if (data.flags) { - virDomainSnapshotObjPtr root = snapshots->first_root; - while (root) { - virDomainSnapshotObjListCount(root, root->def->name, &data); - root = root->sibling; - } - } else { - data.count = snapshots->nroots; - } - - return data.count; + flags ^= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; + return virDomainSnapshotObjListNumFrom(&snapshots->metaroot, flags); } int @@ -14450,7 +14415,7 @@ virDomainSnapshotObjPtr virDomainSnapshotFindByName(const virDomainSnapshotObjListPtr snapshots, const char *name) { - return virHashLookup(snapshots->objs, name); + return name ? virHashLookup(snapshots->objs, name) : &snapshots->metaroot; } void virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, @@ -14536,34 +14501,27 @@ virDomainSnapshotSetRelations(void *payload, struct snapshot_set_relation *curr = data; virDomainSnapshotObjPtr tmp; - if (obj->def->parent) { - obj->parent = virDomainSnapshotFindByName(curr->snapshots, - obj->def->parent); - if (!obj->parent) { - curr->err = -1; - VIR_WARN("snapshot %s lacks parent", obj->def->name); - } else { - tmp = obj->parent; - while (tmp) { - if (tmp == obj) { - curr->err = -1; - obj->parent = NULL; - VIR_WARN("snapshot %s in circular chain", obj->def->name); - break; - } - tmp = tmp->parent; - } - if (!tmp) { - obj->parent->nchildren++; - obj->sibling = obj->parent->first_child; - obj->parent->first_child = obj; + obj->parent = virDomainSnapshotFindByName(curr->snapshots, + obj->def->parent); + if (!obj->parent) { + curr->err = -1; + obj->parent = &curr->snapshots->metaroot; + VIR_WARN("snapshot %s lacks parent", obj->def->name); + } else { + tmp = obj->parent; + while (tmp && tmp->def) { + if (tmp == obj) { + curr->err = -1; + obj->parent = &curr->snapshots->metaroot; + VIR_WARN("snapshot %s in circular chain", obj->def->name); + break; } + tmp = tmp->parent; } - } else { - curr->snapshots->nroots++; - obj->sibling = curr->snapshots->first_root; - curr->snapshots->first_root = obj; } + obj->parent->nchildren++; + obj->sibling = obj->parent->first_child; + obj->parent->first_child = obj; } /* Populate parent link and child count of all snapshots, with all @@ -14583,28 +14541,13 @@ virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots) * of a parent, it is faster to just 0 the count rather than calling * this function on each child. */ void -virDomainSnapshotDropParent(virDomainSnapshotObjListPtr snapshots, - virDomainSnapshotObjPtr snapshot) +virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot) { virDomainSnapshotObjPtr prev = NULL; virDomainSnapshotObjPtr curr = NULL; - size_t *count; - virDomainSnapshotObjPtr *first; - if (snapshot->parent) { - count = &snapshot->parent->nchildren; - first = &snapshot->parent->first_child; - } else { - count = &snapshots->nroots; - first = &snapshots->first_root; - } - - if (!*count || !*first) { - VIR_WARN("inconsistent snapshot relations"); - return; - } - (*count)--; - curr = *first; + snapshot->parent->nchildren--; + curr = snapshot->parent->first_child; while (curr != snapshot) { if (!curr) { VIR_WARN("inconsistent snapshot relations"); @@ -14616,7 +14559,7 @@ virDomainSnapshotDropParent(virDomainSnapshotObjListPtr snapshots, if (prev) prev->sibling = snapshot->sibling; else - *first = snapshot->sibling; + snapshot->parent->first_child = snapshot->sibling; snapshot->parent = NULL; snapshot->sibling = NULL; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 88674eb..e3a3679 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1726,9 +1726,11 @@ struct _virDomainSnapshotDef { typedef struct _virDomainSnapshotObj virDomainSnapshotObj; typedef virDomainSnapshotObj *virDomainSnapshotObjPtr; struct _virDomainSnapshotObj { - virDomainSnapshotDefPtr def; + virDomainSnapshotDefPtr def; /* non-NULL except for metaroot */ - virDomainSnapshotObjPtr parent; /* NULL if root */ + virDomainSnapshotObjPtr parent; /* non-NULL except for metaroot, before + virDomainSnapshotUpdateRelations, or + after virDomainSnapshotDropParent */ virDomainSnapshotObjPtr sibling; /* NULL if last child of parent */ size_t nchildren; virDomainSnapshotObjPtr first_child; /* NULL if no children */ @@ -1741,8 +1743,7 @@ struct _virDomainSnapshotObjList { * for O(1), lockless lookup-by-name */ virHashTable *objs; - size_t nroots; - virDomainSnapshotObjPtr first_root; + virDomainSnapshotObj metaroot; /* Special parent of all root snapshots */ }; typedef enum { @@ -1788,8 +1789,7 @@ int virDomainSnapshotForEachDescendant(virDomainSnapshotObjPtr snapshot, virHashIterator iter, void *data); int virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots); -void virDomainSnapshotDropParent(virDomainSnapshotObjListPtr snapshots, - virDomainSnapshotObjPtr snapshot); +void virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot); /* Guest VM runtime state */ typedef struct _virDomainStateReason virDomainStateReason; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a1b76c2..7038a4c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -10480,7 +10480,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } /* Drop and rebuild the parent relationship, but keep all * child relations by reusing snap. */ - virDomainSnapshotDropParent(&vm->snapshots, other); + virDomainSnapshotDropParent(other); virDomainSnapshotDefFree(other->def); other->def = NULL; snap = other; @@ -10589,18 +10589,12 @@ cleanup: } else { if (update_current) vm->current_snapshot = snap; - if (snap->def->parent) { - other = virDomainSnapshotFindByName(&vm->snapshots, - snap->def->parent); - snap->parent = other; - other->nchildren++; - snap->sibling = other->first_child; - other->first_child = snap; - } else { - vm->snapshots.nroots++; - snap->sibling = vm->snapshots.first_root; - vm->snapshots.first_root = snap; - } + other = virDomainSnapshotFindByName(&vm->snapshots, + snap->def->parent); + snap->parent = other; + other->nchildren++; + snap->sibling = other->first_child; + other->first_child = snap; } } else if (snap) { virDomainSnapshotObjListRemove(&vm->snapshots, snap); @@ -11405,7 +11399,7 @@ qemuDomainSnapshotReparentChildren(void *payload, VIR_FREE(snap->def->parent); snap->parent = rep->parent; - if (rep->parent) { + if (rep->parent->def) { snap->def->parent = strdup(rep->parent->def->name); if (snap->def->parent == NULL) { @@ -11513,15 +11507,9 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, if (rep.err < 0) goto endjob; /* Can't modify siblings during ForEachChild, so do it now. */ - if (snap->parent) { - snap->parent->nchildren += snap->nchildren; - rep.last->sibling = snap->parent->first_child; - snap->parent->first_child = snap->first_child; - } else { - vm->snapshots.nroots += snap->nchildren; - rep.last->sibling = vm->snapshots.first_root; - vm->snapshots.first_root = snap->first_child; - } + snap->parent->nchildren += snap->nchildren; + rep.last->sibling = snap->parent->first_child; + snap->parent->first_child = snap->first_child; } if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) { @@ -11529,7 +11517,7 @@ static int qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, snap->first_child = NULL; ret = 0; } else { - virDomainSnapshotDropParent(&vm->snapshots, snap); + virDomainSnapshotDropParent(snap); ret = qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only); } -- 1.7.10.2