From ced0898f86bf63876ce4a92a196c62d37c5b119c Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 21 Mar 2019 15:33:21 -0500 Subject: [PATCH] snapshot: Add accessor for reparenting snapshot children 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 starts the task with a single new function: virDomainSnapshotMoveChildren(). The logic might not be immediately obvious [okay, that's an understatement - the existing code uses black magic ;-)], so here's an overview: The old code has an implicit for loop around each call to qemuDomainSnapshotReparentChildren() by using virDomainSnapshotForEachChild() (you'll need a wider context than git's default of 3 lines to see that); the new code has a more visible for loop. Then it helps if you realize that the code is making two separate changes to each child object: STRDUP of the new parent name prior to writing XML files (unchanged), and touching up the pointer to the parent object (refactored); the end result is the same whether a single pass made both changes (both in driver code), or whether it is split into two passes making one change each (one in driver code, the other in the new accessor). Signed-off-by: Eric Blake Reviewed-by: John Ferlan --- src/conf/virdomainsnapshotobj.c | 23 +++++++++++++++++++++++ src/conf/virdomainsnapshotobj.h | 2 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_driver.c | 11 +---------- src/test/test_driver.c | 10 +--------- 5 files changed, 28 insertions(+), 19 deletions(-) diff --git a/src/conf/virdomainsnapshotobj.c b/src/conf/virdomainsnapshotobj.c index 7f92ac21d9..271cd344ff 100644 --- a/src/conf/virdomainsnapshotobj.c +++ b/src/conf/virdomainsnapshotobj.c @@ -121,3 +121,26 @@ virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot) snapshot->parent = NULL; snapshot->sibling = NULL; } + + +/* Take all children of @from and convert them into children of @to. */ +void +virDomainSnapshotMoveChildren(virDomainSnapshotObjPtr from, + virDomainSnapshotObjPtr to) +{ + virDomainSnapshotObjPtr child; + virDomainSnapshotObjPtr last; + + if (!from->first_child) + return; + 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/virdomainsnapshotobj.h b/src/conf/virdomainsnapshotobj.h index 957f1b2ea8..22961729e8 100644 --- a/src/conf/virdomainsnapshotobj.h +++ b/src/conf/virdomainsnapshotobj.h @@ -46,5 +46,7 @@ int virDomainSnapshotForEachDescendant(virDomainSnapshotObjPtr snapshot, virHashIterator iter, void *data); void virDomainSnapshotDropParent(virDomainSnapshotObjPtr snapshot); +void virDomainSnapshotMoveChildren(virDomainSnapshotObjPtr from, + virDomainSnapshotObjPtr to); #endif /* LIBVIRT_VIRDOMAINSNAPSHOTOBJ_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 41d17226f9..22f13ce64d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -983,6 +983,7 @@ virDomainObjListRename; virDomainSnapshotDropParent; virDomainSnapshotForEachChild; virDomainSnapshotForEachDescendant; +virDomainSnapshotMoveChildren; # conf/virdomainsnapshotobjlist.h diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 3479098e9c..bc9b6d2b80 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16763,7 +16763,6 @@ struct _virQEMUSnapReparent { virCapsPtr caps; virDomainXMLOptionPtr xmlopt; int err; - virDomainSnapshotObjPtr last; }; @@ -16779,7 +16778,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 +16785,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 +16872,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,10 +16879,7 @@ 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) { diff --git a/src/test/test_driver.c b/src/test/test_driver.c index f73648fbe1..d68a5ce1b1 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -6454,7 +6454,6 @@ struct _testSnapReparentData { virDomainSnapshotObjPtr parent; virDomainObjPtr vm; int err; - virDomainSnapshotObjPtr last; }; static int @@ -6469,7 +6468,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 +6475,6 @@ testDomainSnapshotReparentChildren(void *payload, return 0; } - if (!snap->sibling) - rep->last = snap; return 0; } @@ -6515,17 +6511,13 @@ 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) {