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 | 84 +++
src/conf/virconftypes.h | 3 +
po/POTFILES | 1 +
src/conf/Makefile.inc.am | 2 +
src/conf/checkpoint_conf.c | 535 ++++++++++++++++++
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, 959 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..0fac521efe
--- /dev/null
+++ b/src/conf/checkpoint_conf.h
@@ -0,0 +1,84 @@
+/*
+ * 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,
+} virDomainCheckpointFormatFlags;
+
+unsigned int virDomainCheckpointFormatConvertXMLFlags(unsigned int flags);
+
+virDomainCheckpointDefPtr virDomainCheckpointDefParseString(const char *xmlStr,
+ virCapsPtr caps,
+ virDomainXMLOptionPtr xmlopt,
+ 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 5d0bf43f02..2c27049d55 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 16a21c34f3..f656725e3a 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..69c9040a43
--- /dev/null
+++ b/src/conf/checkpoint_conf.c
@@ -0,0 +1,535 @@
+/*
+ * 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)
+{
+ VIR_AUTOFREE(char *) checkpoint = NULL;
+ VIR_XPATH_NODE_AUTORESTORE(ctxt);
+
+ ctxt->node = node;
+
+ /* Schema guarantees this is non-NULL: */
+ def->name = virXMLPropString(node, "name");
+
+ checkpoint = virXMLPropString(node, "checkpoint");
+ if (checkpoint)
+ /* Schema guarantees this is in range: */
+ def->type = virDomainCheckpointTypeFromString(checkpoint);
+ else
+ def->type = VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP;
+
+ def->bitmap = virXMLPropString(node, "bitmap");
+
+ return 0;
+}
+
+/* flags is bitwise-or of virDomainCheckpointParseFlags. If flags
+ * does not include VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE, then caps
+ * is ignored.
+ */
+static virDomainCheckpointDefPtr
+virDomainCheckpointDefParse(xmlXPathContextPtr ctxt,
+ virCapsPtr caps,
+ virDomainXMLOptionPtr xmlopt,
+ unsigned int flags)
+{
+ virDomainCheckpointDefPtr ret = NULL;
+ size_t i;
+ int n;
+ char *tmp;
+ VIR_AUTOFREE(xmlNodePtr *) nodes = NULL;
+ VIR_AUTOFREE(char *)creation = NULL;
+ VIR_AUTOUNREF(virDomainCheckpointDefPtr) def = NULL;
+
+ 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"));
+ return NULL;
+ }
+ }
+
+ 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"));
+ return NULL;
+ }
+
+ def->parent.parent_name = virXPathString("string(./parent/name)", ctxt);
+
+ 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"));
+ return NULL;
+ }
+ def->parent.dom = virDomainDefParseNode(ctxt->node->doc, domainNode,
+ caps, xmlopt, NULL,
+ domainflags);
+ if (!def->parent.dom)
+ return NULL;
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("missing domain in checkpoint redefine"));
+ return NULL;
+ }
+ } else if (virDomainXMLOptionRunMomentPostParse(xmlopt, &def->parent) < 0) {
+ return NULL;
+ }
+
+ if ((n = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0)
+ return NULL;
+ if (n && VIR_ALLOC_N(def->disks, n) < 0)
+ return NULL;
+ def->ndisks = n;
+ for (i = 0; i < def->ndisks; i++) {
+ if (virDomainCheckpointDiskDefParseXML(nodes[i], ctxt,
+ &def->disks[i]) < 0)
+ return NULL;
+ }
+
+ VIR_STEAL_PTR(ret, def);
+ return ret;
+}
+
+static virDomainCheckpointDefPtr
+virDomainCheckpointDefParseNode(xmlDocPtr xml,
+ xmlNodePtr root,
+ virCapsPtr caps,
+ virDomainXMLOptionPtr xmlopt,
+ 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, flags);
+ cleanup:
+ xmlXPathFreeContext(ctxt);
+ return def;
+}
+
+virDomainCheckpointDefPtr
+virDomainCheckpointDefParseString(const char *xmlStr,
+ virCapsPtr caps,
+ virDomainXMLOptionPtr xmlopt,
+ 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, 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 writeable 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;
+ }
+ if ((virStorageSourceIsEmpty(def->parent.dom->disks[idx]->src) ||
+ def->parent.dom->disks[idx]->src->readonly) &&
+ disk->type != VIR_DOMAIN_CHECKPOINT_TYPE_NONE) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("disk '%s' is empty or readonly"),
+ 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 or readonly drives */
+ if (virStorageSourceIsEmpty(def->parent.dom->disks[i]->src) ||
+ def->parent.dom->disks[i]->src->readonly)
+ 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);
+
+ 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, 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 ff5a77b0e2..693c75b45d 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 9e2e57459e..544328bb18 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -276,7 +276,7 @@ endif WITH_LIBXL
if WITH_QEMU
test_programs += qemuxml2argvtest qemuxml2xmltest \
- qemudomainsnapshotxml2xmltest \
+ qemudomaincheckpointxml2xmltest qemudomainsnapshotxml2xmltest \
qemumonitorjsontest qemuhotplugtest \
qemuagenttest qemucapabilitiestest qemucaps2xmltest \
qemumemlocktest \
@@ -647,6 +647,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
@@ -682,7 +687,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..8a7c0922c7
--- /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 */
+ TEST_INVALID = 1 << 4, /* Test that input fails parse */
+};
+
+static int
+testCompareXMLToXMLFiles(const char *inxml,
+ const char *outxml,
+ unsigned int flags)
+{
+ unsigned int parseflags = 0;
+ unsigned int formatflags = VIR_DOMAIN_CHECKPOINT_FORMAT_SECURE;
+ VIR_AUTOFREE(char *) inXmlData = NULL;
+ VIR_AUTOFREE(char *) outXmlData = NULL;
+ VIR_AUTOFREE(char *) actual = NULL;
+ VIR_AUTOUNREF(virDomainCheckpointDefPtr) def = NULL;
+
+ if (flags & TEST_REDEFINE)
+ parseflags |= VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE;
+
+ if (virTestLoadFile(inxml, &inXmlData) < 0)
+ return -1;
+
+ if (!(flags & TEST_INVALID) &&
+ virTestLoadFile(outxml, &outXmlData) < 0)
+ return -1;
+
+ if (!(def = virDomainCheckpointDefParseString(inXmlData, driver.caps,
+ driver.xmlopt,
+ parseflags))) {
+ if (flags & TEST_INVALID)
+ return 0;
+ return -1;
+ }
+ if (flags & TEST_PARENT) {
+ if (def->parent.parent_name)
+ return -1;
+ if (VIR_STRDUP(def->parent.parent_name, "1525111885") < 0)
+ return -1;
+ }
+ if (flags & TEST_VDA_BITMAP) {
+ virDomainCheckpointDiskDefPtr disk;
+
+ if (VIR_EXPAND_N(def->disks, def->ndisks, 1) < 0)
+ return -1;
+ disk = &def->disks[0];
+ if (disk->bitmap)
+ return -1;
+ if (!disk->name) {
+ disk->type = VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP;
+ if (VIR_STRDUP(disk->name, "vda") < 0)
+ return -1;
+ } else if (STRNEQ(disk->name, "vda")) {
+ return -1;
+ }
+ if (VIR_STRDUP(disk->bitmap, def->parent.name) < 0)
+ return -1;
+ }
+ if (flags & TEST_SIZE) {
+ def->disks[0].size = 1048576;
+ formatflags |= VIR_DOMAIN_CHECKPOINT_FORMAT_SIZE;
+ }
+
+ /* 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)))
+ return -1;
+
+ if (STRNEQ(outXmlData, actual)) {
+ virTestDifferenceFull(stderr, outXmlData, outxml, actual, inxml);
+ return -1;
+ }
+
+ return 0;
+}
+
+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)
+# define DO_TEST_INVALID(name) \
+ DO_TEST("in->out", name, \
+ "qemudomaincheckpointxml2xmlin", \
+ "qemudomaincheckpointxml2xmlout", \
+ 0, TEST_INVALID)
+
+ /* 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("disk-default", 1525889631, TEST_PARENT | 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);
+
+ /* Tests of invalid user input */
+ DO_TEST_INVALID("disk-invalid");
+ DO_TEST_INVALID("name-invalid");
+
+ 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 Wed, Jul 24, 2019 at 12:55:58AM -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.
I'm in two minds as to whether this is really a good idea.
I always have a strong tendancy to prefer consistency across the APIs
we expose, which would point to an opt-in flag for validation. If we
did later find we need a way to skip validation, then we end up having
to add a NO_VALIDATION API flag, which makes inconsitency even more
glaring to app devs
I understand that this can help catch mistakes in apps, but I'm not
sold on it as a default, when it is easy for apps to opt-in if they
want to.
> 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 | 84 +++
> src/conf/virconftypes.h | 3 +
> po/POTFILES | 1 +
> src/conf/Makefile.inc.am | 2 +
> src/conf/checkpoint_conf.c | 535 ++++++++++++++++++
> 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, 959 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..0fac521efe
> --- /dev/null
> +++ b/src/conf/checkpoint_conf.h
> @@ -0,0 +1,84 @@
> +/*
> + * 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,
> +} virDomainCheckpointFormatFlags;
> +
> +unsigned int virDomainCheckpointFormatConvertXMLFlags(unsigned int flags);
> +
> +virDomainCheckpointDefPtr virDomainCheckpointDefParseString(const char *xmlStr,
> + virCapsPtr caps,
> + virDomainXMLOptionPtr xmlopt,
> + 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 5d0bf43f02..2c27049d55 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 16a21c34f3..f656725e3a 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..69c9040a43
> --- /dev/null
> +++ b/src/conf/checkpoint_conf.c
> @@ -0,0 +1,535 @@
> +/*
> + * 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)
> +{
> + VIR_AUTOFREE(char *) checkpoint = NULL;
> + VIR_XPATH_NODE_AUTORESTORE(ctxt);
> +
> + ctxt->node = node;
> +
> + /* Schema guarantees this is non-NULL: */
> + def->name = virXMLPropString(node, "name");
> +
> + checkpoint = virXMLPropString(node, "checkpoint");
> + if (checkpoint)
> + /* Schema guarantees this is in range: */
> + def->type = virDomainCheckpointTypeFromString(checkpoint);
> + else
> + def->type = VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP;
> +
> + def->bitmap = virXMLPropString(node, "bitmap");
> +
> + return 0;
> +}
> +
> +/* flags is bitwise-or of virDomainCheckpointParseFlags. If flags
> + * does not include VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE, then caps
> + * is ignored.
> + */
> +static virDomainCheckpointDefPtr
> +virDomainCheckpointDefParse(xmlXPathContextPtr ctxt,
> + virCapsPtr caps,
> + virDomainXMLOptionPtr xmlopt,
> + unsigned int flags)
> +{
> + virDomainCheckpointDefPtr ret = NULL;
> + size_t i;
> + int n;
> + char *tmp;
> + VIR_AUTOFREE(xmlNodePtr *) nodes = NULL;
> + VIR_AUTOFREE(char *)creation = NULL;
> + VIR_AUTOUNREF(virDomainCheckpointDefPtr) def = NULL;
> +
> + 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"));
> + return NULL;
> + }
> + }
> +
> + 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"));
> + return NULL;
> + }
> +
> + def->parent.parent_name = virXPathString("string(./parent/name)", ctxt);
> +
> + 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"));
> + return NULL;
> + }
> + def->parent.dom = virDomainDefParseNode(ctxt->node->doc, domainNode,
> + caps, xmlopt, NULL,
> + domainflags);
> + if (!def->parent.dom)
> + return NULL;
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing domain in checkpoint redefine"));
> + return NULL;
> + }
> + } else if (virDomainXMLOptionRunMomentPostParse(xmlopt, &def->parent) < 0) {
> + return NULL;
> + }
> +
> + if ((n = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0)
> + return NULL;
> + if (n && VIR_ALLOC_N(def->disks, n) < 0)
> + return NULL;
> + def->ndisks = n;
> + for (i = 0; i < def->ndisks; i++) {
> + if (virDomainCheckpointDiskDefParseXML(nodes[i], ctxt,
> + &def->disks[i]) < 0)
> + return NULL;
> + }
> +
> + VIR_STEAL_PTR(ret, def);
> + return ret;
> +}
> +
> +static virDomainCheckpointDefPtr
> +virDomainCheckpointDefParseNode(xmlDocPtr xml,
> + xmlNodePtr root,
> + virCapsPtr caps,
> + virDomainXMLOptionPtr xmlopt,
> + 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, flags);
> + cleanup:
> + xmlXPathFreeContext(ctxt);
> + return def;
> +}
> +
> +virDomainCheckpointDefPtr
> +virDomainCheckpointDefParseString(const char *xmlStr,
> + virCapsPtr caps,
> + virDomainXMLOptionPtr xmlopt,
> + 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, 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 writeable 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;
> + }
> + if ((virStorageSourceIsEmpty(def->parent.dom->disks[idx]->src) ||
> + def->parent.dom->disks[idx]->src->readonly) &&
> + disk->type != VIR_DOMAIN_CHECKPOINT_TYPE_NONE) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("disk '%s' is empty or readonly"),
> + 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 or readonly drives */
> + if (virStorageSourceIsEmpty(def->parent.dom->disks[i]->src) ||
> + def->parent.dom->disks[i]->src->readonly)
> + 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);
> +
> + 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, 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 ff5a77b0e2..693c75b45d 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 9e2e57459e..544328bb18 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -276,7 +276,7 @@ endif WITH_LIBXL
>
> if WITH_QEMU
> test_programs += qemuxml2argvtest qemuxml2xmltest \
> - qemudomainsnapshotxml2xmltest \
> + qemudomaincheckpointxml2xmltest qemudomainsnapshotxml2xmltest \
> qemumonitorjsontest qemuhotplugtest \
> qemuagenttest qemucapabilitiestest qemucaps2xmltest \
> qemumemlocktest \
> @@ -647,6 +647,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
> @@ -682,7 +687,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..8a7c0922c7
> --- /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 */
> + TEST_INVALID = 1 << 4, /* Test that input fails parse */
> +};
> +
> +static int
> +testCompareXMLToXMLFiles(const char *inxml,
> + const char *outxml,
> + unsigned int flags)
> +{
> + unsigned int parseflags = 0;
> + unsigned int formatflags = VIR_DOMAIN_CHECKPOINT_FORMAT_SECURE;
> + VIR_AUTOFREE(char *) inXmlData = NULL;
> + VIR_AUTOFREE(char *) outXmlData = NULL;
> + VIR_AUTOFREE(char *) actual = NULL;
> + VIR_AUTOUNREF(virDomainCheckpointDefPtr) def = NULL;
> +
> + if (flags & TEST_REDEFINE)
> + parseflags |= VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE;
> +
> + if (virTestLoadFile(inxml, &inXmlData) < 0)
> + return -1;
> +
> + if (!(flags & TEST_INVALID) &&
> + virTestLoadFile(outxml, &outXmlData) < 0)
> + return -1;
> +
> + if (!(def = virDomainCheckpointDefParseString(inXmlData, driver.caps,
> + driver.xmlopt,
> + parseflags))) {
> + if (flags & TEST_INVALID)
> + return 0;
> + return -1;
> + }
> + if (flags & TEST_PARENT) {
> + if (def->parent.parent_name)
> + return -1;
> + if (VIR_STRDUP(def->parent.parent_name, "1525111885") < 0)
> + return -1;
> + }
> + if (flags & TEST_VDA_BITMAP) {
> + virDomainCheckpointDiskDefPtr disk;
> +
> + if (VIR_EXPAND_N(def->disks, def->ndisks, 1) < 0)
> + return -1;
> + disk = &def->disks[0];
> + if (disk->bitmap)
> + return -1;
> + if (!disk->name) {
> + disk->type = VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP;
> + if (VIR_STRDUP(disk->name, "vda") < 0)
> + return -1;
> + } else if (STRNEQ(disk->name, "vda")) {
> + return -1;
> + }
> + if (VIR_STRDUP(disk->bitmap, def->parent.name) < 0)
> + return -1;
> + }
> + if (flags & TEST_SIZE) {
> + def->disks[0].size = 1048576;
> + formatflags |= VIR_DOMAIN_CHECKPOINT_FORMAT_SIZE;
> + }
> +
> + /* 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)))
> + return -1;
> +
> + if (STRNEQ(outXmlData, actual)) {
> + virTestDifferenceFull(stderr, outXmlData, outxml, actual, inxml);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +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)
> +# define DO_TEST_INVALID(name) \
> + DO_TEST("in->out", name, \
> + "qemudomaincheckpointxml2xmlin", \
> + "qemudomaincheckpointxml2xmlout", \
> + 0, TEST_INVALID)
> +
> + /* 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("disk-default", 1525889631, TEST_PARENT | 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);
> +
> + /* Tests of invalid user input */
> + DO_TEST_INVALID("disk-invalid");
> + DO_TEST_INVALID("name-invalid");
> +
> + 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
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jul 24, 2019 at 17:11:19 +0100, Daniel Berrange wrote: > On Wed, Jul 24, 2019 at 12:55:58AM -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. > > I'm in two minds as to whether this is really a good idea. > > I always have a strong tendancy to prefer consistency across the APIs > we expose, which would point to an opt-in flag for validation. If we > did later find we need a way to skip validation, then we end up having > to add a NO_VALIDATION API flag, which makes inconsitency even more > glaring to app devs > > I understand that this can help catch mistakes in apps, but I'm not > sold on it as a default, when it is easy for apps to opt-in if they > want to. I've specifically requested this behaviour as I don't think that there's value in not validating the XML. The XML parser is tied to what we parse anyways and the checks done when parsing are _WAY_ simpler if we know that all the values conform to the schema. Also if we'd ever want to do a generated parser, we will need to strictly adhere to the schema first. Consistency is great as long as it does not hinder progress. And in this case I think that validating everything right away is progress. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jul 24, 2019 at 06:17:24PM +0200, Peter Krempa wrote: > On Wed, Jul 24, 2019 at 17:11:19 +0100, Daniel Berrange wrote: > > On Wed, Jul 24, 2019 at 12:55:58AM -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. > > > > I'm in two minds as to whether this is really a good idea. > > > > I always have a strong tendancy to prefer consistency across the APIs > > we expose, which would point to an opt-in flag for validation. If we > > did later find we need a way to skip validation, then we end up having > > to add a NO_VALIDATION API flag, which makes inconsitency even more > > glaring to app devs > > > > I understand that this can help catch mistakes in apps, but I'm not > > sold on it as a default, when it is easy for apps to opt-in if they > > want to. > > I've specifically requested this behaviour as I don't think that there's > value in not validating the XML. The XML parser is tied to what we parse > anyways and the checks done when parsing are _WAY_ simpler if we know > that all the values conform to the schema. If there is no downside to validation though, why only do it for one schema. We should do it for all of the XML parsers we have. We didn't do this originally because we didn't have confidence in accuracy of the schemas, which was shown a sensible decision many times over as we found a great many schema bugs. None the less, it would still be possible to turn on validation for all our schemas, as simply declare the current _VALIDATE flags are now a no-op. There is the possibility that apps are lazy in creating XML documents adding elements that may not be supported with the version of libvirt they are talking to. That might be caught at parsing time or might be silently ignored with little ill effect. So enforcing validation would mean apps have to be more careful with the XML they feed libvirt. Such enforcement could be considered a good thing even if it risks highlighting existing broken behaviour in apps. > Also if we'd ever want to do a generated parser, we will need to > strictly adhere to the schema first. Consistency is great as long as it > does not hinder progress. And in this case I think that validating > everything right away is progress. If & when we attempt generated parsers, we still have the option to enforce schema validation at that time, so I don't see this as blocking. So overall, either we should turn on validation for all our schemas, or we should require a VALIDATE flag for this new API. Both avoid special case behaviour with the checkpoint APIs. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jul 24, 2019 at 17:40:51 +0100, Daniel Berrange wrote: > On Wed, Jul 24, 2019 at 06:17:24PM +0200, Peter Krempa wrote: > > On Wed, Jul 24, 2019 at 17:11:19 +0100, Daniel Berrange wrote: > > > On Wed, Jul 24, 2019 at 12:55:58AM -0500, Eric Blake wrote: [...] > So overall, either we should turn on validation for all our schemas, > or we should require a VALIDATE flag for this new API. Both avoid > special case behaviour with the checkpoint APIs. I definitely do not want to add a new API with no XML validation. Whether we'll require bundling a way bigger change which actually may break existing APPS using invalid XML is worth together with this, I'm not sure. But adding more legacy cruft seems to be pointless. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jul 24, 2019 at 06:55:18PM +0200, Peter Krempa wrote: > On Wed, Jul 24, 2019 at 17:40:51 +0100, Daniel Berrange wrote: > > On Wed, Jul 24, 2019 at 06:17:24PM +0200, Peter Krempa wrote: > > > On Wed, Jul 24, 2019 at 17:11:19 +0100, Daniel Berrange wrote: > > > > On Wed, Jul 24, 2019 at 12:55:58AM -0500, Eric Blake wrote: > > [...] > > > So overall, either we should turn on validation for all our schemas, > > or we should require a VALIDATE flag for this new API. Both avoid > > special case behaviour with the checkpoint APIs. > > I definitely do not want to add a new API with no XML validation. > Whether we'll require bundling a way bigger change which actually may > break existing APPS using invalid XML is worth together with this, I'm > not sure. > > But adding more legacy cruft seems to be pointless. I don't think its legacy cruft, when it is our normal practice, including in the new XML we added just in the release last month. Validation as standard is only a compelling thing if we're going todo it universally. When only 1 out of 14 schemas does validation that doesn't justify the divergance in behaviour IMHO. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jul 24, 2019 at 18:10:47 +0100, Daniel Berrange wrote: > On Wed, Jul 24, 2019 at 06:55:18PM +0200, Peter Krempa wrote: > > On Wed, Jul 24, 2019 at 17:40:51 +0100, Daniel Berrange wrote: > > > On Wed, Jul 24, 2019 at 06:17:24PM +0200, Peter Krempa wrote: > > > > On Wed, Jul 24, 2019 at 17:11:19 +0100, Daniel Berrange wrote: > > > > > On Wed, Jul 24, 2019 at 12:55:58AM -0500, Eric Blake wrote: > > > > [...] > > > > > So overall, either we should turn on validation for all our schemas, > > > or we should require a VALIDATE flag for this new API. Both avoid > > > special case behaviour with the checkpoint APIs. > > > > I definitely do not want to add a new API with no XML validation. > > Whether we'll require bundling a way bigger change which actually may > > break existing APPS using invalid XML is worth together with this, I'm > > not sure. > > > > But adding more legacy cruft seems to be pointless. > > I don't think its legacy cruft, when it is our normal practice, > including in the new XML we added just in the release last month. > > Validation as standard is only a compelling thing if we're going > todo it universally. When only 1 out of 14 schemas does validation > that doesn't justify the divergance in behaviour IMHO. Given that we've done VERY badly when adding validation and basically the only object supporting validation is the domain object (no, not even the snapshot object supported validation until earlier this month) having schemas is basically a joke. If we don't mandate it from the beginning we will never be able to do it without the risk of breaking users which try to use it wrongly. In my opinion the only sane approach is to validate always and the only exception is historical reasons. Note that requesting dropping mandatory validation will also require re-introducing the checks that Eric removed as pointles after I asked twice to add mandatory validation in recent reviews. At any rate I'm not going to object to dropping mandatory validation, but it will disappoint me greatly if we do that. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 7/24/19 11:40 AM, Daniel P. Berrangé wrote: > On Wed, Jul 24, 2019 at 06:17:24PM +0200, Peter Krempa wrote: >> On Wed, Jul 24, 2019 at 17:11:19 +0100, Daniel Berrange wrote: >>> On Wed, Jul 24, 2019 at 12:55:58AM -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. >>> >>> I'm in two minds as to whether this is really a good idea. >>> >>> I always have a strong tendancy to prefer consistency across the APIs >>> we expose, which would point to an opt-in flag for validation. If we >>> did later find we need a way to skip validation, then we end up having >>> to add a NO_VALIDATION API flag, which makes inconsitency even more >>> glaring to app devs >>> >>> I understand that this can help catch mistakes in apps, but I'm not >>> sold on it as a default, when it is easy for apps to opt-in if they >>> want to. The opt-in argument allowing us to skip something if we run into an RNG bug is slightly tempting, but >> >> I've specifically requested this behaviour as I don't think that there's >> value in not validating the XML. The XML parser is tied to what we parse >> anyways and the checks done when parsing are _WAY_ simpler if we know >> that all the values conform to the schema. the amount of simplifications when you can rely on the data already being validated was rather substantial. > > If there is no downside to validation though, why only do it for one > schema. We should do it for all of the XML parsers we have. We didn't > do this originally because we didn't have confidence in accuracy of > the schemas, which was shown a sensible decision many times over as > we found a great many schema bugs. Yes, I'd love to add more validation to more of our existing legacy APIs, but one project at a time. Maybe a libvirt.conf or qemu.conf switch to opt in to validation (and ignore the flags, where flags exist) is worthwhile (then, if you do run into a schema bug, you can temporarily weaken qemu.conf and rely on the flags on a per-API basis). > > None the less, it would still be possible to turn on validation for > all our schemas, as simply declare the current _VALIDATE flags are > now a no-op. It would also be possible to add a _VALIDATE flag for checkpoints that is initially treated as a no-op (ie. I perform the validation using the RNG schema no matter what, regardless of the flag's presence, because it makes the rest of my code shorter), but if we run into an RNG schema bug down the road, we could then still have the position of omitting the flag to bypass validation at that time. Then again, if we run into an RNG bug where we reject something that looks like it should be valid, an older libvirt will fail to parse the XML no matter what, then by the time you upgrade to a version of libvirt that honors the flag to allow a bypass, you've also upgraded to a version of libvirt that fixes the bug, so why do you need the bypass? > > There is the possibility that apps are lazy in creating XML > documents adding elements that may not be supported with the version > of libvirt they are talking to. That might be caught at parsing time > or might be silently ignored with little ill effect. So enforcing > validation would mean apps have to be more careful with the XML they > feed libvirt. Such enforcement could be considered a good thing even > if it risks highlighting existing broken behaviour in apps. Existing APIs are produced by existing apps (and yes, they have been lazy, where strict validation would probably break some apps). But checkpoints are a brand-new API, and as there are no existing apps producing checkpoint XML (only new stuff), now is a great time to enforce that the new stuff is sane from the get-go. > So overall, either we should turn on validation for all our schemas, > or we should require a VALIDATE flag for this new API. Both avoid > special case behaviour with the checkpoint APIs. There's still time to add it in prior to the 5.6 release if we decide we need the opt-out capability. I'm probably going to commit this patch as-is while we continue the discussion, where we can decide if a follow-up patch needs to make it opt-in rather than mandatory. -- 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
On 7/24/19 5:42 PM, Eric Blake wrote: > the amount of simplifications when you can rely on the data already > being validated was rather substantial. > >> >> If there is no downside to validation though, why only do it for one >> schema. We should do it for all of the XML parsers we have. We didn't >> do this originally because we didn't have confidence in accuracy of >> the schemas, which was shown a sensible decision many times over as >> we found a great many schema bugs. > > Yes, I'd love to add more validation to more of our existing legacy > APIs, but one project at a time. Maybe a libvirt.conf or qemu.conf > switch to opt in to validation (and ignore the flags, where flags exist) > is worthwhile (then, if you do run into a schema bug, you can > temporarily weaken qemu.conf and rely on the flags on a per-API basis). > >> >> None the less, it would still be possible to turn on validation for >> all our schemas, as simply declare the current _VALIDATE flags are >> now a no-op. > > It would also be possible to add a _VALIDATE flag for checkpoints that > is initially treated as a no-op (ie. I perform the validation using the > RNG schema no matter what, regardless of the flag's presence, because it > makes the rest of my code shorter), but if we run into an RNG schema bug > down the road, we could then still have the position of omitting the > flag to bypass validation at that time. Then again, if we run into an > RNG bug where we reject something that looks like it should be valid, an > older libvirt will fail to parse the XML no matter what, then by the > time you upgrade to a version of libvirt that honors the flag to allow a > bypass, you've also upgraded to a version of libvirt that fixes the bug, > so why do you need the bypass? Thinking just a bit more: for the case of defining a new checkpoint, mandatory schema validation is still appealing (the new RNG file is pretty small, and easy enough to compare to the code in checkpoint_conf.c). But for the case of the VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE flag, which includes a full <domain> sub-element, there _is_ a risk that we still have inconsistencies between the domain XML we produce and what the schema parser will require during the REDEFINE (because we don't have schema validation always enabled for domains in general, and because <domain> is so much of a bigger beast to begin with). The last thing we need is for restarting libvirtd to fail to reload all checkpoints merely because of a schema failure on the <domain> subelement. I may still end up implementing a VALIDATE flag (maybe just at the checkpoint_conf.c level for internal use, rather than as a public API flag) that is a no-op for checkpoint creation but allows bypass of the validation of a <domain> sub-element during REDEFINE (and I would actually code it up that way too: try to validate the XML as-is; if it fails, then edit a copy of the XML to remove the <domain> element and try to revalidate the rest, rather than skipping validation altogether - so that the rest of the checkpoint code can still rely on validation rather than reinstating redundant checking code). But it is definitely material for a separate patch: > There's still time to add it in prior to the 5.6 release if we decide we > need the opt-out capability. I'm probably going to commit this patch > as-is while we continue the discussion, where we can decide if a > follow-up patch needs to make it opt-in rather than mandatory. > -- 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
[adding bug-gnulib]
On 7/24/19 12:55 AM, 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.
> +++ b/src/conf/checkpoint_conf.c
> +#include <config.h>
> +
> +#include "configmake.h"
> +#include "internal.h"
> +#include "virbitmap.h"
> +#include "virbuffer.h"
> +#include "datatypes.h"
This causes a compilation failure on mingw, due to libvirt's
"datatypes.h" including <winsock.h> after the point at which gnulib's
"configmake.h" has already #define'd DATADIR into a string, but mingw's
winsock.h tries to use DATADIR as a data-type tag name:
make all-am
make[1]: Entering directory '/home/berrange/src/virt/libvirt/src'
CC conf/libvirt_conf_la-checkpoint_conf.lo
In file included from
/usr/i686-w64-mingw32/sys-root/mingw/include/objbase.h:66,
from
/usr/i686-w64-mingw32/sys-root/mingw/include/ole2.h:17,
from
/usr/i686-w64-mingw32/sys-root/mingw/include/wtypes.h:12,
from
/usr/i686-w64-mingw32/sys-root/mingw/include/winscard.h:10,
from
/usr/i686-w64-mingw32/sys-root/mingw/include/windows.h:97,
from
/usr/i686-w64-mingw32/sys-root/mingw/include/winsock2.h:23,
from ../gnulib/lib/unistd.h:48,
from ./driver.h:24,
from ./datatypes.h:26,
from conf/checkpoint_conf.c:28:
/usr/i686-w64-mingw32/sys-root/mingw/include/objidl.h:12275:2: error:
expected identifier or '(' before string constant
} DATADIR;
^~~~~~~~~
make[1]: *** [Makefile:10127: conf/libvirt_conf_la-checkpoint_conf.lo]
Error 1
And it's not the first time libvirt has run into this issue; I've found
the following commits in 2015 that worked around it:
https://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=976abdf6
https://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=bd205a90
Gnulib should work around this: perhaps "configmake.h" should include
<unistd.h> first when built on mingw, as that is sufficient to trigger
enough other headers to be included such that a later inclusion of
<winsock.h> after "configmake.h" no longer runs into an issue with the
DATADIR pollution breaking compilation, or perhaps gnulib can wrap
<winsock.h> in such a way that it no longer depends on a tag name
DATADIR. In the meantime, I'll push an obvious fix to libvirt to
reorder the header inclusions to work around the problem.
--
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.