From 02c4e24db71213c0eec81461f4a3093fdfda9b7e Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Sat, 16 Mar 2019 22:38:33 -0500 Subject: [PATCH] snapshot: Add accessors for updating snapshot list relations 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. This patch finishes the job started in the previous patch, by getting rid of all direct access to nchildren, first_child, or sibling outside of the lowest level functions, making it easier to refactor later on. The lone new caller to virDomainSnapshotObjListSize() checks for a return != 0, because it wants to handles errors (-1, only possible if the hash table wasn't allocated) and existing snapshots (> 0) in the same manner; we can drop the check for a current snapshot on the grounds that there shouldn't be one if there are no snapshots. Signed-off-by: Eric Blake Reviewed-by: John Ferlan --- src/conf/virdomainsnapshotobj.c | 21 +++++++++++++++ src/conf/virdomainsnapshotobj.h | 3 +++ src/conf/virdomainsnapshotobjlist.c | 42 ++++++++++++++++++----------- src/conf/virdomainsnapshotobjlist.h | 2 ++ src/libvirt_private.syms | 3 +++ src/qemu/qemu_domain.c | 3 +-- src/qemu/qemu_driver.c | 8 ++---- src/test/test_driver.c | 8 ++---- 8 files changed, 61 insertions(+), 29 deletions(-) diff --git a/src/conf/virdomainsnapshotobj.c b/src/conf/virdomainsnapshotobj.c index 271cd344ff..5dba27564c 100644 --- a/src/conf/virdomainsnapshotobj.c +++ b/src/conf/virdomainsnapshotobj.c @@ -123,6 +123,27 @@ virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot) } +/* 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, diff --git a/src/conf/virdomainsnapshotobj.h b/src/conf/virdomainsnapshotobj.h index 22961729e8..0981ea4c25 100644 --- a/src/conf/virdomainsnapshotobj.h +++ b/src/conf/virdomainsnapshotobj.h @@ -46,7 +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.c b/src/conf/virdomainsnapshotobjlist.c index ed3acbc913..545acd48bc 100644 --- a/src/conf/virdomainsnapshotobjlist.c +++ b/src/conf/virdomainsnapshotobjlist.c @@ -74,7 +74,7 @@ virDomainSnapshotObjListParse(const char *xmlStr, _("incorrect flags for bulk parse")); return -1; } - if (snapshots->metaroot.nchildren || snapshots->current) { + if (virDomainSnapshotObjListSize(snapshots) != 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("bulk define of snapshots only possible with " "no existing snapshot")); @@ -140,9 +140,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); @@ -436,6 +434,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 @@ 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/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/libvirt_private.syms b/src/libvirt_private.syms index 22f13ce64d..164f79d1af 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -980,10 +980,12 @@ virDomainObjListRename; # conf/virdomainsnapshotobj.h +virDomainSnapshotDropChildren; virDomainSnapshotDropParent; virDomainSnapshotForEachChild; virDomainSnapshotForEachDescendant; virDomainSnapshotMoveChildren; +virDomainSnapshotSetParent; # conf/virdomainsnapshotobjlist.h @@ -1001,6 +1003,7 @@ virDomainSnapshotObjListNew; virDomainSnapshotObjListNum; virDomainSnapshotObjListParse; virDomainSnapshotObjListRemove; +virDomainSnapshotObjListRemoveAll; virDomainSnapshotSetCurrent; virDomainSnapshotUpdateRelations; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 511b17cc83..b4f21a85c4 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8679,8 +8679,7 @@ qemuDomainSnapshotDiscardAllMetadata(virQEMUDriverPtr driver, rem.err = 0; virDomainSnapshotForEach(vm->snapshots, qemuDomainSnapshotDiscardAll, &rem); - if (virDomainSnapshotUpdateRelations(vm->snapshots) < 0 && !rem.err) - rem.err = -1; + virDomainSnapshotObjListRemoveAll(vm->snapshots); return rem.err; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index bc9b6d2b80..bf7a34a280 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); @@ -16883,8 +16880,7 @@ qemuDomainSnapshotDelete(virDomainSnapshotPtr snapshot, } 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 d68a5ce1b1..a5d0bd9cc5 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); } @@ -6521,8 +6518,7 @@ testDomainSnapshotDelete(virDomainSnapshotPtr snapshot, } 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)) {