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
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
