[libvirt] [PATCH v8 06/21] backup: Parse and output checkpoint XML
Peter Krempa
pkrempa at redhat.com
Wed Apr 24 15:53:01 UTC 2019
On Wed, Apr 17, 2019 at 09:09:06 -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.
>
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
> src/conf/checkpoint_conf.h | 96 +++
> src/conf/virconftypes.h | 3 +
> po/POTFILES | 1 +
> src/conf/Makefile.inc.am | 2 +
> src/conf/checkpoint_conf.c | 550 ++++++++++++++++++
> src/libvirt_private.syms | 8 +
> tests/Makefile.am | 9 +-
> .../internal-active-invalid.xml | 53 ++
> .../internal-inactive-invalid.xml | 53 ++
> tests/domaincheckpointxml2xmltest.c | 223 +++++++
> 10 files changed, 996 insertions(+), 2 deletions(-)
> create mode 100644 src/conf/checkpoint_conf.h
> create mode 100644 src/conf/checkpoint_conf.c
> create mode 100644 tests/domaincheckpointxml2xmlout/internal-active-invalid.xml
> create mode 100644 tests/domaincheckpointxml2xmlout/internal-inactive-invalid.xml
> create mode 100644 tests/domaincheckpointxml2xmltest.c
>
> diff --git a/src/conf/checkpoint_conf.h b/src/conf/checkpoint_conf.h
> new file mode 100644
> index 0000000000..6647d0cd7c
> --- /dev/null
> +++ b/src/conf/checkpoint_conf.h
> @@ -0,0 +1,96 @@
> +/*
> + * 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/>.
> + */
> +
> +#ifndef LIBVIRT_CHECKPOINT_CONF_H
> +# define LIBVIRT_CHECKPOINT_CONF_H
> +
> +# include "internal.h"
> +# include "domain_conf.h"
> +# include "moment_conf.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 common;
> +
> + /* Additional Public XML. */
> + size_t ndisks; /* should not exceed dom->ndisks */
> + virDomainCheckpointDiskDef *disks;
> +};
> +
> +
> +typedef enum {
> + VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE = 1 << 0,
> + VIR_DOMAIN_CHECKPOINT_PARSE_INTERNAL = 1 << 1,
These allow an easy pass to sneak validation into the parser ... I'm not
a fan of that.
> +} 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_INTERNAL = 1 << 3,
> + VIR_DOMAIN_CHECKPOINT_FORMAT_CURRENT = 1 << 4,
> +} virDomainCheckpointFormatFlags;
[...]
> +
> +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);
This should be expressable by schema. If we make schema validation
mandatory for the new APIs which we should anyways we can skip all of
these as we'll automagically get them validated before.
> + 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
> + * are ignored. If flags does not include
> + * VIR_DOMAIN_CHECKPOINT_PARSE_INTERNAL, then current is ignored.
> + */
> +static virDomainCheckpointDefPtr
> +virDomainCheckpointDefParse(xmlXPathContextPtr ctxt,
> + virCapsPtr caps,
> + virDomainXMLOptionPtr xmlopt,
> + bool *current,
> + unsigned int flags)
> +{
> + virDomainCheckpointDefPtr def = NULL;
> + virDomainCheckpointDefPtr ret = NULL;
> + xmlNodePtr *nodes = NULL;
I'm not going to point out that we have more modern approaches here ...
> + size_t i;
> + int n;
> + char *creation = NULL;
> + int active;
> + char *tmp;
> +
> + if (VIR_ALLOC(def) < 0)
> + goto cleanup;
> +
> + def->common.name = virXPathString("string(./name)", ctxt);
> + if (def->common.name == NULL) {
> + if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE) {
... but validation of the XML should not be intermixed with the parser
itself. The parser should only validate what is described by the schema.
(I'm aware that there is plenty prior art, but adding new code just
increases the technical debt and this kind of stuff is hard to refactor
later)
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("a redefined checkpoint must have a name"));
> + goto cleanup;
> + }
> + }
> +
> + def->common.description = virXPathString("string(./description)", ctxt);
> +
> + if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE) {
> + if (virXPathLongLong("string(./creationTime)", ctxt,
> + &def->common.creationTime) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing creationTime from existing checkpoint"));
> + goto cleanup;
> + }
> +
> + def->common.parent = 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"));
> + goto cleanup;
> + }
> + def->common.dom = virDomainDefParseNode(ctxt->node->doc, domainNode,
> + caps, xmlopt, NULL,
> + domainflags);
> + if (!def->common.dom)
> + goto cleanup;
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing domain in checkpoint redefine"));
> + goto cleanup;
> + }
> + } else if (virDomainXMLOptionRunMomentPostParse(xmlopt, &def->common) < 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;
> + }
> +
> + if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_INTERNAL) {
> + if (!current) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("internal parse requested with NULL current"));
> + goto cleanup;
> + }
> + if (virXPathInt("string(./active)", ctxt, &active) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Could not find 'active' element"));
What is this supposed to be used for?
> + goto cleanup;
> + }
> + *current = active != 0;
> + }
> +
> + VIR_STEAL_PTR(ret, def);
> +
> + cleanup:
> + VIR_FREE(creation);
> + VIR_FREE(nodes);
> + virDomainCheckpointDefFree(def);
> +
> + return ret;
> +}
> +
> +virDomainCheckpointDefPtr
> +virDomainCheckpointDefParseNode(xmlDocPtr xml,
> + xmlNodePtr root,
> + virCapsPtr caps,
> + virDomainXMLOptionPtr xmlopt,
> + bool *current,
> + unsigned int flags)
> +{
> + xmlXPathContextPtr ctxt = NULL;
> + virDomainCheckpointDefPtr def = NULL;
> +
> + if (!virXMLNodeNameEqual(root, "domaincheckpoint")) {
> + virReportError(VIR_ERR_XML_ERROR, "%s", _("domaincheckpoint"));
> + goto cleanup;
> + }
> +
> + ctxt = xmlXPathNewContext(xml);
> + if (ctxt == NULL) {
> + virReportOOMError();
> + goto cleanup;
There's nothing to clean up.
> + }
> +
> + 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.
Isn't a bitmap a qemuism?
> + */
> +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->common.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->common.dom) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing domain in checkpoint"));
> + goto cleanup;
This seems unrelated to what the function is doing.
> + }
> +
> + if (def->ndisks > def->common.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->common.dom->ndisks) {
Use an explicit == 0 check here. You used plenty of explicit == NULL
checks above.
> + 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)
Again, explicit comparison please.
> + checkpoint_default = VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP;
> +
> + if (!(map = virBitmapNew(def->common.dom->ndisks)))
> + goto cleanup;
> +
> + /* Double check requested disks. */
> + for (i = 0; i < def->ndisks; i++) {
> + virDomainCheckpointDiskDefPtr disk = &def->disks[i];
> + int idx = virDomainDiskIndexByName(def->common.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;
Storing this is dangerous with disk hotplug.
> +
> + if (STRNEQ(disk->name, def->common.dom->disks[idx]->dst)) {
> + VIR_FREE(disk->name);
> + if (VIR_STRDUP(disk->name, def->common.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->common.dom->ndisks - def->ndisks) < 0)
> + goto cleanup;
> +
> + for (i = 0; i < def->common.dom->ndisks; i++) {
> + virDomainCheckpointDiskDefPtr disk;
> +
> + if (virBitmapIsBitSet(map, i))
> + continue;
> + disk = &def->disks[ndisks++];
> + if (VIR_STRDUP(disk->name, def->common.dom->disks[i]->dst) < 0)
> + goto cleanup;
> + disk->idx = i;
> +
> + /* Don't checkpoint empty drives */
What about readonly drives?
> + if (virStorageSourceIsEmpty(def->common.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)
We should make type mandatory.
> + 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->common.name);
> + virBufferEscapeString(buf, "<description>%s</description>\n",
> + def->common.description);
> +
> + if (def->common.parent) {
> + virBufferAddLit(buf, "<parent>\n");
> + virBufferAdjustIndent(buf, 2);
> + virBufferEscapeString(buf, "<name>%s</name>\n", def->common.parent);
> + virBufferAdjustIndent(buf, -2);
> + virBufferAddLit(buf, "</parent>\n");
> + }
> +
> + if (def->common.creationTime)
> + virBufferAsprintf(buf, "<creationTime>%lld</creationTime>\n",
> + def->common.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->common.dom, caps, domainflags, buf,
> + xmlopt) < 0)
> + goto error;
> +
> + if (flags & VIR_DOMAIN_CHECKPOINT_FORMAT_INTERNAL)
> + virBufferAsprintf(buf, "<active>%d</active>\n",
> + !!(flags & VIR_DOMAIN_CHECKPOINT_FORMAT_CURRENT));
> +
> + virBufferAdjustIndent(buf, -2);
> + virBufferAddLit(buf, "</domaincheckpoint>\n");
> +
> + if (virBufferCheckError(buf) < 0)
> + goto error;
> +
> + return 0;
> +
> + error:
> + virBufferFreeAndReset(buf);
I'm not sure it's fair to the caller to free it's buffer.
> + 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_INTERNAL |
> + VIR_DOMAIN_CHECKPOINT_FORMAT_CURRENT, NULL);
> + if (virDomainCheckpointDefFormatInternal(&buf, def, caps, xmlopt,
> + flags) < 0)
> + return NULL;
> +
> + return virBufferContentAndReset(&buf);
> +}
> diff --git a/tests/domaincheckpointxml2xmlout/internal-active-invalid.xml b/tests/domaincheckpointxml2xmlout/internal-active-invalid.xml
> new file mode 100644
> index 0000000000..a518c58915
> --- /dev/null
> +++ b/tests/domaincheckpointxml2xmlout/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'>
Thinking about it a bit. Is it necessary to store domain at all here?
The checkpoint does not allow to roll back state as snapshots do so
having the domain is quite pointless.
Additionally with external snapshots the checkpoint actually needs to be
changed with a snapshot as moving around in the snapshot tree makes some
checkpoints invalid.
> + <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/domaincheckpointxml2xmltest.c b/tests/domaincheckpointxml2xmltest.c
Since this is not prefixed with qemu ...
> new file mode 100644
> index 0000000000..11ec61658b
> --- /dev/null
> +++ b/tests/domaincheckpointxml2xmltest.c
> @@ -0,0 +1,223 @@
> +#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"
... it should not require qemu. Either drop those or rename it to
qemudomaincheckpointxml2xmltest ...
> +# include "checkpoint_conf.h"
> +# include "testutilsqemu.h"
> +# include "virstring.h"
> +
> +# define VIR_FROM_THIS VIR_FROM_NONE
> +
> +static virQEMUDriver driver;
> +
> +enum {
> + TEST_INTERNAL = 1 << 0, /* Test use of INTERNAL parse/format flag */
> + TEST_REDEFINE = 1 << 1, /* Test use of REDEFINE parse flag */
> + TEST_PARENT = 1 << 2, /* hard-code parent after parse */
> + TEST_VDA_BITMAP = 1 << 3, /* hard-code disk vda after parse */
> + TEST_SIZE = 1 << 4, /* 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_INTERNAL) {
> + parseflags |= VIR_DOMAIN_CHECKPOINT_PARSE_INTERNAL;
> + formatflags |= VIR_DOMAIN_CHECKPOINT_FORMAT_INTERNAL;
> + }
> +
> + 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,
... especially since it wants to use the qemu post parse callbacks.
> + parseflags)))
> + goto cleanup;
> + if (cur) {
> + if (!(flags & TEST_REDEFINE))
> + goto cleanup;
> + formatflags |= VIR_DOMAIN_CHECKPOINT_FORMAT_CURRENT;
> + }
> + if (flags & TEST_PARENT) {
> + if (def->common.parent)
> + goto cleanup;
> + if (VIR_STRDUP(def->common.parent, "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->common.name) < 0)
> + goto cleanup;
> + }
> + 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->common.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);
> + virDomainCheckpointDefFree(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, \
> + "domaincheckpointxml2xmlin", \
> + "domaincheckpointxml2xmlout", \
> + time, flags)
> +# define DO_TEST_OUT(name, flags) \
> + DO_TEST("out->out", name, \
> + "domaincheckpointxml2xmlout", \
> + "domaincheckpointxml2xmlout", \
> + 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);
> +
> + /* Tests of internal state saving - the <active> element is not
> + * permitted or exposed to user XML, so the files are named to
> + * skip schema validation */
> + DO_TEST_OUT("internal-active-invalid", TEST_INTERNAL);
> + DO_TEST_OUT("internal-inactive-invalid", TEST_INTERNAL);
> + /* 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190424/7ca9cd1d/attachment-0001.sig>
More information about the libvir-list
mailing list