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

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.

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

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

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

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

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