[libvirt] [PATCH 14/16] snapshot: Move snapshot list code into generic file
John Ferlan
jferlan at redhat.com
Fri Mar 22 00:33:16 UTC 2019
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 at 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 at redhat.com>
John
More information about the libvir-list
mailing list