On 12/2/20 11:13 AM, Peter Krempa wrote:

We can extract './domain' directly and let the parser deal with the type.

Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

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);

On 12/2/20 11:13 AM, Peter Krempa wrote:

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.

s/it's/its Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- 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 {

On 12/2/20 11:13 AM, Peter Krempa wrote:

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

s/overriden/overridden

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@redhat.com> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

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,

On 12/2/20 11:13 AM, Peter Krempa wrote:

Coniditonalize code which assumes that the domain definition stored in

s/Coniditonalize/Conditionalize

the checkpoint is present.

Signed-off-by: Peter Krempa <pkrempa@redhat.com> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

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)))

On 12/2/20 11:13 AM, Peter Krempa wrote:

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

s/compying/complying

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@redhat.com> ---

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>

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;