Rather than allowing a leaky abstraction where multiple drivers have to open-code operations that update the relations in a virDomainSnapshotObjList, it is better to add accessor functions so that updates to relations are maintained closer to the internals. The goal here is to avoid access to, nchildren, first_child, or sibling outside of the lowest level functions, making it easier to refactor later on. While many of the conversions are fairly straightforward, the MoveChildren refactoring can be a bit interesting to follow. Signed-off-by: Eric Blake <eblake@xxxxxxxxxx> --- src/conf/virdomainsnapshotobj.h | 5 ++++ src/conf/virdomainsnapshotobjlist.h | 2 ++ src/conf/virdomainsnapshotobj.c | 42 +++++++++++++++++++++++++++++ src/conf/virdomainsnapshotobjlist.c | 42 ++++++++++++++++++----------- src/libvirt_private.syms | 3 +++ src/qemu/qemu_driver.c | 19 +++---------- src/test/test_driver.c | 18 +++---------- 7 files changed, 85 insertions(+), 46 deletions(-) diff --git a/src/conf/virdomainsnapshotobj.h b/src/conf/virdomainsnapshotobj.h index 957f1b2ea8..0981ea4c25 100644 --- a/src/conf/virdomainsnapshotobj.h +++ b/src/conf/virdomainsnapshotobj.h @@ -46,5 +46,10 @@ int virDomainSnapshotForEachDescendant(virDomainSnapshotObjPtr snapshot, virHashIterator iter, void *data); void virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot); +void virDomainSnapshotDropChildren(virDomainSnapshotObjPtr snapshot); +void virDomainSnapshotMoveChildren(virDomainSnapshotObjPtr from, + virDomainSnapshotObjPtr to); +void virDomainSnapshotSetParent(virDomainSnapshotObjPtr snapshot, + virDomainSnapshotObjPtr parent); #endif /* LIBVIRT_VIRDOMAINSNAPSHOTOBJ_H */ diff --git a/src/conf/virdomainsnapshotobjlist.h b/src/conf/virdomainsnapshotobjlist.h index e210849441..c13a0b4026 100644 --- a/src/conf/virdomainsnapshotobjlist.h +++ b/src/conf/virdomainsnapshotobjlist.h @@ -55,6 +55,7 @@ int virDomainSnapshotObjListNum(virDomainSnapshotObjListPtr snapshots, unsigned int flags); virDomainSnapshotObjPtr virDomainSnapshotFindByName(virDomainSnapshotObjListPtr snapshots, const char *name); +int virDomainSnapshotObjListSize(virDomainSnapshotObjListPtr snapshots); virDomainSnapshotObjPtr virDomainSnapshotGetCurrent(virDomainSnapshotObjListPtr snapshots); const char *virDomainSnapshotGetCurrentName(virDomainSnapshotObjListPtr snapshots); bool virDomainSnapshotIsCurrentName(virDomainSnapshotObjListPtr snapshots, @@ -63,6 +64,7 @@ void virDomainSnapshotSetCurrent(virDomainSnapshotObjListPtr snapshots, virDomainSnapshotObjPtr snapshot); bool virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, virDomainSnapshotObjPtr snapshot); +void virDomainSnapshotObjListRemoveAll(virDomainSnapshotObjListPtr snapshots); int virDomainSnapshotForEach(virDomainSnapshotObjListPtr snapshots, virHashIterator iter, void *data); diff --git a/src/conf/virdomainsnapshotobj.c b/src/conf/virdomainsnapshotobj.c index 7f92ac21d9..d6b216c7b2 100644 --- a/src/conf/virdomainsnapshotobj.c +++ b/src/conf/virdomainsnapshotobj.c @@ -121,3 +121,45 @@ virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot) snapshot->parent = NULL; snapshot->sibling = NULL; } + + +/* Update @snapshot to no longer have children. */ +void +virDomainSnapshotDropChildren(virDomainSnapshotObjPtr snapshot) +{ + snapshot->nchildren = 0; + snapshot->first_child = NULL; +} + + +/* Add @snapshot to @parent's list of children. */ +void +virDomainSnapshotSetParent(virDomainSnapshotObjPtr snapshot, + virDomainSnapshotObjPtr parent) +{ + snapshot->parent = parent; + parent->nchildren++; + snapshot->sibling = parent->first_child; + parent->first_child = snapshot; +} + + +/* Take all children of @from and convert them into children of @to. */ +void +virDomainSnapshotMoveChildren(virDomainSnapshotObjPtr from, + virDomainSnapshotObjPtr to) +{ + virDomainSnapshotObjPtr child; + virDomainSnapshotObjPtr last; + + for (child = from->first_child; child; child = child->sibling) { + child->parent = to; + if (!child->sibling) + last = child; + } + to->nchildren += from->nchildren; + last->sibling = to->first_child; + to->first_child = from->first_child; + from->nchildren = 0; + from->first_child = NULL; +} diff --git a/src/conf/virdomainsnapshotobjlist.c b/src/conf/virdomainsnapshotobjlist.c index 1eecb89a5d..9538521ab3 100644 --- a/src/conf/virdomainsnapshotobjlist.c +++ b/src/conf/virdomainsnapshotobjlist.c @@ -72,7 +72,7 @@ virDomainSnapshotObjListParse(const char *xmlStr, _("incorrect flags for bulk parse")); return -1; } - if (snapshots->metaroot.nchildren || snapshots->current) { + if (virDomainSnapshotObjListSize(snapshots)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("bulk define of snapshots only possible with " "no existing snapshot")); @@ -143,9 +143,7 @@ virDomainSnapshotObjListParse(const char *xmlStr, if (ret < 0) { /* There were no snapshots before this call; so on error, just * blindly delete anything created before the failure. */ - virHashRemoveAll(snapshots->objs); - snapshots->metaroot.nchildren = 0; - snapshots->metaroot.first_child = NULL; + virDomainSnapshotObjListRemoveAll(snapshots); } xmlXPathFreeContext(ctxt); xmlFreeDoc(xml); @@ -437,6 +435,14 @@ virDomainSnapshotFindByName(virDomainSnapshotObjListPtr snapshots, } +/* Return the number of objects currently in the list */ +int +virDomainSnapshotObjListSize(virDomainSnapshotObjListPtr snapshots) +{ + return virHashSize(snapshots->objs); +} + + /* Return the current snapshot, or NULL */ virDomainSnapshotObjPtr virDomainSnapshotGetCurrent(virDomainSnapshotObjListPtr snapshots) @@ -484,6 +490,15 @@ bool virDomainSnapshotObjListRemove(virDomainSnapshotObjListPtr snapshots, return ret; } +/* Remove all snapshots tracked in the list */ +void +virDomainSnapshotObjListRemoveAll(virDomainSnapshotObjListPtr snapshots) +{ + virHashRemoveAll(snapshots->objs); + virDomainSnapshotDropChildren(&snapshots->metaroot); +} + + int virDomainSnapshotForEach(virDomainSnapshotObjListPtr snapshots, virHashIterator iter, @@ -511,28 +526,26 @@ virDomainSnapshotSetRelations(void *payload, virDomainSnapshotObjPtr obj = payload; struct snapshot_set_relation *curr = data; virDomainSnapshotObjPtr tmp; + virDomainSnapshotObjPtr parent; - obj->parent = virDomainSnapshotFindByName(curr->snapshots, - obj->def->parent); - if (!obj->parent) { + parent = virDomainSnapshotFindByName(curr->snapshots, obj->def->parent); + if (!parent) { curr->err = -1; - obj->parent = &curr->snapshots->metaroot; + parent = &curr->snapshots->metaroot; VIR_WARN("snapshot %s lacks parent", obj->def->name); } else { - tmp = obj->parent; + tmp = parent; while (tmp && tmp->def) { if (tmp == obj) { curr->err = -1; - obj->parent = &curr->snapshots->metaroot; + parent = &curr->snapshots->metaroot; VIR_WARN("snapshot %s in circular chain", obj->def->name); break; } tmp = tmp->parent; } } - obj->parent->nchildren++; - obj->sibling = obj->parent->first_child; - obj->parent->first_child = obj; + virDomainSnapshotSetParent(obj, parent); return 0; } @@ -545,8 +558,7 @@ virDomainSnapshotUpdateRelations(virDomainSnapshotObjListPtr snapshots) { struct snapshot_set_relation act = { snapshots, 0 }; - snapshots->metaroot.nchildren = 0; - snapshots->metaroot.first_child = NULL; + virDomainSnapshotDropChildren(&snapshots->metaroot); virHashForEach(snapshots->objs, virDomainSnapshotSetRelations, &act); if (act.err) snapshots->current = NULL; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 72c5cef528..ffc1724850 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -980,9 +980,12 @@ virDomainObjListRename; # conf/virdomainsnapshotobj.h +virDomainSnapshotDropChildren; virDomainSnapshotDropParent; virDomainSnapshotForEachChild; virDomainSnapshotForEachDescendant; +virDomainSnapshotMoveChildren; +virDomainSnapshotSetParent; # conf/virdomainsnapshotobjlist.h diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 6c71382b93..eb3d112b69 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15926,10 +15926,7 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, } else { other = virDomainSnapshotFindByName(vm->snapshots, snap->def->parent); - snap->parent = other; - other->nchildren++; - snap->sibling = other->first_child; - other->first_child = snap; + virDomainSnapshotSetParent(snap, other); } } else if (snap) { virDomainSnapshotObjListRemove(vm->snapshots, snap); @@ -16763,7 +16760,6 @@ struct _virQEMUSnapReparent { virCapsPtr caps; virDomainXMLOptionPtr xmlopt; int err; - virDomainSnapshotObjPtr last; }; @@ -16779,7 +16775,6 @@ qemuDomainSnapshotReparentChildren(void *payload, return 0; VIR_FREE(snap->def->parent); - snap->parent = rep->parent; if (rep->parent->def && VIR_STRDUP(snap->def->parent, rep->parent->def->name) < 0) { @@ -16787,9 +16782,6 @@ qemuDomainSnapshotReparentChildren(void *payload, return 0; } - if (!snap->sibling) - rep->last = snap; - rep->err = qemuDomainSnapshotWriteMetadata(rep->vm, snap, rep->caps, rep->xmlopt, rep->cfg->snapshotDir); @@ -16877,7 +16869,6 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, rep.parent = snap->parent; rep.vm = vm; rep.err = 0; - rep.last = NULL; rep.caps = driver->caps; rep.xmlopt = driver->xmlopt; virDomainSnapshotForEachChild(snap, @@ -16885,15 +16876,11 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, &rep); if (rep.err < 0) goto endjob; - /* Can't modify siblings during ForEachChild, so do it now. */ - snap->parent->nchildren += snap->nchildren; - rep.last->sibling = snap->parent->first_child; - snap->parent->first_child = snap->first_child; + virDomainSnapshotMoveChildren(snap, snap->parent); } if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) { - snap->nchildren = 0; - snap->first_child = NULL; + virDomainSnapshotDropChildren(snap); ret = 0; } else { ret = qemuDomainSnapshotDiscard(driver, vm, snap, true, metadata_only); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 9cbef70f1c..d3b76bfdbd 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -6416,10 +6416,7 @@ testDomainSnapshotCreateXML(virDomainPtr domain, virDomainSnapshotSetCurrent(vm->snapshots, snap); other = virDomainSnapshotFindByName(vm->snapshots, snap->def->parent); - snap->parent = other; - other->nchildren++; - snap->sibling = other->first_child; - other->first_child = snap; + virDomainSnapshotSetParent(snap, other); } virDomainObjEndAPI(&vm); } @@ -6454,7 +6451,6 @@ struct _testSnapReparentData { virDomainSnapshotObjPtr parent; virDomainObjPtr vm; int err; - virDomainSnapshotObjPtr last; }; static int @@ -6469,7 +6465,6 @@ testDomainSnapshotReparentChildren(void *payload, return 0; VIR_FREE(snap->def->parent); - snap->parent = rep->parent; if (rep->parent->def && VIR_STRDUP(snap->def->parent, rep->parent->def->name) < 0) { @@ -6477,8 +6472,6 @@ testDomainSnapshotReparentChildren(void *payload, return 0; } - if (!snap->sibling) - rep->last = snap; return 0; } @@ -6515,22 +6508,17 @@ testDomainSnapshotDelete(virDomainSnapshotPtr snapshot, rep.parent = snap->parent; rep.vm = vm; rep.err = 0; - rep.last = NULL; virDomainSnapshotForEachChild(snap, testDomainSnapshotReparentChildren, &rep); if (rep.err < 0) goto cleanup; - /* Can't modify siblings during ForEachChild, so do it now. */ - snap->parent->nchildren += snap->nchildren; - rep.last->sibling = snap->parent->first_child; - snap->parent->first_child = snap->first_child; + virDomainSnapshotMoveChildren(snap, snap->parent); } if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY) { - snap->nchildren = 0; - snap->first_child = NULL; + virDomainSnapshotDropChildren(snap); } else { virDomainSnapshotDropParent(snap); if (snap == virDomainSnapshotGetCurrent(vm->snapshots)) { -- 2.20.1 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list