[libvirt] [PATCH v8 07/21] backup: Allow for lists of checkpoint objects

[Eric Blake]

On 4/25/19 7:45 AM, Peter Krempa wrote:
> On Wed, Apr 17, 2019 at 09:09:07 -0500, Eric Blake wrote:
>> Create a new file for managing a list of checkpoint objects, borrowing
>> heavily from existing virDomainSnapshotObjList paradigms.
>>
>> Note that while checkpoints definitely have a use case for multiple
>> children to a single parent (create a base snapshot, create a child
>> snapshot, revert to the base, then create another child snapshot),
>> it's harder to predict how checkpoints will play out with reverting to
>> prior points in time. Thus, in initial use, we may find that in a list
>> of checkpoints, you never have more than one child.  However, as the
>> snapshot machinery is already generic, it is easier to reuse the
>> generic machinery that tracks relations between domain moments than it
>> is to open-code a new list-management scheme just for checkpoints.
>>
>> The virDomainMomentObjList code is not quite polymorphic enough until
>> we patch virDomainMomentDef to be a proper virObject, but doing that
>> is a bigger audit.  So for now, I had to duplicate the cleanup calls
>> based on a bool flag for which type needs cleaning. Oh well.
> 
> I'm afraid that with this being committed the motivation to refactor it
> properly will be gone.

Then I'll try harder to get that audit done before v9.


>> +++ b/src/conf/virdomaincheckpointobjlist.c
>> @@ -0,0 +1,223 @@
> 
> [...]
> 
>> +
>> +#define VIR_FROM_THIS VIR_FROM_DOMAIN_CHECKPOINT
>> +
>> +VIR_LOG_INIT("conf.virdomaincheckpointobjlist");
>> +
>> +struct _virDomainCheckpointObjList {
>> +    virDomainMomentObjListPtr base;
>> +};
> 
> Is this just for compile time typechecking?

Pretty much (but also because I don't have the VirObject inheritance
going, so doing that for snapshots may change what we need here as well).


>> +static int
>> +virDomainCheckpointObjListGetNames(virDomainCheckpointObjListPtr checkpoints,
>> +                                   virDomainMomentObjPtr from,
>> +                                   char **const names,
>> +                                   int maxnames,
>> +                                   unsigned int flags)
>> +{
>> +    /* We intentionally chose our public flags to match the common flags */
>> +    verify(VIR_DOMAIN_CHECKPOINT_LIST_ROOTS ==
>> +           (int) VIR_DOMAIN_MOMENT_LIST_ROOTS);
>> +    verify(VIR_DOMAIN_CHECKPOINT_LIST_TOPOLOGICAL ==
>> +           (int) VIR_DOMAIN_MOMENT_LIST_TOPOLOGICAL);
>> +    verify(VIR_DOMAIN_CHECKPOINT_LIST_LEAVES ==
>> +           (int) VIR_DOMAIN_MOMENT_LIST_LEAVES);
>> +    verify(VIR_DOMAIN_CHECKPOINT_LIST_NO_LEAVES ==
>> +           (int) VIR_DOMAIN_MOMENT_LIST_NO_LEAVES);
>> +    verify(VIR_DOMAIN_CHECKPOINT_LIST_METADATA ==
>> +           (int) VIR_DOMAIN_MOMENT_LIST_METADATA);
>> +    verify(VIR_DOMAIN_CHECKPOINT_LIST_NO_METADATA ==
>> +           (int) VIR_DOMAIN_MOMENT_LIST_NO_METADATA);
> 
> This looks like it should be near the VIR_DOMAIN_CHECKPOINT_LIST enum
> rather than somewhere in the code randomly.

Okay, I can try to move it.


>> +
>> +static void
>> +virDomainSnapshotObjListDataFree(void *payload,
>> +                                 const void *name ATTRIBUTE_UNUSED)
>> +{
>> +    virDomainMomentObjPtr obj = payload;
>> +
>> +    virDomainSnapshotObjFree(obj);
>>  }
>>
>>
>>  virDomainMomentObjListPtr
>> -virDomainMomentObjListNew(void)
>> +virDomainMomentObjListNew(bool snapshot)
> 
> Please create a new entrypoint for this. I don't see the need to
> overload it using the argument.

Better than a bool would be a virClassPtr that describes the class of
the virObject to construct. So yes, making things properly polymorphic
is worthwhile to make this code cleaner.

> 
>>  {
>>      virDomainMomentObjListPtr moments;
>>
>>      if (VIR_ALLOC(moments) < 0)
>>          return NULL;
>> -    moments->objs = virHashCreate(50, virDomainMomentObjListDataFree);
>> +    moments->objs = virHashCreate(50,
>> +                                  snapshot ? virDomainSnapshotObjListDataFree :
>> +                                  virDomainCheckpointObjListDataFree);

With virObject in play, the destructor is already automatically
polymorphic (with virObjectUnref() being the common call to trigger the
correct cleanup).

>>      if (!moments->objs) {
>>          VIR_FREE(moments);
>>          return NULL;

-- 
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/20190506/08af01ee/attachment-0001.sig>