On Mon, May 27, 2019 at 10:18:42 -0300, Maxiwell S. Garcia wrote:

The snapshot-create operation of running guests saves the live XML and uses it to replace the active and inactive domain in case of revert. So, the config XML is ignored by the snapshot process. This commit changes it and adds the config XML in the snapshot XML as the <inactive/domain> entry.

In case of offline guest, the behavior remains the same and the config XML is saved in the snapshot XML as <domain> entry. The behavior of older snapshots of running guests, that don't have the new <inactive/domain>, remains the same too. The revert, in this case, overrides both active and inactive domain with the <domain> entry. So, the <inactive/domain> in the snapshot XML is not required to snapshot work, but it's useful to preserve the config XML of running guests.

Signed-off-by: Maxiwell S. Garcia <maxiwell@linux.ibm.com> --- src/conf/moment_conf.c | 1 + src/conf/moment_conf.h | 1 + src/conf/snapshot_conf.c | 42 ++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_driver.c | 29 ++++++++++++++++++++++----- 4 files changed, 68 insertions(+), 5 deletions(-)

[...]

diff --git a/src/conf/moment_conf.h b/src/conf/moment_conf.h index 00ec1c1904..b8bec7e456 100644 --- a/src/conf/moment_conf.h +++ b/src/conf/moment_conf.h @@ -38,6 +38,7 @@ struct _virDomainMomentDef { long long creationTime; /* in seconds */

virDomainDefPtr dom; + virDomainDefPtr inactiveDom;

I'm slightly worried that this will cause confusion as 'inactiveDom' will be empty if the VM was offline during the snapshot.

diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index c7f29360e7..c66ee997a4 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -294,6 +294,28 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, } else { VIR_WARN("parsing older snapshot that lacks domain"); } + + /* /inactive/domain entry saves the config XML present in a running + * VM. In case of absent, leave parent.inactiveDom NULL and use + * parent.dom for config and live XML. */ + if ((tmp = virXPathString("string(./inactive/domain/@type)", ctxt))) {

In this case I'd prefer if the we don't nest it one level deeper. While the parser is ready for that, the formatter will probably need some fixing first.

+ int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;

The same flags are defined in the block parsing the (active) <domain>. Please move them out so that there's only one copy.

+ xmlNodePtr domainNode = virXPathNode("./inactive/domain", ctxt);

'tmp' is never used. What's the point of getting it? I think you want to use the 'domainNode' to check whether it exists in the first place.

+ + VIR_FREE(tmp); + if (!domainNode) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing inactive domain in snapshot")); + + goto cleanup;

This check then does not make any sense.

+ } + def->parent.inactiveDom = virDomainDefParseNode(ctxt->node->doc, domainNode, + caps, xmlopt, NULL, domainflags); + if (!def->parent.inactiveDom) + goto cleanup; + } + } else if (virDomainXMLOptionRunMomentPostParse(xmlopt, &def->parent) < 0) { goto cleanup; } @@ -511,6 +533,16 @@ virDomainSnapshotRedefineValidate(virDomainSnapshotDefPtr def, VIR_STEAL_PTR(def->parent.dom, otherdef->parent.dom); } } + if (otherdef->parent.inactiveDom) { + if (def->parent.inactiveDom) { + if (!virDomainDefCheckABIStability(otherdef->parent.inactiveDom, + def->parent.inactiveDom, xmlopt))

Checking ABI stability for a non-active VM definition does not seem to be necessary. Could you elaborate why it's needed?

+ return -1; + } else { + /* Transfer the inactive domain def */ + VIR_STEAL_PTR(def->parent.inactiveDom, otherdef->parent.inactiveDom); + } + }

I'm not persuaded that this logic is correct or necessary. Additionally virDomainSnapshotRedefinePrep which calls the above function then reverts the orignal def under some conditions, which was not copied in this patch. Since the function is meant to validate, I don't really think it should move or modify anything.

@@ -876,6 +908,16 @@ virDomainSnapshotDefFormatInternal(virBufferPtr buf, virBufferAddLit(buf, "</domain>\n"); }

+ if (def->parent.inactiveDom) { + virBufferAddLit(buf, "<inactive>\n"); + virBufferAdjustIndent(buf, 2); + if (virDomainDefFormatInternal(def->parent.inactiveDom, caps, + domainflags, buf, xmlopt) < 0)

It's unfortunate that this formats also <domain>.

+ goto error; + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</inactive>\n"); + } + if (virSaveCookieFormatBuf(buf, def->cookie, virDomainXMLOptionGetSaveCookie(xmlopt)) < 0) goto error; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 42b1ce2521..1ce7aa7b42 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15707,6 +15707,15 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) goto endjob;

+ if (vm->newDef) { + if (!(xml = qemuDomainDefFormatLive(driver, vm->newDef, priv->origCPU, + true, true)) || + !(def->parent.inactiveDom = virDomainDefParseString(xml, caps, driver->xmlopt, NULL, + VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)))

This seems to be reimplementing virDomainDefCopy.

+ goto endjob; + } + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; align_match = false;

[...]

@@ -16341,17 +16351,22 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, * in the failure cases where we know there was no change? */ }

- /* Prepare to copy the snapshot inactive xml as the config of this - * domain. - * - * XXX Should domain snapshots track live xml rather - * than inactive xml? */ + /* Prepare to copy the snapshot inactive domain as the config XML + * and the snapshot domain as the live XML. In case of inactive domain + * NULL, both config and live XML will be copied from snapshot domain. + */ if (snap->def->dom) { config = virDomainDefCopy(snap->def->dom, caps, driver->xmlopt, NULL, true); if (!config) goto endjob; } + if (snap->def->inactiveDom) { + inactiveConfig = virDomainDefCopy(snap->def->inactiveDom, caps, + driver->xmlopt, NULL, true);

... here you use it.

+ if (!inactiveConfig) + goto endjob; + }

Other than the above it looks okay to me and makes sense to store this along with the normal vm config. I think the 'confusion' aspect mentioned above can be overlooked but should probably be documented (similarly to how we got used to vm->def and vm->newDef). The comment about not nesting the XML elements differently is based on our use of a parsing limit on the depth of nesting of XML we use which could cause problems if the XML was at the limit of nesting. I don't think it should be too hard to modify/extract virDomainDefFormatInternal so that it does not format the top level elements. (virXMLFormatElement should make it relatively easy)