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@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@redhat.com> John