On Wed, Aug 14, 2019 at 11:47:21 -0300, Maxiwell S. Garcia wrote:

The function virDomainDefFormatInternal() has the predefined root name "domain" to format the XML. But to save both active and inactive domain in the snapshot XML, the new root name "inactiveDomain" was created. So, this function was modified to be driven by the new flag VIR_DOMAIN_DEF_FORMAT_INACTIVE_NODE to choose the correct root name.

Signed-off-by: Maxiwell S. Garcia <maxiwell@linux.ibm.com> --- src/conf/domain_conf.c | 13 ++++++++++--- src/conf/domain_conf.h | 1 + 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 03312afaaf..7d6393b9ac 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -28230,6 +28230,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, unsigned char *uuid; char uuidstr[VIR_UUID_STRING_BUFLEN]; const char *type = NULL; + const char *rootname = NULL; int n; size_t i; char *netprefix = NULL; @@ -28238,7 +28239,8 @@ virDomainDefFormatInternal(virDomainDefPtr def, VIR_DOMAIN_DEF_FORMAT_STATUS | VIR_DOMAIN_DEF_FORMAT_ACTUAL_NET | VIR_DOMAIN_DEF_FORMAT_PCI_ORIG_STATES | - VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST, + VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST | + VIR_DOMAIN_DEF_FORMAT_INACTIVE_NODE, -1);

if (!(type = virDomainVirtTypeToString(def->virtType))) { @@ -28250,7 +28252,12 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (def->id == -1) flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE;

- virBufferAsprintf(buf, "<domain type='%s'", type); + if (flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE_NODE) + rootname = "inactiveDomain"; + else + rootname = "domain";

This looks like an ugly hack. I suggest a bit different approach which involves creating a new function. Just add a new parameter for the root element name, rename this function, and create a new one with the original name which would be a tiny wrapper around the renamed function passing "domain" as the root element name. This way you can keep existing code untouched and just call the new function when you need a different root element name. In other places we sometimes separate formatter for the internals from the code formatting the container element. But since we need to add attributes to the container element, such approach would be quite ugly in this case. Jirka

On 8/14/19 11:47 AM, 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 <inactiveDomain> 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 <inactiveDomain>, remains the same too. The revert, in this case, overrides both active and inactive domain with the <domain> entry. So, the <inactiveDomain> 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> ---

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

src/conf/moment_conf.c | 1 + src/conf/moment_conf.h | 11 +++++++++++ src/conf/snapshot_conf.c | 23 +++++++++++++++++++++-- src/qemu/qemu_driver.c | 37 ++++++++++++++++++++++++++++--------- 4 files changed, 61 insertions(+), 11 deletions(-)

diff --git a/src/conf/moment_conf.c b/src/conf/moment_conf.c index fea13f0f97..f54a44b33e 100644 --- a/src/conf/moment_conf.c +++ b/src/conf/moment_conf.c @@ -66,6 +66,7 @@ virDomainMomentDefDispose(void *obj) VIR_FREE(def->description); VIR_FREE(def->parent_name); virDomainDefFree(def->dom); + virDomainDefFree(def->inactiveDom); }

/* Provide defaults for creation time and moment name after parsing XML */ diff --git a/src/conf/moment_conf.h b/src/conf/moment_conf.h index 9fdbef2172..70cc47bd70 100644 --- a/src/conf/moment_conf.h +++ b/src/conf/moment_conf.h @@ -36,7 +36,18 @@ struct _virDomainMomentDef { char *parent_name; long long creationTime; /* in seconds */

+ /* + * Store the active domain definition in case of online + * guest and the inactive domain definition in case of + * offline guest + */ virDomainDefPtr dom; + + /* + * Store the inactive domain definition in case of online + * guest and leave NULL in case of offline guest + */ + virDomainDefPtr inactiveDom; };

virClassPtr virClassForDomainMomentDef(void); diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 7996589ad2..36435043ca 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -235,6 +235,7 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, virDomainSnapshotDefPtr def = NULL; virDomainSnapshotDefPtr ret = NULL; xmlNodePtr *nodes = NULL; + xmlNodePtr inactiveDomNode = NULL; size_t i; int n; char *creation = NULL, *state = NULL; @@ -244,6 +245,8 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, char *memoryFile = NULL; bool offline = !!(flags & VIR_DOMAIN_SNAPSHOT_PARSE_OFFLINE); virSaveCookieCallbacksPtr saveCookie = virDomainXMLOptionGetSaveCookie(xmlopt); + int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE | + VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;

if (!(def = virDomainSnapshotDefNew())) return NULL; @@ -293,8 +296,6 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, * clients will have to decide between best effort * initialization or outright failure. */ 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); @@ -311,6 +312,16 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, } else { VIR_WARN("parsing older snapshot that lacks domain"); } + + /* /inactiveDomain 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 ((inactiveDomNode = virXPathNode("./inactiveDomain", ctxt))) { + def->parent.inactiveDom = virDomainDefParseNode(ctxt->node->doc, inactiveDomNode, + caps, xmlopt, NULL, domainflags); + if (!def->parent.inactiveDom) + goto cleanup; + } } else if (virDomainXMLOptionRunMomentPostParse(xmlopt, &def->parent) < 0) { goto cleanup; } @@ -405,6 +416,7 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt, VIR_FREE(nodes); VIR_FREE(memorySnapshot); VIR_FREE(memoryFile); + VIR_FREE(inactiveDomNode); virObjectUnref(def);

return ret; @@ -908,6 +920,13 @@ virDomainSnapshotDefFormatInternal(virBufferPtr buf, virBufferAddLit(buf, "</domain>\n"); }

+ if (def->parent.inactiveDom) { + int inactivedomainflags = domainflags | VIR_DOMAIN_DEF_FORMAT_INACTIVE_NODE; + if (virDomainDefFormatInternal(def->parent.inactiveDom, caps, + inactivedomainflags, buf, xmlopt) < 0) + goto error; + } + 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 48c7b5628b..b3edb40a80 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15911,6 +15911,13 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) goto endjob;

+ if (vm->newDef) { + def->parent.inactiveDom = virDomainDefCopy(vm->newDef, caps, + driver->xmlopt, priv->qemuCaps, true); + if (!def->parent.inactiveDom) + goto endjob; + } + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; align_match = false; @@ -16443,6 +16450,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, qemuDomainObjPrivatePtr priv; int rc; virDomainDefPtr config = NULL; + virDomainDefPtr inactiveConfig = NULL; virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; bool was_stopped = false; @@ -16544,11 +16552,10 @@ 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, priv->qemuCaps, true); @@ -16556,6 +16563,15 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto endjob; }

+ if (snap->def->inactiveDom) { + inactiveConfig = virDomainDefCopy(snap->def->inactiveDom, caps, + driver->xmlopt, priv->qemuCaps, true); + if (!inactiveConfig) + goto endjob; + } else { + inactiveConfig = config; + } + cookie = (qemuDomainSaveCookiePtr) snapdef->cookie;

switch ((virDomainSnapshotState) snapdef->state) { @@ -16658,16 +16674,19 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto endjob; } if (config) { - virDomainObjAssignDef(vm, config, false, NULL); virCPUDefFree(priv->origCPU); VIR_STEAL_PTR(priv->origCPU, origCPU); } + if (inactiveConfig) + virDomainObjAssignDef(vm, inactiveConfig, false, NULL); } else { /* Transitions 2, 3 */ load: was_stopped = true; + if (inactiveConfig) + virDomainObjAssignDef(vm, inactiveConfig, false, NULL); if (config) - virDomainObjAssignDef(vm, config, false, NULL); + virDomainObjAssignDef(vm, config, true, NULL);

/* No cookie means libvirt which saved the domain was too old to * mess up the CPU definitions. @@ -16753,8 +16772,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, qemuProcessEndJob(driver, vm); goto cleanup; } - if (config) - virDomainObjAssignDef(vm, config, false, NULL); + if (inactiveConfig) + virDomainObjAssignDef(vm, inactiveConfig, false, NULL);

if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING | VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) {