10:24 p.m.
On 7/8/19 7:06 AM, Peter Krempa wrote:
On Sat, Jul 06, 2019 at 22:56:04 -0500, Eric Blake wrote:
> Add a new file checkpoint_conf.c that performs the translation to and
> from new XML describing a checkpoint. The code shares a common base
> class with snapshots, since a checkpoint similarly represents the
> domain state at a moment in time. Add some basic testing of round trip
> XML handling through the new code.
>
> Of note - this code intentionally differs from snapshots in that XML
> schema validation is unconditional, rather than based on a public API
> flag. Also, the redefine flag requires the <domain> sub-element to be
> present, rather than catering to historical back-compat to older
> versions.
>
> Signed-off-by: Eric Blake <eblake(a)redhat.com>
> ---
> +static int
> +virDomainCheckpointDiskDefParseXML(xmlNodePtr node,
> + xmlXPathContextPtr ctxt,
> + virDomainCheckpointDiskDefPtr def)
> +{
> + int ret = -1;
> + char *checkpoint = NULL;
> + char *bitmap = NULL;
> + xmlNodePtr saved = ctxt->node;
> +
> + ctxt->node = node;
> +
> + def->name = virXMLPropString(node, "name");
> + if (!def->name) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing name from disk checkpoint element"));
'name' is mandatory in the schema for <disk>
True, and I've fixed things to mandate schema compliance, so I'll drop
the unreachable error.
> + goto cleanup;
> + }
> +
> + checkpoint = virXMLPropString(node, "checkpoint");
This attribute seems to be mandatory in the schema [1]
Not quite. It is mandatory if you want checkpoint='no', but optional if
you want to default to checkpoint='bitmap'.
> + if (checkpoint) {
> + def->type = virDomainCheckpointTypeFromString(checkpoint);
> + if (def->type <= 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unknown disk checkpoint setting
'%s'"),
> + checkpoint);
These are also mandated.
Dropping another unreachable error.
> + goto cleanup;
> + }
> + } else {
> + def->type = VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP;
[1] so this should never happen
The schema permits it to be absent; I can enhance the
qemudomaincheckpointxml2xml test to cover that.
> + }
> +
> + bitmap = virXMLPropString(node, "bitmap");
> + if (bitmap) {
> + if (def->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("disk checkpoint bitmap '%s' requires
"
> + "type='bitmap'"),
> + bitmap);
And this is also mandated by the schema.
Correct, dropping another dead block.
> + goto cleanup;
> + }
> + VIR_STEAL_PTR(def->bitmap, bitmap);
> + }
> +
> + ret = 0;
> + cleanup:
> + ctxt->node = saved;
> +
> + VIR_FREE(checkpoint);
> + VIR_FREE(bitmap);
> + if (ret < 0)
> + virDomainCheckpointDiskDefClear(def);
And with a couple of VIR_AUTOFREE and VIR_XPATH_NODE_AUTORESTORE, this
entire cleanup label can disappear.
> + return ret;
> +}
> +
> +/* flags is bitwise-or of virDomainCheckpointParseFlags. If flags
> + * does not include VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE, then caps
> + * and current are ignored.
> + */
> +static virDomainCheckpointDefPtr
> +virDomainCheckpointDefParse(xmlXPathContextPtr ctxt,
> + virCapsPtr caps,
> + virDomainXMLOptionPtr xmlopt,
> + bool *current,
> + unsigned int flags)
> +{
> + virDomainCheckpointDefPtr def = NULL;
> + virDomainCheckpointDefPtr ret = NULL;
> + xmlNodePtr *nodes = NULL;
> + size_t i;
> + int n;
> + char *creation = NULL;
> + int active;
> + char *tmp;
> +
> + if (!(def = virDomainCheckpointDefNew()))
> + return NULL;
> +
> + def->parent.name = virXPathString("string(./name)", ctxt);
> + if (def->parent.name == NULL) {
> + if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("a redefined checkpoint must have a name"));
> + goto cleanup;
> + }
> + }
> +
> + def->parent.description = virXPathString("string(./description)",
ctxt);
> +
> + if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE) {
> + if (virXPathLongLong("string(./creationTime)", ctxt,
> + &def->parent.creationTime) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing creationTime from existing
checkpoint"));
> + goto cleanup;
In v8 I've pointed out that we should not intermix validation of the XML
with the parsing.
If I'm understanding, you want me to populate the entire struct with as
much (or as little) information as possible in the first half of the
function, then in the second half, if PARSE_REDEFINE was set, check that
the information that is optional for normal defines is present for a
redefine? The RNG schema doesn't make that easy to has something
optional by default but mandatory when a flag was passed, so there still
has to be a manual check; for timestamp it might be easy to validate
that a non-zero timestamp was set by the parse code, but it may be
harder for other elements where you are complaining about intermixed
validation. In the meantime, this copies the flow used by snapshots, so
I'm hesitant to rearrange it too much.
> +static virDomainCheckpointDefPtr
> +virDomainCheckpointDefParseNode(xmlDocPtr xml,
> + xmlNodePtr root,
> + virCapsPtr caps,
> + virDomainXMLOptionPtr xmlopt,
> + bool *current,
> + unsigned int flags)
> +{
> + xmlXPathContextPtr ctxt = NULL;
> + virDomainCheckpointDefPtr def = NULL;
> + VIR_AUTOFREE(char *) schema = NULL;
> +
> + if (!virXMLNodeNameEqual(root, "domaincheckpoint")) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
_("domaincheckpoint"));
> + goto cleanup;
The schema will be able to validate this.
Concur, another dead check dropped.
> +int
> +virDomainCheckpointAlignDisks(virDomainCheckpointDefPtr def)
> +{
> + int ret = -1;
> + virBitmapPtr map = NULL;
> + size_t i;
> + int ndisks;
> + int checkpoint_default = VIR_DOMAIN_CHECKPOINT_TYPE_NONE;
> +
> + if (!def->parent.dom) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing domain in checkpoint"));
> + goto cleanup;
Is this possible?
The schema doesn't guarantee it, but it is a coding error if the driver
didn't set the domain. But since we don't use assert(), I don't have
any better way to express that fact.
> + for (i = 0; i < def->parent.dom->ndisks; i++) {
> + virDomainCheckpointDiskDefPtr disk;
> +
> + if (virBitmapIsBitSet(map, i))
> + continue;
> + disk = &def->disks[ndisks++];
> + if (VIR_STRDUP(disk->name, def->parent.dom->disks[i]->dst) <
0)
> + goto cleanup;
> + disk->idx = i;
> +
> + /* Don't checkpoint empty drives */
Without -blockdev you'll also have a problem also with readonly disks as
qemu does not open the actual file for writing and thus writing the
bitmap won't work.
Also ... it doesn't make much sense ... since the bitmap will be empty
forever.
True; I'll tweak things so that readonly disks do not default to having
a checkpoint added.
> +static int
> +virDomainCheckpointDiskDefFormat(virBufferPtr buf,
> + virDomainCheckpointDiskDefPtr disk,
> + unsigned int flags)
> +{
> + if (!disk->name)
> + return 0;
> +
> + virBufferEscapeString(buf, "<disk name='%s'",
disk->name);
> + if (disk->type)
> + virBufferAsprintf(buf, " checkpoint='%s'",
> + virDomainCheckpointTypeToString(disk->type));
> + if (disk->bitmap) {
> + virBufferEscapeString(buf, " bitmap='%s'",
disk->bitmap);
> + if (flags & VIR_DOMAIN_CHECKPOINT_FORMAT_SIZE)
> + virBufferAsprintf(buf, " size='%llu'",
disk->size);
The schema does not tie the size attribute to the presence of the
bitmap name.
I can try to tweak that.
> + }
> + virBufferAddLit(buf, "/>\n");
> + return 0;
> +}
[...]
ACK in general, most of the comments are not problems, if they'll be
addressed later. Note that since this patch introduces a big pile of
old-style code and technical debt it's more than desired that you
address it at some time.
I just re-compared this against snapshot_conf.c, and didn't see obvious
differences where snapshots had moved on to more modern code. Yes,
there's probably things that can be modernized (and my v10 will have a
few more VIR_AUTOFREE), but I don't want to delay this any longer than
necessary. Thanks for the close reviews so far.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org