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