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(a)redhat.com>
---
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;