Add a new file checkpoint_conf.c that performs the translation to and
from new XML describing a checkpoint. The code shares a common base
class with snapshots, since a checkpoint similarly represents the
domain state at a moment in time. Add some basic testing of round trip
XML handling through the new code.
Of note - this code intentionally differs from snapshots in that XML
schema validation is unconditional, rather than based on a public API
flag. Also, the redefine flag requires the <domain> sub-element to be
present, rather than catering to historical back-compat to older
versions.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
src/conf/checkpoint_conf.h | 86 +++
src/conf/virconftypes.h | 3 +
po/POTFILES | 1 +
src/conf/Makefile.inc.am | 2 +
src/conf/checkpoint_conf.c | 576 ++++++++++++++++++
src/libvirt_private.syms | 8 +
tests/Makefile.am | 9 +-
.../internal-active-invalid.xml | 53 ++
.../internal-inactive-invalid.xml | 53 ++
tests/qemudomaincheckpointxml2xmltest.c | 213 +++++++
10 files changed, 1002 insertions(+), 2 deletions(-)
create mode 100644 src/conf/checkpoint_conf.h
create mode 100644 src/conf/checkpoint_conf.c
create mode 100644 tests/qemudomaincheckpointxml2xmlout/internal-active-invalid.xml
create mode 100644 tests/qemudomaincheckpointxml2xmlout/internal-inactive-invalid.xml
create mode 100644 tests/qemudomaincheckpointxml2xmltest.c
diff --git a/src/conf/checkpoint_conf.h b/src/conf/checkpoint_conf.h
new file mode 100644
index 0000000000..6215549a20
--- /dev/null
+++ b/src/conf/checkpoint_conf.h
@@ -0,0 +1,86 @@
+/*
+ * checkpoint_conf.h: domain checkpoint XML processing
+ * (based on snapshot_conf.h)
+ *
+ * Copyright (C) 2006-2019 Red Hat, Inc.
+ * Copyright (C) 2006-2008 Daniel P. Berrange
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>.
+ */
+
+#pragma once
+
+#include "internal.h"
+#include "domain_conf.h"
+#include "moment_conf.h"
+#include "virobject.h"
+
+/* Items related to checkpoint state */
+
+typedef enum {
+ VIR_DOMAIN_CHECKPOINT_TYPE_DEFAULT = 0,
+ VIR_DOMAIN_CHECKPOINT_TYPE_NONE,
+ VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP,
+
+ VIR_DOMAIN_CHECKPOINT_TYPE_LAST
+} virDomainCheckpointType;
+
+/* Stores disk-checkpoint information */
+typedef struct _virDomainCheckpointDiskDef virDomainCheckpointDiskDef;
+typedef virDomainCheckpointDiskDef *virDomainCheckpointDiskDefPtr;
+struct _virDomainCheckpointDiskDef {
+ char *name; /* name matching the <target dev='...' of the domain */
+ int idx; /* index within checkpoint->dom->disks that matches name */
+ int type; /* virDomainCheckpointType */
+ char *bitmap; /* bitmap name, if type is bitmap */
+ unsigned long long size; /* current checkpoint size in bytes */
+};
+
+/* Stores the complete checkpoint metadata */
+struct _virDomainCheckpointDef {
+ virDomainMomentDef parent;
+
+ /* Additional Public XML. */
+ size_t ndisks; /* should not exceed dom->ndisks */
+ virDomainCheckpointDiskDef *disks;
+};
+
+
+typedef enum {
+ VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE = 1 << 0,
+} virDomainCheckpointParseFlags;
+
+typedef enum {
+ VIR_DOMAIN_CHECKPOINT_FORMAT_SECURE = 1 << 0,
+ VIR_DOMAIN_CHECKPOINT_FORMAT_NO_DOMAIN = 1 << 1,
+ VIR_DOMAIN_CHECKPOINT_FORMAT_SIZE = 1 << 2,
+ VIR_DOMAIN_CHECKPOINT_FORMAT_CURRENT = 1 << 3,
+} virDomainCheckpointFormatFlags;
+
+unsigned int virDomainCheckpointFormatConvertXMLFlags(unsigned int flags);
+
+virDomainCheckpointDefPtr virDomainCheckpointDefParseString(const char *xmlStr,
+ virCapsPtr caps,
+ virDomainXMLOptionPtr xmlopt,
+ bool *current,
+ unsigned int flags);
+virDomainCheckpointDefPtr virDomainCheckpointDefNew(void);
+char *virDomainCheckpointDefFormat(virDomainCheckpointDefPtr def,
+ virCapsPtr caps,
+ virDomainXMLOptionPtr xmlopt,
+ unsigned int flags);
+int virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr checkpoint);
+
+VIR_ENUM_DECL(virDomainCheckpoint);
diff --git a/src/conf/virconftypes.h b/src/conf/virconftypes.h
index b8f553f7fb..fe3f0af14f 100644
--- a/src/conf/virconftypes.h
+++ b/src/conf/virconftypes.h
@@ -102,6 +102,9 @@ typedef virDomainBlkiotune *virDomainBlkiotunePtr;
typedef struct _virDomainBlockIoTuneInfo virDomainBlockIoTuneInfo;
typedef virDomainBlockIoTuneInfo *virDomainBlockIoTuneInfoPtr;
+typedef struct _virDomainCheckpointDef virDomainCheckpointDef;
+typedef virDomainCheckpointDef *virDomainCheckpointDefPtr;
+
typedef struct _virDomainChrDef virDomainChrDef;
typedef virDomainChrDef *virDomainChrDefPtr;
diff --git a/po/POTFILES b/po/POTFILES
index 21eea80329..adf907ed93 100644
--- a/po/POTFILES
+++ b/po/POTFILES
@@ -15,6 +15,7 @@ src/bhyve/bhyve_monitor.c
src/bhyve/bhyve_parse_command.c
src/bhyve/bhyve_process.c
src/conf/capabilities.c
+src/conf/checkpoint_conf.c
src/conf/cpu_conf.c
src/conf/device_conf.c
src/conf/domain_addr.c
diff --git a/src/conf/Makefile.inc.am b/src/conf/Makefile.inc.am
index 6b52ba674b..f0135bcb1c 100644
--- a/src/conf/Makefile.inc.am
+++ b/src/conf/Makefile.inc.am
@@ -14,6 +14,8 @@ NETDEV_CONF_SOURCES = \
DOMAIN_CONF_SOURCES = \
conf/capabilities.c \
conf/capabilities.h \
+ conf/checkpoint_conf.c \
+ conf/checkpoint_conf.h \
conf/domain_addr.c \
conf/domain_addr.h \
conf/domain_capabilities.c \
diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c
new file mode 100644
index 0000000000..d50d63ac0e
--- /dev/null
+++ b/src/conf/checkpoint_conf.c
@@ -0,0 +1,576 @@
+/*
+ * checkpoint_conf.c: domain checkpoint XML processing
+ *
+ * Copyright (C) 2006-2019 Red Hat, Inc.
+ * Copyright (C) 2006-2008 Daniel P. Berrange
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>.
+ */
+
+#include <config.h>
+
+#include "configmake.h"
+#include "internal.h"
+#include "virbitmap.h"
+#include "virbuffer.h"
+#include "datatypes.h"
+#include "domain_conf.h"
+#include "virlog.h"
+#include "viralloc.h"
+#include "checkpoint_conf.h"
+#include "virstoragefile.h"
+#include "viruuid.h"
+#include "virfile.h"
+#include "virerror.h"
+#include "virxml.h"
+#include "virstring.h"
+
+#define VIR_FROM_THIS VIR_FROM_DOMAIN_CHECKPOINT
+
+VIR_LOG_INIT("conf.checkpoint_conf");
+
+static virClassPtr virDomainCheckpointDefClass;
+static void virDomainCheckpointDefDispose(void *obj);
+
+static int
+virDomainCheckpointOnceInit(void)
+{
+ if (!VIR_CLASS_NEW(virDomainCheckpointDef, virClassForDomainMomentDef()))
+ return -1;
+
+ return 0;
+}
+
+VIR_ONCE_GLOBAL_INIT(virDomainCheckpoint);
+
+VIR_ENUM_IMPL(virDomainCheckpoint,
+ VIR_DOMAIN_CHECKPOINT_TYPE_LAST,
+ "default", "no", "bitmap");
+
+
+/* Checkpoint Def functions */
+static void
+virDomainCheckpointDiskDefClear(virDomainCheckpointDiskDefPtr disk)
+{
+ VIR_FREE(disk->name);
+ VIR_FREE(disk->bitmap);
+}
+
+/* Allocate a new virDomainCheckpointDef; free with virObjectUnref() */
+virDomainCheckpointDefPtr
+virDomainCheckpointDefNew(void)
+{
+ virDomainCheckpointDefPtr def;
+
+ if (virDomainCheckpointInitialize() < 0)
+ return NULL;
+
+ def = virObjectNew(virDomainCheckpointDefClass);
+ return def;
+}
+
+static void
+virDomainCheckpointDefDispose(void *obj)
+{
+ virDomainCheckpointDefPtr def = obj;
+ size_t i;
+
+ for (i = 0; i < def->ndisks; i++)
+ virDomainCheckpointDiskDefClear(&def->disks[i]);
+ VIR_FREE(def->disks);
+}
+
+static int
+virDomainCheckpointDiskDefParseXML(xmlNodePtr node,
+ xmlXPathContextPtr ctxt,
+ virDomainCheckpointDiskDefPtr def)
+{
+ int ret = -1;
+ char *checkpoint = NULL;
+ char *bitmap = NULL;
+ xmlNodePtr saved = ctxt->node;
+
+ ctxt->node = node;
+
+ def->name = virXMLPropString(node, "name");
+ if (!def->name) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("missing name from disk checkpoint element"));
+ goto cleanup;
+ }
+
+ checkpoint = virXMLPropString(node, "checkpoint");
+ if (checkpoint) {
+ def->type = virDomainCheckpointTypeFromString(checkpoint);
+ if (def->type <= 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown disk checkpoint setting '%s'"),
+ checkpoint);
+ goto cleanup;
+ }
+ } else {
+ def->type = VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP;
+ }
+
+ bitmap = virXMLPropString(node, "bitmap");
+ if (bitmap) {
+ if (def->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("disk checkpoint bitmap '%s' requires "
+ "type='bitmap'"),
+ bitmap);
+ goto cleanup;
+ }
+ VIR_STEAL_PTR(def->bitmap, bitmap);
+ }
+
+ ret = 0;
+ cleanup:
+ ctxt->node = saved;
+
+ VIR_FREE(checkpoint);
+ VIR_FREE(bitmap);
+ if (ret < 0)
+ virDomainCheckpointDiskDefClear(def);
+ return ret;
+}
+
+/* flags is bitwise-or of virDomainCheckpointParseFlags. If flags
+ * does not include VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE, then caps
+ * and current are ignored.
+ */
+static virDomainCheckpointDefPtr
+virDomainCheckpointDefParse(xmlXPathContextPtr ctxt,
+ virCapsPtr caps,
+ virDomainXMLOptionPtr xmlopt,
+ bool *current,
+ unsigned int flags)
+{
+ virDomainCheckpointDefPtr def = NULL;
+ virDomainCheckpointDefPtr ret = NULL;
+ xmlNodePtr *nodes = NULL;
+ size_t i;
+ int n;
+ char *creation = NULL;
+ int active;
+ char *tmp;
+
+ if (!(def = virDomainCheckpointDefNew()))
+ return NULL;
+
+ def->parent.name = virXPathString("string(./name)", ctxt);
+ if (def->parent.name == NULL) {
+ if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE) {
+ virReportError(VIR_ERR_XML_ERROR, "%s",
+ _("a redefined checkpoint must have a name"));
+ goto cleanup;
+ }
+ }
+
+ def->parent.description = virXPathString("string(./description)", ctxt);
+
+ if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE) {
+ if (virXPathLongLong("string(./creationTime)", ctxt,
+ &def->parent.creationTime) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("missing creationTime from existing checkpoint"));
+ goto cleanup;
+ }
+
+ def->parent.parent_name = virXPathString("string(./parent/name)", ctxt);
+
+ if (virXPathInt("string(./current)", ctxt, &active) < 0 || !current) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("missing current from existing checkpoint"));
+ goto cleanup;
+ }
+ *current = !!active;
+
+ if ((tmp = virXPathString("string(./domain/@type)", ctxt))) {
+ int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
+ VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
+ xmlNodePtr domainNode = virXPathNode("./domain", ctxt);
+
+ VIR_FREE(tmp);
+ if (!domainNode) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("missing domain in checkpoint"));
+ goto cleanup;
+ }
+ def->parent.dom = virDomainDefParseNode(ctxt->node->doc, domainNode,
+ caps, xmlopt, NULL,
+ domainflags);
+ if (!def->parent.dom)
+ goto cleanup;
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("missing domain in checkpoint redefine"));
+ goto cleanup;
+ }
+ } else if (virDomainXMLOptionRunMomentPostParse(xmlopt, &def->parent) < 0) {
+ goto cleanup;
+ }
+
+ if ((n = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0)
+ goto cleanup;
+ if (n && VIR_ALLOC_N(def->disks, n) < 0)
+ goto cleanup;
+ def->ndisks = n;
+ for (i = 0; i < def->ndisks; i++) {
+ if (virDomainCheckpointDiskDefParseXML(nodes[i], ctxt,
+ &def->disks[i]) < 0)
+ goto cleanup;
+ }
+
+ VIR_STEAL_PTR(ret, def);
+
+ cleanup:
+ VIR_FREE(creation);
+ VIR_FREE(nodes);
+ virObjectUnref(def);
+
+ return ret;
+}
+
+static virDomainCheckpointDefPtr
+virDomainCheckpointDefParseNode(xmlDocPtr xml,
+ xmlNodePtr root,
+ virCapsPtr caps,
+ virDomainXMLOptionPtr xmlopt,
+ bool *current,
+ unsigned int flags)
+{
+ xmlXPathContextPtr ctxt = NULL;
+ virDomainCheckpointDefPtr def = NULL;
+ VIR_AUTOFREE(char *) schema = NULL;
+
+ if (!virXMLNodeNameEqual(root, "domaincheckpoint")) {
+ virReportError(VIR_ERR_XML_ERROR, "%s", _("domaincheckpoint"));
+ goto cleanup;
+ }
+
+ /* This is a new enough API to make schema validation unconditional */
+ schema = virFileFindResource("domaincheckpoint.rng",
+ abs_top_srcdir "/docs/schemas",
+ PKGDATADIR "/schemas");
+ if (!schema)
+ return NULL;
+ if (virXMLValidateAgainstSchema(schema, xml) < 0)
+ return NULL;
+
+ ctxt = xmlXPathNewContext(xml);
+ if (ctxt == NULL) {
+ virReportOOMError();
+ goto cleanup;
+ }
+
+ ctxt->node = root;
+ def = virDomainCheckpointDefParse(ctxt, caps, xmlopt, current, flags);
+ cleanup:
+ xmlXPathFreeContext(ctxt);
+ return def;
+}
+
+virDomainCheckpointDefPtr
+virDomainCheckpointDefParseString(const char *xmlStr,
+ virCapsPtr caps,
+ virDomainXMLOptionPtr xmlopt,
+ bool *current,
+ unsigned int flags)
+{
+ virDomainCheckpointDefPtr ret = NULL;
+ xmlDocPtr xml;
+ int keepBlanksDefault = xmlKeepBlanksDefault(0);
+
+ if ((xml = virXMLParse(NULL, xmlStr, _("(domain_checkpoint)")))) {
+ xmlKeepBlanksDefault(keepBlanksDefault);
+ ret = virDomainCheckpointDefParseNode(xml, xmlDocGetRootElement(xml),
+ caps, xmlopt, current, flags);
+ xmlFreeDoc(xml);
+ }
+ xmlKeepBlanksDefault(keepBlanksDefault);
+
+ return ret;
+}
+
+
+/**
+ * virDomainCheckpointDefAssignBitmapNames:
+ * @def: checkpoint def object
+ *
+ * Generate default bitmap names for checkpoint targets. Returns 0 on
+ * success, -1 on error.
+ */
+static int
+virDomainCheckpointDefAssignBitmapNames(virDomainCheckpointDefPtr def)
+{
+ size_t i;
+
+ for (i = 0; i < def->ndisks; i++) {
+ virDomainCheckpointDiskDefPtr disk = &def->disks[i];
+
+ if (disk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP ||
+ disk->bitmap)
+ continue;
+
+ if (VIR_STRDUP(disk->bitmap, def->parent.name) < 0)
+ return -1;
+ }
+
+ return 0;
+}
+
+
+static int
+virDomainCheckpointCompareDiskIndex(const void *a, const void *b)
+{
+ const virDomainCheckpointDiskDef *diska = a;
+ const virDomainCheckpointDiskDef *diskb = b;
+
+ /* Integer overflow shouldn't be a problem here. */
+ return diska->idx - diskb->idx;
+}
+
+/* Align def->disks to def->domain. Sort the list of def->disks,
+ * filling in any missing disks with appropriate default. Convert
+ * paths to disk targets for uniformity. Issue an error and return -1
+ * if any def->disks[n]->name appears more than once or does not map
+ * to dom->disks. */
+int
+virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def)
+{
+ int ret = -1;
+ virBitmapPtr map = NULL;
+ size_t i;
+ int ndisks;
+ int checkpoint_default = VIR_DOMAIN_CHECKPOINT_TYPE_NONE;
+
+ if (!def->parent.dom) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("missing domain in checkpoint"));
+ goto cleanup;
+ }
+
+ if (def->ndisks > def->parent.dom->ndisks) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("too many disk checkpoint requests for domain"));
+ goto cleanup;
+ }
+
+ /* Unlikely to have a guest without disks but technically possible. */
+ if (!def->parent.dom->ndisks) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("domain must have at least one disk to perform "
+ "checkpoints"));
+ goto cleanup;
+ }
+
+ /* If <disks> omitted, do bitmap on all disks; otherwise, do nothing
+ * for omitted disks */
+ if (!def->ndisks)
+ checkpoint_default = VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP;
+
+ if (!(map = virBitmapNew(def->parent.dom->ndisks)))
+ goto cleanup;
+
+ /* Double check requested disks. */
+ for (i = 0; i < def->ndisks; i++) {
+ virDomainCheckpointDiskDefPtr disk = &def->disks[i];
+ int idx = virDomainDiskIndexByName(def->parent.dom, disk->name, false);
+
+ if (idx < 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("no disk named '%s'"), disk->name);
+ goto cleanup;
+ }
+
+ if (virBitmapIsBitSet(map, idx)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("disk '%s' specified twice"),
+ disk->name);
+ goto cleanup;
+ }
+ ignore_value(virBitmapSetBit(map, idx));
+ disk->idx = idx;
+
+ if (STRNEQ(disk->name, def->parent.dom->disks[idx]->dst)) {
+ VIR_FREE(disk->name);
+ if (VIR_STRDUP(disk->name, def->parent.dom->disks[idx]->dst) < 0)
+ goto cleanup;
+ }
+ }
+
+ /* Provide defaults for all remaining disks. */
+ ndisks = def->ndisks;
+ if (VIR_EXPAND_N(def->disks, def->ndisks,
+ def->parent.dom->ndisks - def->ndisks) < 0)
+ goto cleanup;
+
+ for (i = 0; i < def->parent.dom->ndisks; i++) {
+ virDomainCheckpointDiskDefPtr disk;
+
+ if (virBitmapIsBitSet(map, i))
+ continue;
+ disk = &def->disks[ndisks++];
+ if (VIR_STRDUP(disk->name, def->parent.dom->disks[i]->dst) < 0)
+ goto cleanup;
+ disk->idx = i;
+
+ /* Don't checkpoint empty drives */
+ if (virStorageSourceIsEmpty(def->parent.dom->disks[i]->src))
+ disk->type = VIR_DOMAIN_CHECKPOINT_TYPE_NONE;
+ else
+ disk->type = checkpoint_default;
+ }
+
+ qsort(&def->disks[0], def->ndisks, sizeof(def->disks[0]),
+ virDomainCheckpointCompareDiskIndex);
+
+ /* Generate default bitmap names for checkpoint */
+ if (virDomainCheckpointDefAssignBitmapNames(def) < 0)
+ goto cleanup;
+
+ ret = 0;
+
+ cleanup:
+ virBitmapFree(map);
+ return ret;
+}
+
+
+/* Converts public VIR_DOMAIN_CHECKPOINT_XML_* into
+ * VIR_DOMAIN_CHECKPOINT_FORMAT_* flags, and silently ignores any other
+ * flags. */
+unsigned int virDomainCheckpointFormatConvertXMLFlags(unsigned int flags)
+{
+ unsigned int formatFlags = 0;
+
+ if (flags & VIR_DOMAIN_CHECKPOINT_XML_SECURE)
+ formatFlags |= VIR_DOMAIN_CHECKPOINT_FORMAT_SECURE;
+ if (flags & VIR_DOMAIN_CHECKPOINT_XML_NO_DOMAIN)
+ formatFlags |= VIR_DOMAIN_CHECKPOINT_FORMAT_NO_DOMAIN;
+ if (flags & VIR_DOMAIN_CHECKPOINT_XML_SIZE)
+ formatFlags |= VIR_DOMAIN_CHECKPOINT_FORMAT_SIZE;
+
+ return formatFlags;
+}
+
+
+static int
+virDomainCheckpointDiskDefFormat(virBufferPtr buf,
+ virDomainCheckpointDiskDefPtr disk,
+ unsigned int flags)
+{
+ if (!disk->name)
+ return 0;
+
+ virBufferEscapeString(buf, "<disk name='%s'", disk->name);
+ if (disk->type)
+ virBufferAsprintf(buf, " checkpoint='%s'",
+ virDomainCheckpointTypeToString(disk->type));
+ if (disk->bitmap) {
+ virBufferEscapeString(buf, " bitmap='%s'", disk->bitmap);
+ if (flags & VIR_DOMAIN_CHECKPOINT_FORMAT_SIZE)
+ virBufferAsprintf(buf, " size='%llu'", disk->size);
+ }
+ virBufferAddLit(buf, "/>\n");
+ return 0;
+}
+
+
+static int
+virDomainCheckpointDefFormatInternal(virBufferPtr buf,
+ virDomainCheckpointDefPtr def,
+ virCapsPtr caps,
+ virDomainXMLOptionPtr xmlopt,
+ unsigned int flags)
+{
+ size_t i;
+ unsigned int domainflags = VIR_DOMAIN_DEF_FORMAT_INACTIVE;
+
+ if (flags & VIR_DOMAIN_CHECKPOINT_FORMAT_SECURE)
+ domainflags |= VIR_DOMAIN_DEF_FORMAT_SECURE;
+
+ virBufferAddLit(buf, "<domaincheckpoint>\n");
+ virBufferAdjustIndent(buf, 2);
+
+ virBufferEscapeString(buf, "<name>%s</name>\n", def->parent.name);
+ virBufferEscapeString(buf, "<description>%s</description>\n",
+ def->parent.description);
+
+ if (def->parent.parent_name) {
+ virBufferAddLit(buf, "<parent>\n");
+ virBufferAdjustIndent(buf, 2);
+ virBufferEscapeString(buf, "<name>%s</name>\n",
+ def->parent.parent_name);
+ virBufferAdjustIndent(buf, -2);
+ virBufferAddLit(buf, "</parent>\n");
+ }
+
+ if (def->parent.creationTime)
+ virBufferAsprintf(buf, "<creationTime>%lld</creationTime>\n",
+ def->parent.creationTime);
+
+ virBufferAsprintf(buf, "<current>%d</current>\n",
+ !!(flags & VIR_DOMAIN_CHECKPOINT_FORMAT_CURRENT));
+
+ if (def->ndisks) {
+ virBufferAddLit(buf, "<disks>\n");
+ virBufferAdjustIndent(buf, 2);
+ for (i = 0; i < def->ndisks; i++) {
+ if (virDomainCheckpointDiskDefFormat(buf, &def->disks[i],
+ flags) < 0)
+ goto error;
+ }
+ virBufferAdjustIndent(buf, -2);
+ virBufferAddLit(buf, "</disks>\n");
+ }
+
+ if (!(flags & VIR_DOMAIN_CHECKPOINT_FORMAT_NO_DOMAIN) &&
+ virDomainDefFormatInternal(def->parent.dom, caps, domainflags, buf,
+ xmlopt) < 0)
+ goto error;
+
+ virBufferAdjustIndent(buf, -2);
+ virBufferAddLit(buf, "</domaincheckpoint>\n");
+
+ if (virBufferCheckError(buf) < 0)
+ goto error;
+
+ return 0;
+
+ error:
+ virBufferFreeAndReset(buf);
+ return -1;
+}
+
+char *
+virDomainCheckpointDefFormat(virDomainCheckpointDefPtr def,
+ virCapsPtr caps,
+ virDomainXMLOptionPtr xmlopt,
+ unsigned int flags)
+{
+ virBuffer buf = VIR_BUFFER_INITIALIZER;
+
+ virCheckFlags(VIR_DOMAIN_CHECKPOINT_FORMAT_SECURE |
+ VIR_DOMAIN_CHECKPOINT_FORMAT_NO_DOMAIN |
+ VIR_DOMAIN_CHECKPOINT_FORMAT_SIZE |
+ VIR_DOMAIN_CHECKPOINT_FORMAT_CURRENT, NULL);
+ if (virDomainCheckpointDefFormatInternal(&buf, def, caps, xmlopt,
+ flags) < 0)
+ return NULL;
+
+ return virBufferContentAndReset(&buf);
+}
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b8772d2895..a7e508f217 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -69,6 +69,14 @@ virCapabilitiesSetHostCPU;
virCapabilitiesSetNetPrefix;
+# conf/checkpoint_conf.h
+virDomainCheckpointDefFormat;
+virDomainCheckpointDefNew;
+virDomainCheckpointDefParseString;
+virDomainCheckpointTypeFromString;
+virDomainCheckpointTypeToString;
+
+
# conf/cpu_conf.h
virCPUCacheModeTypeFromString;
virCPUCacheModeTypeToString;
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 66c6ad911e..1fd698c2d2 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -277,7 +277,7 @@ endif WITH_LIBXL
if WITH_QEMU
test_programs += qemuxml2argvtest qemuxml2xmltest \
- qemudomainsnapshotxml2xmltest \
+ qemudomaincheckpointxml2xmltest qemudomainsnapshotxml2xmltest \
qemumonitorjsontest qemuhotplugtest \
qemuagenttest qemucapabilitiestest qemucaps2xmltest \
qemumemlocktest \
@@ -658,6 +658,11 @@ qemublocktest_LDADD = \
$(qemu_LDADDS) \
$(NULL)
+qemudomaincheckpointxml2xmltest_SOURCES = \
+ qemudomaincheckpointxml2xmltest.c testutilsqemu.c testutilsqemu.h \
+ testutils.c testutils.h
+qemudomaincheckpointxml2xmltest_LDADD = $(qemu_LDADDS)
+
qemudomainsnapshotxml2xmltest_SOURCES = \
qemudomainsnapshotxml2xmltest.c testutilsqemu.c testutilsqemu.h \
testutils.c testutils.h
@@ -693,7 +698,7 @@ qemufirmwaretest_LDADD = $(qemu_LDADDS)
else ! WITH_QEMU
EXTRA_DIST += qemuxml2argvtest.c qemuxml2xmltest.c \
- qemudomainsnapshotxml2xmltest.c \
+ qemudomaincheckpointxml2xmltest.c qemudomainsnapshotxml2xmltest.c \
testutilsqemu.c testutilsqemu.h \
testutilsqemuschema.c testutilsqemuschema.h \
qemumonitorjsontest.c qemuhotplugtest.c \
diff --git a/tests/qemudomaincheckpointxml2xmlout/internal-active-invalid.xml b/tests/qemudomaincheckpointxml2xmlout/internal-active-invalid.xml
new file mode 100644
index 0000000000..a518c58915
--- /dev/null
+++ b/tests/qemudomaincheckpointxml2xmlout/internal-active-invalid.xml
@@ -0,0 +1,53 @@
+<domaincheckpoint>
+ <name>1525889631</name>
+ <description>Completion of updates after OS install</description>
+ <parent>
+ <name>1525111885</name>
+ </parent>
+ <creationTime>1525889631</creationTime>
+ <disks>
+ <disk name='vda' checkpoint='bitmap' bitmap='1525889631'/>
+ <disk name='vdb' checkpoint='no'/>
+ </disks>
+ <domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219136</memory>
+ <currentMemory unit='KiB'>219136</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-i686</emulator>
+ <disk type='file' device='disk'>
+ <driver name='qemu' type='qcow2'/>
+ <source file='/tmp/data.img'/>
+ <target dev='vda' bus='virtio'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+ </disk>
+ <disk type='file' device='disk'>
+ <driver name='qemu' type='qcow2'/>
+ <source file='/tmp/logs.img'/>
+ <target dev='vdb' bus='virtio'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
+ </disk>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='none'/>
+ </devices>
+ </domain>
+ <active>1</active>
+</domaincheckpoint>
diff --git a/tests/qemudomaincheckpointxml2xmlout/internal-inactive-invalid.xml b/tests/qemudomaincheckpointxml2xmlout/internal-inactive-invalid.xml
new file mode 100644
index 0000000000..df14c97836
--- /dev/null
+++ b/tests/qemudomaincheckpointxml2xmlout/internal-inactive-invalid.xml
@@ -0,0 +1,53 @@
+<domaincheckpoint>
+ <name>1525889631</name>
+ <description>Completion of updates after OS install</description>
+ <parent>
+ <name>1525111885</name>
+ </parent>
+ <creationTime>1525889631</creationTime>
+ <disks>
+ <disk name='vda' checkpoint='bitmap' bitmap='1525889631'/>
+ <disk name='vdb' checkpoint='no'/>
+ </disks>
+ <domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219136</memory>
+ <currentMemory unit='KiB'>219136</currentMemory>
+ <vcpu placement='static'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-i686</emulator>
+ <disk type='file' device='disk'>
+ <driver name='qemu' type='qcow2'/>
+ <source file='/tmp/data.img'/>
+ <target dev='vda' bus='virtio'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
+ </disk>
+ <disk type='file' device='disk'>
+ <driver name='qemu' type='qcow2'/>
+ <source file='/tmp/logs.img'/>
+ <target dev='vdb' bus='virtio'/>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/>
+ </disk>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='none'/>
+ </devices>
+ </domain>
+ <active>0</active>
+</domaincheckpoint>
diff --git a/tests/qemudomaincheckpointxml2xmltest.c b/tests/qemudomaincheckpointxml2xmltest.c
new file mode 100644
index 0000000000..d509df37d4
--- /dev/null
+++ b/tests/qemudomaincheckpointxml2xmltest.c
@@ -0,0 +1,213 @@
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+
+#include <sys/types.h>
+#include <fcntl.h>
+
+#include "testutils.h"
+
+#ifdef WITH_QEMU
+
+# include "internal.h"
+# include "qemu/qemu_conf.h"
+# include "qemu/qemu_domain.h"
+# include "checkpoint_conf.h"
+# include "testutilsqemu.h"
+# include "virstring.h"
+
+# define VIR_FROM_THIS VIR_FROM_NONE
+
+static virQEMUDriver driver;
+
+enum {
+ TEST_REDEFINE = 1 << 0, /* Test use of REDEFINE parse flag */
+ TEST_PARENT = 1 << 1, /* hard-code parent after parse */
+ TEST_VDA_BITMAP = 1 << 2, /* hard-code disk vda after parse */
+ TEST_SIZE = 1 << 3, /* Test use of SIZE format flag */
+};
+
+static int
+testCompareXMLToXMLFiles(const char *inxml,
+ const char *outxml,
+ unsigned int flags)
+{
+ char *inXmlData = NULL;
+ char *outXmlData = NULL;
+ char *actual = NULL;
+ int ret = -1;
+ virDomainCheckpointDefPtr def = NULL;
+ unsigned int parseflags = 0;
+ unsigned int formatflags = VIR_DOMAIN_CHECKPOINT_FORMAT_SECURE;
+ bool cur = false;
+
+ if (flags & TEST_REDEFINE)
+ parseflags |= VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE;
+
+ if (virTestLoadFile(inxml, &inXmlData) < 0)
+ goto cleanup;
+
+ if (virTestLoadFile(outxml, &outXmlData) < 0)
+ goto cleanup;
+
+ if (!(def = virDomainCheckpointDefParseString(inXmlData, driver.caps,
+ driver.xmlopt, &cur,
+ parseflags)))
+ goto cleanup;
+ if (cur) {
+ if (!(flags & TEST_REDEFINE))
+ goto cleanup;
+ formatflags |= VIR_DOMAIN_CHECKPOINT_FORMAT_CURRENT;
+ }
+ if (flags & TEST_PARENT) {
+ if (def->parent.parent_name)
+ goto cleanup;
+ if (VIR_STRDUP(def->parent.parent_name, "1525111885") < 0)
+ goto cleanup;
+ }
+ if (flags & TEST_VDA_BITMAP) {
+ virDomainCheckpointDiskDefPtr disk;
+
+ if (VIR_EXPAND_N(def->disks, def->ndisks, 1) < 0)
+ goto cleanup;
+ disk = &def->disks[0];
+ if (disk->bitmap)
+ goto cleanup;
+ if (!disk->name) {
+ disk->type = VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP;
+ if (VIR_STRDUP(disk->name, "vda") < 0)
+ goto cleanup;
+ } else if (STRNEQ(disk->name, "vda")) {
+ goto cleanup;
+ }
+ if (VIR_STRDUP(disk->bitmap, def->parent.name) < 0)
+ goto cleanup;
+ }
+ if (flags & TEST_SIZE) {
+ def->disks[0].size = 1048576;
+ formatflags |= (VIR_DOMAIN_CHECKPOINT_FORMAT_SIZE |
+ VIR_DOMAIN_CHECKPOINT_FORMAT_CURRENT);
+ }
+
+ /* Parsing XML does not populate the domain definition; work
+ * around that by not requesting domain on output */
+ if (!def->parent.dom)
+ formatflags |= VIR_DOMAIN_CHECKPOINT_FORMAT_NO_DOMAIN;
+
+ if (!(actual = virDomainCheckpointDefFormat(def, driver.caps,
+ driver.xmlopt,
+ formatflags)))
+ goto cleanup;
+
+ if (STRNEQ(outXmlData, actual)) {
+ virTestDifferenceFull(stderr, outXmlData, outxml, actual, inxml);
+ goto cleanup;
+ }
+
+ ret = 0;
+
+ cleanup:
+ VIR_FREE(inXmlData);
+ VIR_FREE(outXmlData);
+ VIR_FREE(actual);
+ virObjectUnref(def);
+ return ret;
+}
+
+struct testInfo {
+ const char *inxml;
+ const char *outxml;
+ long long creationTime;
+ unsigned int flags;
+};
+static long long mocktime;
+
+static int
+testCheckpointPostParse(virDomainMomentDefPtr def)
+{
+ if (!mocktime)
+ return 0;
+ if (def->creationTime)
+ return -1;
+ def->creationTime = mocktime;
+ if (!def->name &&
+ virAsprintf(&def->name, "%lld", def->creationTime) < 0)
+ return -1;
+ return 0;
+}
+
+static int
+testCompareXMLToXMLHelper(const void *data)
+{
+ const struct testInfo *info = data;
+
+ mocktime = info->creationTime;
+ return testCompareXMLToXMLFiles(info->inxml, info->outxml, info->flags);
+}
+
+
+static int
+mymain(void)
+{
+ int ret = 0;
+
+ if (qemuTestDriverInit(&driver) < 0)
+ return EXIT_FAILURE;
+
+ virDomainXMLOptionSetMomentPostParse(driver.xmlopt,
+ testCheckpointPostParse);
+
+# define DO_TEST(prefix, name, inpath, outpath, time, flags) \
+ do { \
+ const struct testInfo info = {abs_srcdir "/" inpath "/" name ".xml", \
+ abs_srcdir "/" outpath "/" name ".xml", \
+ time, flags}; \
+ if (virTestRun("CHECKPOINT XML-2-XML " prefix " " name, \
+ testCompareXMLToXMLHelper, &info) < 0) \
+ ret = -1; \
+ } while (0)
+
+# define DO_TEST_INOUT(name, time, flags) \
+ DO_TEST("in->out", name, \
+ "qemudomaincheckpointxml2xmlin", \
+ "qemudomaincheckpointxml2xmlout", \
+ time, flags)
+# define DO_TEST_OUT(name, flags) \
+ DO_TEST("out->out", name, \
+ "qemudomaincheckpointxml2xmlout", \
+ "qemudomaincheckpointxml2xmlout", \
+ 0, flags | TEST_REDEFINE)
+
+ /* Unset or set all envvars here that are copied in qemudBuildCommandLine
+ * using ADD_ENV_COPY, otherwise these tests may fail due to unexpected
+ * values for these envvars */
+ setenv("PATH", "/bin", 1);
+
+ /* Test a normal user redefine */
+ DO_TEST_OUT("redefine", 0);
+
+ /* Tests of valid user input, and resulting output */
+ DO_TEST_INOUT("empty", 1525889631, TEST_VDA_BITMAP);
+ DO_TEST_INOUT("sample", 1525889631, TEST_PARENT | TEST_VDA_BITMAP);
+ DO_TEST_INOUT("size", 1553648510,
+ TEST_PARENT | TEST_VDA_BITMAP | TEST_SIZE);
+
+ qemuTestDriverFree(&driver);
+
+ return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
+}
+
+VIR_TEST_MAIN(mymain)
+
+#else
+
+int
+main(void)
+{
+ return EXIT_AM_SKIP;
+}
+
+#endif /* WITH_QEMU */
--
2.20.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Sat, Jul 06, 2019 at 22:56:04 -0500, Eric Blake wrote:
> Add a new file checkpoint_conf.c that performs the translation to and
> from new XML describing a checkpoint. The code shares a common base
> class with snapshots, since a checkpoint similarly represents the
> domain state at a moment in time. Add some basic testing of round trip
> XML handling through the new code.
>
> Of note - this code intentionally differs from snapshots in that XML
> schema validation is unconditional, rather than based on a public API
> flag. Also, the redefine flag requires the <domain> sub-element to be
> present, rather than catering to historical back-compat to older
> versions.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
[...]
> diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c
> new file mode 100644
> index 0000000000..d50d63ac0e
> --- /dev/null
> +++ b/src/conf/checkpoint_conf.c
> @@ -0,0 +1,576 @@
[...]
> +static int
> +virDomainCheckpointDiskDefParseXML(xmlNodePtr node,
> + xmlXPathContextPtr ctxt,
> + virDomainCheckpointDiskDefPtr def)
> +{
> + int ret = -1;
> + char *checkpoint = NULL;
> + char *bitmap = NULL;
> + xmlNodePtr saved = ctxt->node;
> +
> + ctxt->node = node;
> +
> + def->name = virXMLPropString(node, "name");
> + if (!def->name) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing name from disk checkpoint element"));
'name' is mandatory in the schema for <disk>
> + goto cleanup;
> + }
> +
> + checkpoint = virXMLPropString(node, "checkpoint");
This attribute seems to be mandatory in the schema [1]
> + if (checkpoint) {
> + def->type = virDomainCheckpointTypeFromString(checkpoint);
> + if (def->type <= 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unknown disk checkpoint setting '%s'"),
> + checkpoint);
These are also mandated.
> + goto cleanup;
> + }
> + } else {
> + def->type = VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP;
[1] so this should never happen
> + }
> +
> + bitmap = virXMLPropString(node, "bitmap");
> + if (bitmap) {
> + if (def->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("disk checkpoint bitmap '%s' requires "
> + "type='bitmap'"),
> + bitmap);
And this is also mandated by the schema.
> + goto cleanup;
> + }
> + VIR_STEAL_PTR(def->bitmap, bitmap);
> + }
> +
> + ret = 0;
> + cleanup:
> + ctxt->node = saved;
> +
> + VIR_FREE(checkpoint);
> + VIR_FREE(bitmap);
> + if (ret < 0)
> + virDomainCheckpointDiskDefClear(def);
> + return ret;
> +}
> +
> +/* flags is bitwise-or of virDomainCheckpointParseFlags. If flags
> + * does not include VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE, then caps
> + * and current are ignored.
> + */
> +static virDomainCheckpointDefPtr
> +virDomainCheckpointDefParse(xmlXPathContextPtr ctxt,
> + virCapsPtr caps,
> + virDomainXMLOptionPtr xmlopt,
> + bool *current,
> + unsigned int flags)
> +{
> + virDomainCheckpointDefPtr def = NULL;
> + virDomainCheckpointDefPtr ret = NULL;
> + xmlNodePtr *nodes = NULL;
> + size_t i;
> + int n;
> + char *creation = NULL;
> + int active;
> + char *tmp;
> +
> + if (!(def = virDomainCheckpointDefNew()))
> + return NULL;
> +
> + def->parent.name = virXPathString("string(./name)", ctxt);
> + if (def->parent.name == NULL) {
> + if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("a redefined checkpoint must have a name"));
> + goto cleanup;
> + }
> + }
> +
> + def->parent.description = virXPathString("string(./description)", ctxt);
> +
> + if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE) {
> + if (virXPathLongLong("string(./creationTime)", ctxt,
> + &def->parent.creationTime) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing creationTime from existing checkpoint"));
> + goto cleanup;
In v8 I've pointed out that we should not intermix validation of the XML
with the parsing.
> + }
> +
> + def->parent.parent_name = virXPathString("string(./parent/name)", ctxt);
> +
> + if (virXPathInt("string(./current)", ctxt, &active) < 0 || !current) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing current from existing checkpoint"));
> + goto cleanup;
> + }
> + *current = !!active;
> +
> + if ((tmp = virXPathString("string(./domain/@type)", ctxt))) {
> + int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
> + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
> + xmlNodePtr domainNode = virXPathNode("./domain", ctxt);
> +
> + VIR_FREE(tmp);
> + if (!domainNode) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing domain in checkpoint"));
> + goto cleanup;
> + }
> + def->parent.dom = virDomainDefParseNode(ctxt->node->doc, domainNode,
> + caps, xmlopt, NULL,
> + domainflags);
> + if (!def->parent.dom)
> + goto cleanup;
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing domain in checkpoint redefine"));
> + goto cleanup;
> + }
> + } else if (virDomainXMLOptionRunMomentPostParse(xmlopt, &def->parent) < 0) {
> + goto cleanup;
> + }
> +
> + if ((n = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0)
> + goto cleanup;
> + if (n && VIR_ALLOC_N(def->disks, n) < 0)
> + goto cleanup;
> + def->ndisks = n;
> + for (i = 0; i < def->ndisks; i++) {
> + if (virDomainCheckpointDiskDefParseXML(nodes[i], ctxt,
> + &def->disks[i]) < 0)
> + goto cleanup;
> + }
> +
> + VIR_STEAL_PTR(ret, def);
> +
> + cleanup:
> + VIR_FREE(creation);
> + VIR_FREE(nodes);
> + virObjectUnref(def);
> +
> + return ret;
> +}
> +
> +static virDomainCheckpointDefPtr
> +virDomainCheckpointDefParseNode(xmlDocPtr xml,
> + xmlNodePtr root,
> + virCapsPtr caps,
> + virDomainXMLOptionPtr xmlopt,
> + bool *current,
> + unsigned int flags)
> +{
> + xmlXPathContextPtr ctxt = NULL;
> + virDomainCheckpointDefPtr def = NULL;
> + VIR_AUTOFREE(char *) schema = NULL;
> +
> + if (!virXMLNodeNameEqual(root, "domaincheckpoint")) {
> + virReportError(VIR_ERR_XML_ERROR, "%s", _("domaincheckpoint"));
> + goto cleanup;
The schema will be able to validate this.
> + }
> +
> + /* This is a new enough API to make schema validation unconditional */
> + schema = virFileFindResource("domaincheckpoint.rng",
> + abs_top_srcdir "/docs/schemas",
> + PKGDATADIR "/schemas");
> + if (!schema)
> + return NULL;
> + if (virXMLValidateAgainstSchema(schema, xml) < 0)
> + return NULL;
> +
> + ctxt = xmlXPathNewContext(xml);
> + if (ctxt == NULL) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + ctxt->node = root;
> + def = virDomainCheckpointDefParse(ctxt, caps, xmlopt, current, flags);
> + cleanup:
> + xmlXPathFreeContext(ctxt);
> + return def;
> +}
[...]
> +int
> +virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def)
> +{
> + int ret = -1;
> + virBitmapPtr map = NULL;
> + size_t i;
> + int ndisks;
> + int checkpoint_default = VIR_DOMAIN_CHECKPOINT_TYPE_NONE;
> +
> + if (!def->parent.dom) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing domain in checkpoint"));
> + goto cleanup;
Is this possible?
> + }
> +
> + if (def->ndisks > def->parent.dom->ndisks) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("too many disk checkpoint requests for domain"));
> + goto cleanup;
> + }
> +
> + /* Unlikely to have a guest without disks but technically possible. */
> + if (!def->parent.dom->ndisks) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("domain must have at least one disk to perform "
> + "checkpoints"));
> + goto cleanup;
> + }
> +
> + /* If <disks> omitted, do bitmap on all disks; otherwise, do nothing
> + * for omitted disks */
> + if (!def->ndisks)
> + checkpoint_default = VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP;
> +
> + if (!(map = virBitmapNew(def->parent.dom->ndisks)))
> + goto cleanup;
> +
> + /* Double check requested disks. */
> + for (i = 0; i < def->ndisks; i++) {
> + virDomainCheckpointDiskDefPtr disk = &def->disks[i];
> + int idx = virDomainDiskIndexByName(def->parent.dom, disk->name, false);
> +
> + if (idx < 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("no disk named '%s'"), disk->name);
> + goto cleanup;
> + }
> +
> + if (virBitmapIsBitSet(map, idx)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("disk '%s' specified twice"),
> + disk->name);
> + goto cleanup;
> + }
> + ignore_value(virBitmapSetBit(map, idx));
> + disk->idx = idx;
> +
> + if (STRNEQ(disk->name, def->parent.dom->disks[idx]->dst)) {
> + VIR_FREE(disk->name);
> + if (VIR_STRDUP(disk->name, def->parent.dom->disks[idx]->dst) < 0)
> + goto cleanup;
> + }
> + }
> +
> + /* Provide defaults for all remaining disks. */
> + ndisks = def->ndisks;
> + if (VIR_EXPAND_N(def->disks, def->ndisks,
> + def->parent.dom->ndisks - def->ndisks) < 0)
> + goto cleanup;
> +
> + for (i = 0; i < def->parent.dom->ndisks; i++) {
> + virDomainCheckpointDiskDefPtr disk;
> +
> + if (virBitmapIsBitSet(map, i))
> + continue;
> + disk = &def->disks[ndisks++];
> + if (VIR_STRDUP(disk->name, def->parent.dom->disks[i]->dst) < 0)
> + goto cleanup;
> + disk->idx = i;
> +
> + /* Don't checkpoint empty drives */
Without -blockdev you'll also have a problem also with readonly disks as
qemu does not open the actual file for writing and thus writing the
bitmap won't work.
Also ... it doesn't make much sense ... since the bitmap will be empty
forever.
> + if (virStorageSourceIsEmpty(def->parent.dom->disks[i]->src))
> + disk->type = VIR_DOMAIN_CHECKPOINT_TYPE_NONE;
> + else
> + disk->type = checkpoint_default;
> + }
> +
> + qsort(&def->disks[0], def->ndisks, sizeof(def->disks[0]),
> + virDomainCheckpointCompareDiskIndex);
> +
> + /* Generate default bitmap names for checkpoint */
> + if (virDomainCheckpointDefAssignBitmapNames(def) < 0)
> + goto cleanup;
> +
> + ret = 0;
> +
> + cleanup:
> + virBitmapFree(map);
> + return ret;
> +}
[...]
> +static int
> +virDomainCheckpointDiskDefFormat(virBufferPtr buf,
> + virDomainCheckpointDiskDefPtr disk,
> + unsigned int flags)
> +{
> + if (!disk->name)
> + return 0;
> +
> + virBufferEscapeString(buf, "<disk name='%s'", disk->name);
> + if (disk->type)
> + virBufferAsprintf(buf, " checkpoint='%s'",
> + virDomainCheckpointTypeToString(disk->type));
> + if (disk->bitmap) {
> + virBufferEscapeString(buf, " bitmap='%s'", disk->bitmap);
> + if (flags & VIR_DOMAIN_CHECKPOINT_FORMAT_SIZE)
> + virBufferAsprintf(buf, " size='%llu'", disk->size);
The schema does not tie the size attribute to the presence of the
bitmap name.
> + }
> + virBufferAddLit(buf, "/>\n");
> + return 0;
> +}
[...]
ACK in general, most of the comments are not problems, if they'll be
addressed later. Note that since this patch introduces a big pile of
old-style code and technical debt it's more than desired that you
address it at some time.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 7/8/19 7:06 AM, Peter Krempa wrote:
> On Sat, Jul 06, 2019 at 22:56:04 -0500, Eric Blake wrote:
>> Add a new file checkpoint_conf.c that performs the translation to and
>> from new XML describing a checkpoint. The code shares a common base
>> class with snapshots, since a checkpoint similarly represents the
>> domain state at a moment in time. Add some basic testing of round trip
>> XML handling through the new code.
>>
>> Of note - this code intentionally differs from snapshots in that XML
>> schema validation is unconditional, rather than based on a public API
>> flag. Also, the redefine flag requires the <domain> sub-element to be
>> present, rather than catering to historical back-compat to older
>> versions.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>
>> +static int
>> +virDomainCheckpointDiskDefParseXML(xmlNodePtr node,
>> + xmlXPathContextPtr ctxt,
>> + virDomainCheckpointDiskDefPtr def)
>> +{
>> + int ret = -1;
>> + char *checkpoint = NULL;
>> + char *bitmap = NULL;
>> + xmlNodePtr saved = ctxt->node;
>> +
>> + ctxt->node = node;
>> +
>> + def->name = virXMLPropString(node, "name");
>> + if (!def->name) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("missing name from disk checkpoint element"));
>
> 'name' is mandatory in the schema for <disk>
True, and I've fixed things to mandate schema compliance, so I'll drop
the unreachable error.
>
>> + goto cleanup;
>> + }
>> +
>> + checkpoint = virXMLPropString(node, "checkpoint");
>
> This attribute seems to be mandatory in the schema [1]
Not quite. It is mandatory if you want checkpoint='no', but optional if
you want to default to checkpoint='bitmap'.
>
>> + if (checkpoint) {
>> + def->type = virDomainCheckpointTypeFromString(checkpoint);
>> + if (def->type <= 0) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("unknown disk checkpoint setting '%s'"),
>> + checkpoint);
>
> These are also mandated.
Dropping another unreachable error.
>
>> + goto cleanup;
>> + }
>> + } else {
>> + def->type = VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP;
>
> [1] so this should never happen
The schema permits it to be absent; I can enhance the
qemudomaincheckpointxml2xml test to cover that.
>
>> + }
>> +
>> + bitmap = virXMLPropString(node, "bitmap");
>> + if (bitmap) {
>> + if (def->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) {
>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("disk checkpoint bitmap '%s' requires "
>> + "type='bitmap'"),
>> + bitmap);
>
> And this is also mandated by the schema.
Correct, dropping another dead block.
>
>> + goto cleanup;
>> + }
>> + VIR_STEAL_PTR(def->bitmap, bitmap);
>> + }
>> +
>> + ret = 0;
>> + cleanup:
>> + ctxt->node = saved;
>> +
>> + VIR_FREE(checkpoint);
>> + VIR_FREE(bitmap);
>> + if (ret < 0)
>> + virDomainCheckpointDiskDefClear(def);
And with a couple of VIR_AUTOFREE and VIR_XPATH_NODE_AUTORESTORE, this
entire cleanup label can disappear.
>> + return ret;
>> +}
>> +
>> +/* flags is bitwise-or of virDomainCheckpointParseFlags. If flags
>> + * does not include VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE, then caps
>> + * and current are ignored.
>> + */
>> +static virDomainCheckpointDefPtr
>> +virDomainCheckpointDefParse(xmlXPathContextPtr ctxt,
>> + virCapsPtr caps,
>> + virDomainXMLOptionPtr xmlopt,
>> + bool *current,
>> + unsigned int flags)
>> +{
>> + virDomainCheckpointDefPtr def = NULL;
>> + virDomainCheckpointDefPtr ret = NULL;
>> + xmlNodePtr *nodes = NULL;
>> + size_t i;
>> + int n;
>> + char *creation = NULL;
>> + int active;
>> + char *tmp;
>> +
>> + if (!(def = virDomainCheckpointDefNew()))
>> + return NULL;
>> +
>> + def->parent.name = virXPathString("string(./name)", ctxt);
>> + if (def->parent.name == NULL) {
>> + if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE) {
>> + virReportError(VIR_ERR_XML_ERROR, "%s",
>> + _("a redefined checkpoint must have a name"));
>> + goto cleanup;
>> + }
>> + }
>> +
>> + def->parent.description = virXPathString("string(./description)", ctxt);
>> +
>> + if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE) {
>> + if (virXPathLongLong("string(./creationTime)", ctxt,
>> + &def->parent.creationTime) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("missing creationTime from existing checkpoint"));
>> + goto cleanup;
>
> In v8 I've pointed out that we should not intermix validation of the XML
> with the parsing.
If I'm understanding, you want me to populate the entire struct with as
much (or as little) information as possible in the first half of the
function, then in the second half, if PARSE_REDEFINE was set, check that
the information that is optional for normal defines is present for a
redefine? The RNG schema doesn't make that easy to has something
optional by default but mandatory when a flag was passed, so there still
has to be a manual check; for timestamp it might be easy to validate
that a non-zero timestamp was set by the parse code, but it may be
harder for other elements where you are complaining about intermixed
validation. In the meantime, this copies the flow used by snapshots, so
I'm hesitant to rearrange it too much.
>> +static virDomainCheckpointDefPtr
>> +virDomainCheckpointDefParseNode(xmlDocPtr xml,
>> + xmlNodePtr root,
>> + virCapsPtr caps,
>> + virDomainXMLOptionPtr xmlopt,
>> + bool *current,
>> + unsigned int flags)
>> +{
>> + xmlXPathContextPtr ctxt = NULL;
>> + virDomainCheckpointDefPtr def = NULL;
>> + VIR_AUTOFREE(char *) schema = NULL;
>> +
>> + if (!virXMLNodeNameEqual(root, "domaincheckpoint")) {
>> + virReportError(VIR_ERR_XML_ERROR, "%s", _("domaincheckpoint"));
>> + goto cleanup;
>
> The schema will be able to validate this.
Concur, another dead check dropped.
>> +int
>> +virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def)
>> +{
>> + int ret = -1;
>> + virBitmapPtr map = NULL;
>> + size_t i;
>> + int ndisks;
>> + int checkpoint_default = VIR_DOMAIN_CHECKPOINT_TYPE_NONE;
>> +
>> + if (!def->parent.dom) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("missing domain in checkpoint"));
>> + goto cleanup;
>
> Is this possible?
The schema doesn't guarantee it, but it is a coding error if the driver
didn't set the domain. But since we don't use assert(), I don't have
any better way to express that fact.
>> + for (i = 0; i < def->parent.dom->ndisks; i++) {
>> + virDomainCheckpointDiskDefPtr disk;
>> +
>> + if (virBitmapIsBitSet(map, i))
>> + continue;
>> + disk = &def->disks[ndisks++];
>> + if (VIR_STRDUP(disk->name, def->parent.dom->disks[i]->dst) < 0)
>> + goto cleanup;
>> + disk->idx = i;
>> +
>> + /* Don't checkpoint empty drives */
>
> Without -blockdev you'll also have a problem also with readonly disks as
> qemu does not open the actual file for writing and thus writing the
> bitmap won't work.
>
> Also ... it doesn't make much sense ... since the bitmap will be empty
> forever.
True; I'll tweak things so that readonly disks do not default to having
a checkpoint added.
>> +static int
>> +virDomainCheckpointDiskDefFormat(virBufferPtr buf,
>> + virDomainCheckpointDiskDefPtr disk,
>> + unsigned int flags)
>> +{
>> + if (!disk->name)
>> + return 0;
>> +
>> + virBufferEscapeString(buf, "<disk name='%s'", disk->name);
>> + if (disk->type)
>> + virBufferAsprintf(buf, " checkpoint='%s'",
>> + virDomainCheckpointTypeToString(disk->type));
>> + if (disk->bitmap) {
>> + virBufferEscapeString(buf, " bitmap='%s'", disk->bitmap);
>> + if (flags & VIR_DOMAIN_CHECKPOINT_FORMAT_SIZE)
>> + virBufferAsprintf(buf, " size='%llu'", disk->size);
>
> The schema does not tie the size attribute to the presence of the
> bitmap name.
I can try to tweak that.
>
>> + }
>> + virBufferAddLit(buf, "/>\n");
>> + return 0;
>> +}
>
>
> [...]
>
> ACK in general, most of the comments are not problems, if they'll be
> addressed later. Note that since this patch introduces a big pile of
> old-style code and technical debt it's more than desired that you
> address it at some time.
I just re-compared this against snapshot_conf.c, and didn't see obvious
differences where snapshots had moved on to more modern code. Yes,
there's probably things that can be modernized (and my v10 will have a
few more VIR_AUTOFREE), but I don't want to delay this any longer than
necessary. Thanks for the close reviews so far.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.