7:33 p.m.
On 3/20/19 1:41 AM, Eric Blake wrote:
Finish the code motion of generic moment list handling. Well, mostly
-
we need to convert to using virObject to get polymorphic cleanup
functions (so for now there are still a few lingering ties specific to
snapshots). In this case, I kept virDomainSnapshotObjList as a
wrapper type around the new generic virDomainMomentObjList; the bulk
of the algorithms move, but the old functions remain and forward to
the generic helpers.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/conf/virconftypes.h | 3 +
src/conf/virdomainmomentobjlist.h | 32 +++
src/conf/virdomainsnapshotobjlist.h | 6 -
src/conf/virdomainmomentobjlist.c | 356 ++++++++++++++++++++++++++++
src/conf/virdomainsnapshotobjlist.c | 266 +++------------------
5 files changed, 422 insertions(+), 241 deletions(-)
[...]
diff --git a/src/conf/virdomainmomentobjlist.c
b/src/conf/virdomainmomentobjlist.c
index 766d7fe2e4..f987329a6b 100644
--- a/src/conf/virdomainmomentobjlist.c
+++ b/src/conf/virdomainmomentobjlist.c
[...]
+
+/* Add def to the list and return a matching object, or NULL on error */
+virDomainMomentObjPtr
+virDomainMomentAssignDef(virDomainMomentObjListPtr moments,
+ virDomainMomentDefPtr def)
+{
+ virDomainMomentObjPtr moment;
+
+ if (virHashLookup(moments->objs, def->name) != NULL) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unexpected domain moment %s already exists"),
+ def->name);
+ return NULL;
+ }
+
+ if (!(moment = virDomainMomentObjNew()))
+ return NULL;
+ moment->def = def;
I think you may want this after the AddEntry... When/if this converts to
object code w/ virObjectUnref, it leaves the possibility of a double free.
Typically callers have two options - 1. on error free the @def passed 2.
on success set @def = NULL so that cleanup/error processing code doesn't
free it since it's been consumed by the object. In "some" code I'd
create @objdef locals to keep things separate and obvious with of course
*ObjGetDef() calls to populate.
+
+ if (virHashAddEntry(moments->objs, moment->def->name, moment) < 0) {
+ VIR_FREE(moment);
Eventually I assume this will be a virObjectUnref(moment) and that's
when the @def fun would begin...
+ return NULL;
+ }
+
+ return moment;
+}
+
+
[...]
+int
+virDomainMomentObjListGetNames(virDomainMomentObjListPtr moments,
+ virDomainMomentObjPtr from,
+ char **const names,
+ int maxnames,
+ unsigned int flags)
+{
+ struct virDomainMomentNameData data = { names, maxnames, flags, 0,
+ false, moments->filter };
+ size_t i;
+
+ if (!from) {
+ /* LIST_ROOTS and LIST_DESCENDANTS have the same bit value,
+ * but opposite semantics. Toggle here to get the correct
+ * traversal on the metaroot. */
+ flags ^= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS;
Seems like we're crossing paths here _SNAPSHOT_... also below
+ from = &moments->metaroot;
+ }
+
+ /* We handle LIST_ROOT/LIST_DESCENDANTS and LIST_TOPOLOGICAL directly,
+ * mask those bits out to determine when we must use the filter callback. */
+ data.flags &= ~(VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS |
+ VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL);
+
+ /* If this common code is being used, we assume that all snapshots
+ * have metadata, and thus can handle METADATA up front as an
+ * all-or-none filter. XXX This might not always be true, if we
+ * add the ability to track qcow2 internal snapshots without the
+ * use of metadata. */
+ if ((data.flags & VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA) ==
+ VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA)
+ return 0;
+ data.flags &= ~VIR_DOMAIN_SNAPSHOT_FILTERS_METADATA;
This above hunk seems to be duplicated from
virDomainSnapshotObjListGetNames. There's some flags in between that are
left there. So how much of this is reused w/ Checkpoints and how much
technical debt can we absorb short term.
I do see you responded to patch9 with moving some of these definitions.
Is it feasible to split/convert into using _MOMENT_ - I don't want to
burden you with reworks and menial stuff that we can do/fixup later.
I think you know how much more code exists in your branches that could
be impacted. I can leave this decision to you, but it is a concern we
probably need to address at some point.
+
+ if (flags & VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS) {
+ /* We could just always do a topological visit; but it is
+ * possible to optimize for less stack usage and time when a
+ * simpler full hashtable visit or counter will do. */
+ if (from->def || (names &&
+ (flags & VIR_DOMAIN_SNAPSHOT_LIST_TOPOLOGICAL)))
+ virDomainMomentForEachDescendant(from,
+ virDomainMomentObjListCopyNames,
+ &data);
+ else if (names || data.flags)
+ virHashForEach(moments->objs, virDomainMomentObjListCopyNames,
+ &data);
+ else
+ data.count = virHashSize(moments->objs);
+ } else if (names || data.flags) {
+ virDomainMomentForEachChild(from,
+ virDomainMomentObjListCopyNames, &data);
+ } else {
+ data.count = from->nchildren;
+ }
+
+ if (data.error) {
+ for (i = 0; i < data.count; i++)
+ VIR_FREE(names[i]);
+ return -1;
+ }
+
+ return data.count;
+}
+
[...]
diff --git a/src/conf/virdomainsnapshotobjlist.c
b/src/conf/virdomainsnapshotobjlist.c
index 8ecb131176..31ed1c672d 100644
--- a/src/conf/virdomainsnapshotobjlist.c
+++ b/src/conf/virdomainsnapshotobjlist.c
[...]
+
void
virDomainSnapshotObjListFree(virDomainSnapshotObjListPtr snapshots)
{
- if (!snapshots)
- return;
For safety probably should keep the above 2.... Many callers will still
expect to call *ListFree methods even though the argument is NULL.
- virHashFree(snapshots->objs);
+ virDomainMomentObjListFree(snapshots->base);
VIR_FREE(snapshots);
}
[...]
I'll go with this even though it's not perfect - to some degree it may
only be a name change or just technical debt promise of a future patch
and of course review to do something.
I'm not worried yet so much about the *AssignDef note, but it is
something you may want to consider.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John