Re: [libvirt] [PATCH 07/16] snapshot: Add accessors for updating snapshot list relations
On 3/20/19 1:40 AM, Eric Blake wrote:
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.
Understatement ;-) Some black magic occurs
The qemuDomainSnapshotReparentChildren "snap->parent = rep->parent" gets
replaced by the new for loop... Tough to see without watching really
closely.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
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.c
b/src/conf/virdomainsnapshotobj.c
index 7f92ac21d9..d6b216c7b2 100644
--- a/src/conf/virdomainsnapshotobj.c
+++ b/src/conf/virdomainsnapshotobj.c
[...]
+/* 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;
Silly Coverity compiler gets quite confused thinking that @last couldn't
be set while not considering the above loop couldn't end without it
unless of course from->first_child == NULL I suppose, which would be a
different issue. Still if before the for loop we check "if
(!from->first_child) return;", then coverity is happy.
+ to->first_child = from->first_child;
+ from->nchildren = 0;
+ from->first_child = NULL;
Or virDomainSnapshotDropChildren... Still makes sense, albeit new. Seems
to make VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY processing unnecessary.
It's not a problem but perhaps worthy of a mention.
+}
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)) {
The only way the call could return < 0 from virHashSize is if @snapshots
== NULL... Just noting it - no problem.
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("bulk define of snapshots only possible with "
"no existing snapshot"));
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
BTW: There could have been 3 patches out of this, but I'm fine with 1.