[libvirt] [PATCH 14/16] snapshot: Move snapshot list code into generic file

[Eric Blake]

On 3/21/19 7:33 PM, John Ferlan wrote:
> 
> 
> 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(-)
>>
> 

>> +
>> +/* 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.

Straight code motion for now, so that fix is worth a separate patch (but
one I can make quickly as it is trivial).

> 
> 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);

Indeed, swapping the two lines makes more sense for a future conversion
to virObjectUnref(moment).


>> +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

Yes, my original idea was using VIR_DOMAIN_CHECKPOINT_LIST_ROOTS =
VIR_DOMAIN_SNAPSHOT_LIST_ROOTS to ensure that the bit values that the
generic code uses are indeed identical. And,

> 
>> +        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.

the intent is that this particular flag is easy to handle in common code
(so again, the new API flags for checkpoint would reuse the snapshot bit
value).  But your idea:

> 
> 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.

Having a new internal-only enum in the public headers that both snapshot
and checkpoint reuse, rather than having checkpoint equal to snapshot,
sounds even nicer.  I'll do that as a separate patch still needing
review (since it touches public API).


>> +++ 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.

Ouch - you caught me on rebase churn. At one point, I was toying with
inheritance:

struct _virDomainSnapshotObjList {
  virDomainMomentObjList parent;
}

but without virObject, it didn't work right, so for this patch, I
switched back to container:

struct _virDomainSnapshotObjList {
  virDomainMomentObjListPtr base;
}

but forgot to undo my tweak here.


> 
> [...]
> 
> 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
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190321/29415278/attachment-0001.sig>