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.
>
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
> src/conf/checkpoint_conf.h | 7 +
> src/conf/domain_conf.h | 2 +
> src/conf/virconftypes.h | 6 +
> src/conf/virdomaincheckpointobjlist.h | 74 +++++++++
> src/conf/virdomainmomentobjlist.h | 2 +-
> src/conf/virdomainobjlist.h | 7 +-
> src/conf/Makefile.inc.am | 2 +
> src/conf/checkpoint_conf.c | 86 ++++++++++
> src/conf/domain_conf.c | 6 +
> src/conf/virdomaincheckpointobjlist.c | 223 ++++++++++++++++++++++++++
> src/conf/virdomainmomentobjlist.c | 40 ++++-
> src/conf/virdomainobjlist.c | 11 ++
> src/conf/virdomainsnapshotobjlist.c | 2 +-
> src/libvirt_private.syms | 18 +++
> 14 files changed, 477 insertions(+), 9 deletions(-)
> create mode 100644 src/conf/virdomaincheckpointobjlist.h
> create mode 100644 src/conf/virdomaincheckpointobjlist.c
[...]
> diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c
> index 87f76e6518..cb0d71936f 100644
> --- a/src/conf/checkpoint_conf.c
> +++ b/src/conf/checkpoint_conf.c
> @@ -35,6 +35,7 @@
> #include "virerror.h"
> #include "virxml.h"
> #include "virstring.h"
> +#include "virdomaincheckpointobjlist.h"
>
> #define VIR_FROM_THIS VIR_FROM_DOMAIN_CHECKPOINT
>
> @@ -548,3 +549,88 @@ virDomainCheckpointDefFormat(virDomainCheckpointDefPtr def,
>
> return virBufferContentAndReset(&buf);
> }
> +
> +
> +int
> +virDomainCheckpointRedefinePrep(virDomainPtr domain,
> + virDomainObjPtr vm,
> + virDomainCheckpointDefPtr *defptr,
> + virDomainMomentObjPtr *chk,
> + virDomainXMLOptionPtr xmlopt,
> + bool *update_current)
> +{
> + virDomainCheckpointDefPtr def = *defptr;
> + char uuidstr[VIR_UUID_STRING_BUFLEN];
> + virDomainMomentObjPtr other;
> + virDomainCheckpointDefPtr otherdef;
> +
> + virUUIDFormat(domain->uuid, uuidstr);
> +
> + /* TODO: Move this and snapshot version into virDomainMoment */
> + /* Prevent circular chains */
> + if (def->common.parent) {
> + if (STREQ(def->common.name, def->common.parent)) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("cannot set checkpoint %s as its own parent"),
> + def->common.name);
> + return -1;
> + }
> + other = virDomainCheckpointFindByName(vm->checkpoints, def->common.parent);
> + if (!other) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("parent %s for checkpoint %s not found"),
> + def->common.parent, def->common.name);
> + return -1;
> + }
> + otherdef = virDomainCheckpointObjGetDef(other);
> + while (otherdef->common.parent) {
> + if (STREQ(otherdef->common.parent, def->common.name)) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("parent %s would create cycle to %s"),
> + otherdef->common.name, def->common.name);
> + return -1;
> + }
> + other = virDomainCheckpointFindByName(vm->checkpoints,
> + otherdef->common.parent);
> + if (!other) {
> + VIR_WARN("checkpoints are inconsistent for %s",
> + vm->def->name);
> + break;
> + }
> + otherdef = virDomainCheckpointObjGetDef(other);
> + }
> + }
> +
> + if (!def->common.dom ||
> + memcmp(def->common.dom->uuid, domain->uuid, VIR_UUID_BUFLEN)) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("definition for checkpoint %s must use uuid %s"),
> + def->common.name, uuidstr);
> + return -1;
> + }
> + if (virDomainCheckpointAlignDisks(def) < 0)
> + return -1;
> +
> + other = virDomainCheckpointFindByName(vm->checkpoints, def->common.name);
> + otherdef = other ? virDomainCheckpointObjGetDef(other) : NULL;
> + if (other) {
> + if (!virDomainDefCheckABIStability(otherdef->common.dom,
> + def->common.dom, xmlopt))
> + return -1;
> +
> + if (other == virDomainCheckpointGetCurrent(vm->checkpoints)) {
> + *update_current = true;
> + virDomainCheckpointSetCurrent(vm->checkpoints, NULL);
> + }
> +
> + /* Drop and rebuild the parent relationship, but keep all
> + * child relations by reusing chk. */
> + virDomainMomentDropParent(other);
> + virDomainCheckpointDefFree(otherdef);
> + other->def = &(*defptr)->common;
> + *defptr = NULL;
> + *chk = other;
> + }
> +
> + return 0;
> +}
[...]
> diff --git a/src/conf/virdomaincheckpointobjlist.c b/src/conf/virdomaincheckpointobjlist.c
> new file mode 100644
> index 0000000000..dbc1c25c5e
> --- /dev/null
> +++ 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?
> +virDomainMomentObjPtr
> +virDomainCheckpointAssignDef(virDomainCheckpointObjListPtr checkpoints,
> + virDomainCheckpointDefPtr def)
> +{
> + return virDomainMomentAssignDef(checkpoints->base, &def->common);
> +}
> +
> +
> +static bool
> +virDomainCheckpointFilter(virDomainMomentObjPtr obj ATTRIBUTE_UNUSED,
> + unsigned int flags)
> +{
> + /* For now, we have no further filters than what the common code handles. */
> + virCheckFlags(0, false);
> + return true;
> +}
> +
> +
> +virDomainCheckpointObjListPtr
> +virDomainCheckpointObjListNew(void)
> +{
> + virDomainCheckpointObjListPtr checkpoints;
> +
> + if (VIR_ALLOC(checkpoints) < 0)
> + return NULL;
> + checkpoints->base = virDomainMomentObjListNew(false);
> + if (!checkpoints->base) {
> + VIR_FREE(checkpoints);
> + return NULL;
> + }
> + return checkpoints;
> +}
> +
> +
> +void
> +virDomainCheckpointObjListFree(virDomainCheckpointObjListPtr checkpoints)
> +{
> + if (!checkpoints)
> + return;
> + virDomainMomentObjListFree(checkpoints->base);
> + VIR_FREE(checkpoints);
> +}
> +
> +
> +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.
> +
> + return virDomainMomentObjListGetNames(checkpoints->base, from, names,
> + maxnames, flags,
> + virDomainCheckpointFilter, 0);
> +}
[...]
> diff --git a/src/conf/virdomainmomentobjlist.c b/src/conf/virdomainmomentobjlist.c
> index 7f90632a53..3dd6da7acb 100644
> --- a/src/conf/virdomainmomentobjlist.c
> +++ b/src/conf/virdomainmomentobjlist.c
[...]
> @@ -242,23 +258,35 @@ virDomainMomentAssignDef(virDomainMomentObjListPtr moments,
>
>
> static void
> -virDomainMomentObjListDataFree(void *payload,
> - const void *name ATTRIBUTE_UNUSED)
> +virDomainCheckpointObjListDataFree(void *payload,
> + const void *name ATTRIBUTE_UNUSED)
> {
> virDomainMomentObjPtr obj = payload;
>
> - virDomainMomentObjFree(obj);
> + virDomainCheckpointObjFree(obj);
> +}
> +
> +
> +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;
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
