On Sat, Jul 06, 2019 at 22:56:04 -0500, Eric Blake wrote:
> Add a new file checkpoint_conf.c that performs the translation to and
> from new XML describing a checkpoint. The code shares a common base
> class with snapshots, since a checkpoint similarly represents the
> domain state at a moment in time. Add some basic testing of round trip
> XML handling through the new code.
>
> Of note - this code intentionally differs from snapshots in that XML
> schema validation is unconditional, rather than based on a public API
> flag. Also, the redefine flag requires the <domain> sub-element to be
> present, rather than catering to historical back-compat to older
> versions.
>
> Signed-off-by: Eric Blake <eblake@xxxxxxxxxx>
> ---
[...]
> diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c
> new file mode 100644
> index 0000000000..d50d63ac0e
> --- /dev/null
> +++ b/src/conf/checkpoint_conf.c
> @@ -0,0 +1,576 @@
[...]
> +static int
> +virDomainCheckpointDiskDefParseXML(xmlNodePtr node,
> + xmlXPathContextPtr ctxt,
> + virDomainCheckpointDiskDefPtr def)
> +{
> + int ret = -1;
> + char *checkpoint = NULL;
> + char *bitmap = NULL;
> + xmlNodePtr saved = ctxt->node;
> +
> + ctxt->node = node;
> +
> + def->name = virXMLPropString(node, "name");
> + if (!def->name) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing name from disk checkpoint element"));
'name' is mandatory in the schema for <disk>
> + goto cleanup;
> + }
> +
> + checkpoint = virXMLPropString(node, "checkpoint");
This attribute seems to be mandatory in the schema [1]
> + if (checkpoint) {
> + def->type = virDomainCheckpointTypeFromString(checkpoint);
> + if (def->type <= 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unknown disk checkpoint setting '%s'"),
> + checkpoint);
These are also mandated.
> + goto cleanup;
> + }
> + } else {
> + def->type = VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP;
[1] so this should never happen
> + }
> +
> + bitmap = virXMLPropString(node, "bitmap");
> + if (bitmap) {
> + if (def->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("disk checkpoint bitmap '%s' requires "
> + "type='bitmap'"),
> + bitmap);
And this is also mandated by the schema.
> + goto cleanup;
> + }
> + VIR_STEAL_PTR(def->bitmap, bitmap);
> + }
> +
> + ret = 0;
> + cleanup:
> + ctxt->node = saved;
> +
> + VIR_FREE(checkpoint);
> + VIR_FREE(bitmap);
> + if (ret < 0)
> + virDomainCheckpointDiskDefClear(def);
> + return ret;
> +}
> +
> +/* flags is bitwise-or of virDomainCheckpointParseFlags. If flags
> + * does not include VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE, then caps
> + * and current are ignored.
> + */
> +static virDomainCheckpointDefPtr
> +virDomainCheckpointDefParse(xmlXPathContextPtr ctxt,
> + virCapsPtr caps,
> + virDomainXMLOptionPtr xmlopt,
> + bool *current,
> + unsigned int flags)
> +{
> + virDomainCheckpointDefPtr def = NULL;
> + virDomainCheckpointDefPtr ret = NULL;
> + xmlNodePtr *nodes = NULL;
> + size_t i;
> + int n;
> + char *creation = NULL;
> + int active;
> + char *tmp;
> +
> + if (!(def = virDomainCheckpointDefNew()))
> + return NULL;
> +
> + def->parent.name = virXPathString("string(./name)", ctxt);
> + if (def->parent.name == NULL) {
> + if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("a redefined checkpoint must have a name"));
> + goto cleanup;
> + }
> + }
> +
> + def->parent.description = virXPathString("string(./description)", ctxt);
> +
> + if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE) {
> + if (virXPathLongLong("string(./creationTime)", ctxt,
> + &def->parent.creationTime) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing creationTime from existing checkpoint"));
> + goto cleanup;
In v8 I've pointed out that we should not intermix validation of the XML
with the parsing.
> + }
> +
> + def->parent.parent_name = virXPathString("string(./parent/name)", ctxt);
> +
> + if (virXPathInt("string(./current)", ctxt, &active) < 0 || !current) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing current from existing checkpoint"));
> + goto cleanup;
> + }
> + *current = !!active;
> +
> + if ((tmp = virXPathString("string(./domain/@type)", ctxt))) {
> + int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
> + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
> + xmlNodePtr domainNode = virXPathNode("./domain", ctxt);
> +
> + VIR_FREE(tmp);
> + if (!domainNode) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing domain in checkpoint"));
> + goto cleanup;
> + }
> + def->parent.dom = virDomainDefParseNode(ctxt->node->doc, domainNode,
> + caps, xmlopt, NULL,
> + domainflags);
> + if (!def->parent.dom)
> + goto cleanup;
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing domain in checkpoint redefine"));
> + goto cleanup;
> + }
> + } else if (virDomainXMLOptionRunMomentPostParse(xmlopt, &def->parent) < 0) {
> + goto cleanup;
> + }
> +
> + if ((n = virXPathNodeSet("./disks/*", ctxt, &nodes)) < 0)
> + goto cleanup;
> + if (n && VIR_ALLOC_N(def->disks, n) < 0)
> + goto cleanup;
> + def->ndisks = n;
> + for (i = 0; i < def->ndisks; i++) {
> + if (virDomainCheckpointDiskDefParseXML(nodes[i], ctxt,
> + &def->disks[i]) < 0)
> + goto cleanup;
> + }
> +
> + VIR_STEAL_PTR(ret, def);
> +
> + cleanup:
> + VIR_FREE(creation);
> + VIR_FREE(nodes);
> + virObjectUnref(def);
> +
> + return ret;
> +}
> +
> +static virDomainCheckpointDefPtr
> +virDomainCheckpointDefParseNode(xmlDocPtr xml,
> + xmlNodePtr root,
> + virCapsPtr caps,
> + virDomainXMLOptionPtr xmlopt,
> + bool *current,
> + unsigned int flags)
> +{
> + xmlXPathContextPtr ctxt = NULL;
> + virDomainCheckpointDefPtr def = NULL;
> + VIR_AUTOFREE(char *) schema = NULL;
> +
> + if (!virXMLNodeNameEqual(root, "domaincheckpoint")) {
> + virReportError(VIR_ERR_XML_ERROR, "%s", _("domaincheckpoint"));
> + goto cleanup;
The schema will be able to validate this.
> + }
> +
> + /* This is a new enough API to make schema validation unconditional */
> + schema = virFileFindResource("domaincheckpoint.rng",
> + abs_top_srcdir "/docs/schemas",
> + PKGDATADIR "/schemas");
> + if (!schema)
> + return NULL;
> + if (virXMLValidateAgainstSchema(schema, xml) < 0)
> + return NULL;
> +
> + ctxt = xmlXPathNewContext(xml);
> + if (ctxt == NULL) {
> + virReportOOMError();
> + goto cleanup;
> + }
> +
> + ctxt->node = root;
> + def = virDomainCheckpointDefParse(ctxt, caps, xmlopt, current, flags);
> + cleanup:
> + xmlXPathFreeContext(ctxt);
> + return def;
> +}
[...]
> +int
> +virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def)
> +{
> + int ret = -1;
> + virBitmapPtr map = NULL;
> + size_t i;
> + int ndisks;
> + int checkpoint_default = VIR_DOMAIN_CHECKPOINT_TYPE_NONE;
> +
> + if (!def->parent.dom) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing domain in checkpoint"));
> + goto cleanup;
Is this possible?
> + }
> +
> + if (def->ndisks > def->parent.dom->ndisks) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("too many disk checkpoint requests for domain"));
> + goto cleanup;
> + }
> +
> + /* Unlikely to have a guest without disks but technically possible. */
> + if (!def->parent.dom->ndisks) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("domain must have at least one disk to perform "
> + "checkpoints"));
> + goto cleanup;
> + }
> +
> + /* If <disks> omitted, do bitmap on all disks; otherwise, do nothing
> + * for omitted disks */
> + if (!def->ndisks)
> + checkpoint_default = VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP;
> +
> + if (!(map = virBitmapNew(def->parent.dom->ndisks)))
> + goto cleanup;
> +
> + /* Double check requested disks. */
> + for (i = 0; i < def->ndisks; i++) {
> + virDomainCheckpointDiskDefPtr disk = &def->disks[i];
> + int idx = virDomainDiskIndexByName(def->parent.dom, disk->name, false);
> +
> + if (idx < 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("no disk named '%s'"), disk->name);
> + goto cleanup;
> + }
> +
> + if (virBitmapIsBitSet(map, idx)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("disk '%s' specified twice"),
> + disk->name);
> + goto cleanup;
> + }
> + ignore_value(virBitmapSetBit(map, idx));
> + disk->idx = idx;
> +
> + if (STRNEQ(disk->name, def->parent.dom->disks[idx]->dst)) {
> + VIR_FREE(disk->name);
> + if (VIR_STRDUP(disk->name, def->parent.dom->disks[idx]->dst) < 0)
> + goto cleanup;
> + }
> + }
> +
> + /* Provide defaults for all remaining disks. */
> + ndisks = def->ndisks;
> + if (VIR_EXPAND_N(def->disks, def->ndisks,
> + def->parent.dom->ndisks - def->ndisks) < 0)
> + goto cleanup;
> +
> + for (i = 0; i < def->parent.dom->ndisks; i++) {
> + virDomainCheckpointDiskDefPtr disk;
> +
> + if (virBitmapIsBitSet(map, i))
> + continue;
> + disk = &def->disks[ndisks++];
> + if (VIR_STRDUP(disk->name, def->parent.dom->disks[i]->dst) < 0)
> + goto cleanup;
> + disk->idx = i;
> +
> + /* Don't checkpoint empty drives */
Without -blockdev you'll also have a problem also with readonly disks as
qemu does not open the actual file for writing and thus writing the
bitmap won't work.
Also ... it doesn't make much sense ... since the bitmap will be empty
forever.
> + if (virStorageSourceIsEmpty(def->parent.dom->disks[i]->src))
> + disk->type = VIR_DOMAIN_CHECKPOINT_TYPE_NONE;
> + else
> + disk->type = checkpoint_default;
> + }
> +
> + qsort(&def->disks[0], def->ndisks, sizeof(def->disks[0]),
> + virDomainCheckpointCompareDiskIndex);
> +
> + /* Generate default bitmap names for checkpoint */
> + if (virDomainCheckpointDefAssignBitmapNames(def) < 0)
> + goto cleanup;
> +
> + ret = 0;
> +
> + cleanup:
> + virBitmapFree(map);
> + return ret;
> +}
[...]
> +static int
> +virDomainCheckpointDiskDefFormat(virBufferPtr buf,
> + virDomainCheckpointDiskDefPtr disk,
> + unsigned int flags)
> +{
> + if (!disk->name)
> + return 0;
> +
> + virBufferEscapeString(buf, "<disk name='%s'", disk->name);
> + if (disk->type)
> + virBufferAsprintf(buf, " checkpoint='%s'",
> + virDomainCheckpointTypeToString(disk->type));
> + if (disk->bitmap) {
> + virBufferEscapeString(buf, " bitmap='%s'", disk->bitmap);
> + if (flags & VIR_DOMAIN_CHECKPOINT_FORMAT_SIZE)
> + virBufferAsprintf(buf, " size='%llu'", disk->size);
The schema does not tie the size attribute to the presence of the
bitmap name.
> + }
> + virBufferAddLit(buf, "/>\n");
> + return 0;
> +}
[...]
ACK in general, most of the comments are not problems, if they'll be
addressed later. Note that since this patch introduces a big pile of
old-style code and technical debt it's more than desired that you
address it at some time.
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
