We can extract './domain' directly and let the parser deal with the type. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/checkpoint_conf.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c index a8d18928de..33b6699be7 100644 --- a/src/conf/checkpoint_conf.c +++ b/src/conf/checkpoint_conf.c @@ -126,7 +126,6 @@ virDomainCheckpointDefParse(xmlXPathContextPtr ctxt, virDomainCheckpointDefPtr ret = NULL; size_t i; int n; - char *tmp; g_autofree xmlNodePtr *nodes = NULL; g_autoptr(virDomainCheckpointDef) def = NULL; @@ -146,6 +145,8 @@ virDomainCheckpointDefParse(xmlXPathContextPtr ctxt, def->parent.description = virXPathString("string(./description)", ctxt); if (flags & VIR_DOMAIN_CHECKPOINT_PARSE_REDEFINE) { + xmlNodePtr domainNode; + if (virXPathLongLong("string(./creationTime)", ctxt, &def->parent.creationTime) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -155,17 +156,10 @@ virDomainCheckpointDefParse(xmlXPathContextPtr ctxt, def->parent.parent_name = virXPathString("string(./parent/name)", ctxt); - if ((tmp = virXPathString("string(./domain/@type)", ctxt))) { + if ((domainNode = virXPathNode("./domain", 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, xmlopt, parseOpaque, domainflags); -- 2.28.0

Fix the type for a variable holding flags to the usual 'unsigned int' and change the name to be more appropriate to it's use. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/checkpoint_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c index 33b6699be7..8744ac83e1 100644 --- a/src/conf/checkpoint_conf.c +++ b/src/conf/checkpoint_conf.c @@ -157,12 +157,12 @@ virDomainCheckpointDefParse(xmlXPathContextPtr ctxt, def->parent.parent_name = virXPathString("string(./parent/name)", ctxt); if ((domainNode = virXPathNode("./domain", ctxt))) { - int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE | - VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE; + unsigned int domainParseFlags = VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE; def->parent.dom = virDomainDefParseNode(ctxt->node->doc, domainNode, xmlopt, parseOpaque, - domainflags); + domainParseFlags); if (!def->parent.dom) return NULL; } else { -- 2.28.0

Checking the definition ABI when redefining checkpoints doesn't make much sense for the following reasons: * the domain definition in the checkpoint is mostly unused (a relic adopted from the snapshot code) * can be very easily overriden by deleting the checkpoint metadata before redefinition Rather than complicating the logic when we'll be taking into account that the domain definition may be missing, let's just remove the check. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/checkpoint_conf.c | 7 +------ src/conf/checkpoint_conf.h | 3 +-- src/qemu/qemu_checkpoint.c | 7 +++---- src/test/test_driver.c | 2 +- 4 files changed, 6 insertions(+), 13 deletions(-) diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c index 8744ac83e1..73fdb78e7a 100644 --- a/src/conf/checkpoint_conf.c +++ b/src/conf/checkpoint_conf.c @@ -545,8 +545,7 @@ virDomainCheckpointRedefinePrep(virDomainObjPtr vm, virDomainMomentObjPtr virDomainCheckpointRedefineCommit(virDomainObjPtr vm, - virDomainCheckpointDefPtr *defptr, - virDomainXMLOptionPtr xmlopt) + virDomainCheckpointDefPtr *defptr) { virDomainCheckpointDefPtr def = *defptr; virDomainMomentObjPtr other = NULL; @@ -556,10 +555,6 @@ virDomainCheckpointRedefineCommit(virDomainObjPtr vm, other = virDomainCheckpointFindByName(vm->checkpoints, def->parent.name); if (other) { otherdef = virDomainCheckpointObjGetDef(other); - if (!virDomainDefCheckABIStability(otherdef->parent.dom, - def->parent.dom, xmlopt)) - return NULL; - /* Drop and rebuild the parent relationship, but keep all * child relations by reusing chk. */ virDomainMomentDropParent(other); diff --git a/src/conf/checkpoint_conf.h b/src/conf/checkpoint_conf.h index 631f863151..4508f61b29 100644 --- a/src/conf/checkpoint_conf.h +++ b/src/conf/checkpoint_conf.h @@ -97,7 +97,6 @@ virDomainCheckpointRedefinePrep(virDomainObjPtr vm, virDomainMomentObjPtr virDomainCheckpointRedefineCommit(virDomainObjPtr vm, - virDomainCheckpointDefPtr *defptr, - virDomainXMLOptionPtr xmlopt); + virDomainCheckpointDefPtr *defptr); VIR_ENUM_DECL(virDomainCheckpoint); diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index e8d18b2e02..1740cadabf 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -439,8 +439,7 @@ qemuCheckpointRedefineValidateBitmaps(virDomainObjPtr vm, static virDomainMomentObjPtr -qemuCheckpointRedefine(virQEMUDriverPtr driver, - virDomainObjPtr vm, +qemuCheckpointRedefine(virDomainObjPtr vm, virDomainCheckpointDefPtr *def, bool *update_current, bool validate_bitmaps) @@ -452,7 +451,7 @@ qemuCheckpointRedefine(virQEMUDriverPtr driver, qemuCheckpointRedefineValidateBitmaps(vm, *def) < 0) return NULL; - return virDomainCheckpointRedefineCommit(vm, def, driver->xmlopt); + return virDomainCheckpointRedefineCommit(vm, def); } @@ -605,7 +604,7 @@ qemuCheckpointCreateXML(virDomainPtr domain, return NULL; if (redefine) { - chk = qemuCheckpointRedefine(driver, vm, &def, &update_current, validate_bitmaps); + chk = qemuCheckpointRedefine(vm, &def, &update_current, validate_bitmaps); } else { chk = qemuCheckpointCreate(driver, vm, &def); } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 136269de3d..29c4c86b1d 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -8991,7 +8991,7 @@ testDomainCheckpointCreateXML(virDomainPtr domain, if (virDomainCheckpointRedefinePrep(vm, def, &update_current) < 0) goto cleanup; - if (!(chk = virDomainCheckpointRedefineCommit(vm, &def, privconn->xmlopt))) + if (!(chk = virDomainCheckpointRedefineCommit(vm, &def))) goto cleanup; } else { if (!(def->parent.dom = virDomainDefCopy(vm->def, -- 2.28.0

Coniditonalize code which assumes that the domain definition stored in the checkpoint is present. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/checkpoint_conf.c | 31 +++++++++++++------------ tests/qemudomaincheckpointxml2xmltest.c | 5 ---- 2 files changed, 16 insertions(+), 20 deletions(-) diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c index 73fdb78e7a..2071494d52 100644 --- a/src/conf/checkpoint_conf.c +++ b/src/conf/checkpoint_conf.c @@ -475,10 +475,10 @@ virDomainCheckpointDefFormatInternal(virBufferPtr buf, virBufferAddLit(buf, "</disks>\n"); } - if (!(flags & VIR_DOMAIN_CHECKPOINT_FORMAT_NO_DOMAIN) && - virDomainDefFormatInternal(def->parent.dom, xmlopt, - buf, domainflags) < 0) - return -1; + if (def->parent.dom && !(flags & VIR_DOMAIN_CHECKPOINT_FORMAT_NO_DOMAIN)) { + if (virDomainDefFormatInternal(def->parent.dom, xmlopt, buf, domainflags) < 0) + return -1; + } virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</domaincheckpoint>\n"); @@ -510,23 +510,24 @@ virDomainCheckpointRedefinePrep(virDomainObjPtr vm, virDomainCheckpointDefPtr def, bool *update_current) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; virDomainMomentObjPtr parent = NULL; - virUUIDFormat(vm->def->uuid, uuidstr); - if (virDomainCheckpointCheckCycles(vm->checkpoints, def, vm->def->name) < 0) return -1; - if (!def->parent.dom || - memcmp(def->parent.dom->uuid, vm->def->uuid, VIR_UUID_BUFLEN)) { - virReportError(VIR_ERR_INVALID_ARG, - _("definition for checkpoint %s must use uuid %s"), - def->parent.name, uuidstr); - return -1; + if (def->parent.dom) { + if (memcmp(def->parent.dom->uuid, vm->def->uuid, VIR_UUID_BUFLEN)) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; + virUUIDFormat(vm->def->uuid, uuidstr); + virReportError(VIR_ERR_INVALID_ARG, + _("definition for checkpoint %s must use uuid %s"), + def->parent.name, uuidstr); + return -1; + } + + if (virDomainCheckpointAlignDisks(def) < 0) + return -1; } - if (virDomainCheckpointAlignDisks(def) < 0) - return -1; if (def->parent.parent_name && (parent = virDomainCheckpointFindByName(vm->checkpoints, diff --git a/tests/qemudomaincheckpointxml2xmltest.c b/tests/qemudomaincheckpointxml2xmltest.c index a5a5b59205..8b4b75d753 100644 --- a/tests/qemudomaincheckpointxml2xmltest.c +++ b/tests/qemudomaincheckpointxml2xmltest.c @@ -87,11 +87,6 @@ testCompareXMLToXMLFiles(const char *inxml, 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.xmlopt, formatflags))) -- 2.28.0

The domain definition stored with a checkpoint isn't used currently apart from matching disks when creating a new checkpoints. As some users of the incremental backup API want to provide backups in offline mode under their control (obviously while compying with our documentation on how the on-disk state should be handled) and then want to define the checkpoint for live use, supplying a <domain> sub-element is overly complex and not actually needed by the code. Relax the restriction when re-defining a checkpoint so that <domain> is not necessary and add (alibistic) documentation saying that future actions may not work if it's missing. Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- docs/formatcheckpoint.rst | 12 +++++++++--- src/conf/checkpoint_conf.c | 4 ---- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/docs/formatcheckpoint.rst b/docs/formatcheckpoint.rst index f159f2a7a3..ff3f1e8c00 100644 --- a/docs/formatcheckpoint.rst +++ b/docs/formatcheckpoint.rst @@ -1,3 +1,5 @@ +.. role:: since + Checkpoint XML format ===================== @@ -103,12 +105,16 @@ The top-level ``domaincheckpoint`` element may contain the following elements: A readonly representation of the inactive `domain configuration <formatdomain.html>`__ at the time the checkpoint was created. This element may be omitted for output brevity by supplying the - ``VIR_DOMAIN_CHECKPOINT_XML_NO_DOMAIN`` flag, but the resulting XML is no - longer viable for use with the ``VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE`` flag - of ``virDomainCheckpointCreateXML()``. The domain will have + ``VIR_DOMAIN_CHECKPOINT_XML_NO_DOMAIN`` flag. The domain will have security-sensitive information omitted unless the flag ``VIR_DOMAIN_CHECKPOINT_XML_SECURE`` is provided on a read-write connection. + ``virDomainCheckpointCreateXML()`` requires that the ``<domain>`` is present + when used with ``VIR_DOMAIN_CHECKPOINT_CREATE_REDEFINE``. + :since:`Since 7.0.0` the ``<domain>`` element can be omitted when redefining + a checkpoint, but hypervisors may not support certain operations if it's + missing. + Examples -------- diff --git a/src/conf/checkpoint_conf.c b/src/conf/checkpoint_conf.c index 2071494d52..2df5e41495 100644 --- a/src/conf/checkpoint_conf.c +++ b/src/conf/checkpoint_conf.c @@ -165,10 +165,6 @@ virDomainCheckpointDefParse(xmlXPathContextPtr ctxt, domainParseFlags); 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; -- 2.28.0