[libvirt] [PATCH v2] snapshot: Store both config and live XML in the snapshot domain
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> --- src/conf/moment_conf.c | 1 + src/conf/moment_conf.h | 11 +++++++++ src/conf/snapshot_conf.c | 48 +++++++++++++++++++++++++++++++++++----- src/qemu/qemu_driver.c | 27 +++++++++++++++++----- src/util/virxml.c | 45 +++++++++++++++++++++++++++++++++++++ src/util/virxml.h | 8 +++++++ 6 files changed, 130 insertions(+), 10 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 324901a560..8aeac9ab20 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -243,6 +243,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; @@ -292,8 +294,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); @@ -309,6 +309,20 @@ 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 (virXPathString("string(./inactiveDomain/@type)", ctxt)) { + xmlNodePtr domainNode = virXPathNode("./inactiveDomain", ctxt); + + if (domainNode) { + 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; } @@ -845,6 +859,10 @@ virDomainSnapshotDefFormatInternal(virBufferPtr buf, { size_t i; int domainflags = VIR_DOMAIN_DEF_FORMAT_INACTIVE; + virBuffer inactivedom_buf = VIR_BUFFER_INITIALIZER; + xmlXPathContextPtr inactivedom_ctxt = NULL; + char *inactivedom_str = NULL; + int ret = -1; if (flags & VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE) domainflags |= VIR_DOMAIN_DEF_FORMAT_SECURE; @@ -903,6 +921,20 @@ virDomainSnapshotDefFormatInternal(virBufferPtr buf, virBufferAddLit(buf, "</domain>\n"); } + if (def->parent.inactiveDom) { + if (virDomainDefFormatInternal(def->parent.inactiveDom, caps, + domainflags, &inactivedom_buf, xmlopt) < 0) + goto error; + + inactivedom_ctxt = virXPathBuildContext(&inactivedom_buf); + if (!(inactivedom_str = virXPathRenameNode("/domain", "inactiveDomain", + inactivedom_ctxt))) + goto error; + + virBufferAddStr(buf, inactivedom_str); + virBufferAddLit(buf, "\n"); + } + if (virSaveCookieFormatBuf(buf, def->cookie, virDomainXMLOptionGetSaveCookie(xmlopt)) < 0) goto error; @@ -917,11 +949,17 @@ virDomainSnapshotDefFormatInternal(virBufferPtr buf, if (virBufferCheckError(buf) < 0) goto error; - return 0; + ret = 0; error: - virBufferFreeAndReset(buf); - return -1; + VIR_FREE(inactivedom_str); + xmlXPathFreeContext(inactivedom_ctxt); + virBufferFreeAndReset(&inactivedom_buf); + + if (ret < 0) + virBufferFreeAndReset(buf); + + return ret; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 482f915b67..9b95e9b766 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15697,6 +15697,13 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) goto endjob; + if (vm->newDef) { + def->parent.inactiveDom = virDomainDefCopy(vm->newDef, caps, + driver->xmlopt, NULL, 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; @@ -16231,6 +16238,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, qemuDomainObjPrivatePtr priv; int rc; virDomainDefPtr config = NULL; + virDomainDefPtr inactiveConfig = NULL; virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; bool was_stopped = false; @@ -16331,17 +16339,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); + if (!inactiveConfig) + goto endjob; + } cookie = (qemuDomainSaveCookiePtr) snapdef->cookie; @@ -16592,6 +16605,10 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto endjob; } + if (inactiveConfig) { + virDomainDefFree(vm->newDef); + VIR_STEAL_PTR(vm->newDef, inactiveConfig); + } ret = 0; endjob: diff --git a/src/util/virxml.c b/src/util/virxml.c index f55b9a362c..756c0eedbc 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -1408,3 +1408,48 @@ virXPathContextNodeRestore(virXPathContextNodeSavePtr save) save->ctxt->node = save->node; } + + +/** + * virXPathBuildContext: convert an parent buffer to an + * XPath context ptr. The caller has to free the ptr. + */ +xmlXPathContextPtr +virXPathBuildContext(virBufferPtr root) +{ + xmlDocPtr doc; + + if (!root) + return NULL; + + doc = virXMLParseString(virBufferCurrentContent(root), NULL); + if (!doc) + return NULL; + + return xmlXPathNewContext(doc); +} + + +/** + * virXPathRenameNode: get the XML node using the 'xpath' and + * rename it with the 'newname' string. + * + * Returns the XML string of the node found by 'xpath' or NULL + * on error. The caller has to free the string. + */ +char * +virXPathRenameNode(const char *xpath, + const char *newname, + xmlXPathContextPtr ctxt) +{ + xmlNodePtr node; + + if (!xpath || !newname || !ctxt) + return NULL; + + if (!(node = virXPathNode(xpath, ctxt))) + return NULL; + + xmlNodeSetName(node, (xmlChar *) newname); + return virXMLNodeToString(node->doc, node); +} diff --git a/src/util/virxml.h b/src/util/virxml.h index 6208977dd1..48a507c3c1 100644 --- a/src/util/virxml.h +++ b/src/util/virxml.h @@ -220,6 +220,14 @@ virXMLFormatElement(virBufferPtr buf, virBufferPtr childBuf) ATTRIBUTE_RETURN_CHECK; +xmlXPathContextPtr +virXPathBuildContext(virBufferPtr root); + +char * +virXPathRenameNode(const char *xpath, + const char *newname, + xmlXPathContextPtr ctxt); + struct _virXPathContextNodeSave { xmlXPathContextPtr ctxt; xmlNodePtr node; -- 2.20.1
Max, Code looks ok. Two tests (virsh-checkpoint and virsh-snapshot) are failing, but they are also failing on master, thus I say this patch get a pass because it didn't break anything else. Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> On 7/23/19 4:47 PM, 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> --- src/conf/moment_conf.c | 1 + src/conf/moment_conf.h | 11 +++++++++ src/conf/snapshot_conf.c | 48 +++++++++++++++++++++++++++++++++++----- src/qemu/qemu_driver.c | 27 +++++++++++++++++----- src/util/virxml.c | 45 +++++++++++++++++++++++++++++++++++++ src/util/virxml.h | 8 +++++++ 6 files changed, 130 insertions(+), 10 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 324901a560..8aeac9ab20 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -243,6 +243,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; @@ -292,8 +294,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); @@ -309,6 +309,20 @@ 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 (virXPathString("string(./inactiveDomain/@type)", ctxt)) { + xmlNodePtr domainNode = virXPathNode("./inactiveDomain", ctxt); + + if (domainNode) { + 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; } @@ -845,6 +859,10 @@ virDomainSnapshotDefFormatInternal(virBufferPtr buf, { size_t i; int domainflags = VIR_DOMAIN_DEF_FORMAT_INACTIVE; + virBuffer inactivedom_buf = VIR_BUFFER_INITIALIZER; + xmlXPathContextPtr inactivedom_ctxt = NULL; + char *inactivedom_str = NULL; + int ret = -1;
if (flags & VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE) domainflags |= VIR_DOMAIN_DEF_FORMAT_SECURE; @@ -903,6 +921,20 @@ virDomainSnapshotDefFormatInternal(virBufferPtr buf, virBufferAddLit(buf, "</domain>\n"); }
+ if (def->parent.inactiveDom) { + if (virDomainDefFormatInternal(def->parent.inactiveDom, caps, + domainflags, &inactivedom_buf, xmlopt) < 0) + goto error; + + inactivedom_ctxt = virXPathBuildContext(&inactivedom_buf); + if (!(inactivedom_str = virXPathRenameNode("/domain", "inactiveDomain", + inactivedom_ctxt))) + goto error; + + virBufferAddStr(buf, inactivedom_str); + virBufferAddLit(buf, "\n"); + } + if (virSaveCookieFormatBuf(buf, def->cookie, virDomainXMLOptionGetSaveCookie(xmlopt)) < 0) goto error; @@ -917,11 +949,17 @@ virDomainSnapshotDefFormatInternal(virBufferPtr buf, if (virBufferCheckError(buf) < 0) goto error;
- return 0; + ret = 0;
error: - virBufferFreeAndReset(buf); - return -1; + VIR_FREE(inactivedom_str); + xmlXPathFreeContext(inactivedom_ctxt); + virBufferFreeAndReset(&inactivedom_buf); + + if (ret < 0) + virBufferFreeAndReset(buf); + + return ret; }
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 482f915b67..9b95e9b766 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15697,6 +15697,13 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) goto endjob;
+ if (vm->newDef) { + def->parent.inactiveDom = virDomainDefCopy(vm->newDef, caps, + driver->xmlopt, NULL, 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; @@ -16231,6 +16238,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, qemuDomainObjPrivatePtr priv; int rc; virDomainDefPtr config = NULL; + virDomainDefPtr inactiveConfig = NULL; virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; bool was_stopped = false; @@ -16331,17 +16339,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); + if (!inactiveConfig) + goto endjob; + }
cookie = (qemuDomainSaveCookiePtr) snapdef->cookie;
@@ -16592,6 +16605,10 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto endjob; }
+ if (inactiveConfig) { + virDomainDefFree(vm->newDef); + VIR_STEAL_PTR(vm->newDef, inactiveConfig); + } ret = 0;
endjob: diff --git a/src/util/virxml.c b/src/util/virxml.c index f55b9a362c..756c0eedbc 100644 --- a/src/util/virxml.c +++ b/src/util/virxml.c @@ -1408,3 +1408,48 @@ virXPathContextNodeRestore(virXPathContextNodeSavePtr save)
save->ctxt->node = save->node; } + + +/** + * virXPathBuildContext: convert an parent buffer to an + * XPath context ptr. The caller has to free the ptr. + */ +xmlXPathContextPtr +virXPathBuildContext(virBufferPtr root) +{ + xmlDocPtr doc; + + if (!root) + return NULL; + + doc = virXMLParseString(virBufferCurrentContent(root), NULL); + if (!doc) + return NULL; + + return xmlXPathNewContext(doc); +} + + +/** + * virXPathRenameNode: get the XML node using the 'xpath' and + * rename it with the 'newname' string. + * + * Returns the XML string of the node found by 'xpath' or NULL + * on error. The caller has to free the string. + */ +char * +virXPathRenameNode(const char *xpath, + const char *newname, + xmlXPathContextPtr ctxt) +{ + xmlNodePtr node; + + if (!xpath || !newname || !ctxt) + return NULL; + + if (!(node = virXPathNode(xpath, ctxt))) + return NULL; + + xmlNodeSetName(node, (xmlChar *) newname); + return virXMLNodeToString(node->doc, node); +} diff --git a/src/util/virxml.h b/src/util/virxml.h index 6208977dd1..48a507c3c1 100644 --- a/src/util/virxml.h +++ b/src/util/virxml.h @@ -220,6 +220,14 @@ virXMLFormatElement(virBufferPtr buf, virBufferPtr childBuf) ATTRIBUTE_RETURN_CHECK;
+xmlXPathContextPtr +virXPathBuildContext(virBufferPtr root); + +char * +virXPathRenameNode(const char *xpath, + const char *newname, + xmlXPathContextPtr ctxt); + struct _virXPathContextNodeSave { xmlXPathContextPtr ctxt; xmlNodePtr node;
On 7/31/19 2:45 PM, Daniel Henrique Barboza wrote:
Max,
Code looks ok. Two tests (virsh-checkpoint and virsh-snapshot) are failing, but they are also failing on master, thus I say this patch get a pass because it didn't break anything else.
What failures are you seeing? Those were just recently added, and if they are failing for you, it's worth getting them fixed. But I'm not seeing them fail on my end.
On 7/23/19 4:47 PM, 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.
Since checkpoints are brand new, and also created always on a running image, should they also gain an <inactiveDomain> entry? And if we are fast enough, would it be worth mandating that entry on a checkpoint REDEFINE (even though we can't do it for a snapshot REDEFINE)? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Eric, On 7/31/19 4:52 PM, Eric Blake wrote:
Max,
Code looks ok. Two tests (virsh-checkpoint and virsh-snapshot) are failing, but they are also failing on master, thus I say this patch get a pass because it didn't break anything else. What failures are you seeing? Those were just recently added, and if
On 7/31/19 2:45 PM, Daniel Henrique Barboza wrote: they are failing for you, it's worth getting them fixed. But I'm not seeing them fail on my end.
I ended up sending a patch to explain why the patch was breaking in my machine and propose a fix for it. Thanks, DHB
On 7/23/19 4:47 PM, 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. Since checkpoints are brand new, and also created always on a running image, should they also gain an <inactiveDomain> entry? And if we are fast enough, would it be worth mandating that entry on a checkpoint REDEFINE (even though we can't do it for a snapshot REDEFINE)?
On Wed, Jul 31, 2019 at 02:52:17PM -0500, Eric Blake wrote:
On 7/31/19 2:45 PM, Daniel Henrique Barboza wrote:
Max,
Code looks ok. Two tests (virsh-checkpoint and virsh-snapshot) are failing, but they are also failing on master, thus I say this patch get a pass because it didn't break anything else.
What failures are you seeing? Those were just recently added, and if they are failing for you, it's worth getting them fixed. But I'm not seeing them fail on my end.
On 7/23/19 4:47 PM, 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.
Since checkpoints are brand new, and also created always on a running image, should they also gain an <inactiveDomain> entry? And if we are fast enough, would it be worth mandating that entry on a checkpoint REDEFINE (even though we can't do it for a snapshot REDEFINE)?
Hi Eric, IIUC, checkpoints do not have commands that override the config XML. So, I think that <inactiveDomain>, in this case, is not necessary. What do you think?
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Jul 31, 2019 at 14:52:17 -0500, Eric Blake wrote:
On 7/31/19 2:45 PM, Daniel Henrique Barboza wrote:
Max,
Code looks ok. Two tests (virsh-checkpoint and virsh-snapshot) are failing, but they are also failing on master, thus I say this patch get a pass because it didn't break anything else.
What failures are you seeing? Those were just recently added, and if they are failing for you, it's worth getting them fixed. But I'm not seeing them fail on my end.
On 7/23/19 4:47 PM, 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.
Since checkpoints are brand new, and also created always on a running image, should they also gain an <inactiveDomain> entry? And if we are fast enough, would it be worth mandating that entry on a checkpoint REDEFINE (even though we can't do it for a snapshot REDEFINE)?
Can you actually revert to a checkpoint? I don't think so, which means there's no reason for storing the inactive XML for it. Jirka
On 8/9/19 9:02 AM, Jiri Denemark wrote:
On Wed, Jul 31, 2019 at 14:52:17 -0500, Eric Blake wrote:
On 7/31/19 2:45 PM, Daniel Henrique Barboza wrote:
Max,
Code looks ok. Two tests (virsh-checkpoint and virsh-snapshot) are failing, but they are also failing on master, thus I say this patch get a pass because it didn't break anything else.
What failures are you seeing? Those were just recently added, and if they are failing for you, it's worth getting them fixed. But I'm not seeing them fail on my end.
On 7/23/19 4:47 PM, 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.
Since checkpoints are brand new, and also created always on a running image, should they also gain an <inactiveDomain> entry? And if we are fast enough, would it be worth mandating that entry on a checkpoint REDEFINE (even though we can't do it for a snapshot REDEFINE)?
Can you actually revert to a checkpoint? I don't think so, which means there's no reason for storing the inactive XML for it.
At present, no. But down the road, when you revert a snapshot that was created coincedentally with a checkpoint, we may need to revert to that checkpoint as part of reverting to the snapshot. But then again, the snapshot's inactive XML would be sufficient. Okay, I'm leaning towards not needing an inactiveDomain in checkpoints. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Tue, Jul 23, 2019 at 16:47:05 -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 <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> --- src/conf/moment_conf.c | 1 + src/conf/moment_conf.h | 11 +++++++++ src/conf/snapshot_conf.c | 48 +++++++++++++++++++++++++++++++++++----- src/qemu/qemu_driver.c | 27 +++++++++++++++++----- src/util/virxml.c | 45 +++++++++++++++++++++++++++++++++++++ src/util/virxml.h | 8 +++++++ 6 files changed, 130 insertions(+), 10 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 324901a560..8aeac9ab20 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -243,6 +243,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; @@ -292,8 +294,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); @@ -309,6 +309,20 @@ 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 (virXPathString("string(./inactiveDomain/@type)", ctxt)) {
This would leak memory allocated by virXPathString.
+ xmlNodePtr domainNode = virXPathNode("./inactiveDomain", ctxt); + + if (domainNode) {
This doesn't make sense at all. How could it be NULL if you were able to get its attribute? To solve both issues and also make the code simple, you can just do if ((domainNode = virXPathNode("./inactiveDomain", ctxt))) {
+ 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; } @@ -845,6 +859,10 @@ virDomainSnapshotDefFormatInternal(virBufferPtr buf, { size_t i; int domainflags = VIR_DOMAIN_DEF_FORMAT_INACTIVE; + virBuffer inactivedom_buf = VIR_BUFFER_INITIALIZER; + xmlXPathContextPtr inactivedom_ctxt = NULL; + char *inactivedom_str = NULL; + int ret = -1;
if (flags & VIR_DOMAIN_SNAPSHOT_FORMAT_SECURE) domainflags |= VIR_DOMAIN_DEF_FORMAT_SECURE; @@ -903,6 +921,20 @@ virDomainSnapshotDefFormatInternal(virBufferPtr buf, virBufferAddLit(buf, "</domain>\n"); }
+ if (def->parent.inactiveDom) { + if (virDomainDefFormatInternal(def->parent.inactiveDom, caps, + domainflags, &inactivedom_buf, xmlopt) < 0) + goto error; + + inactivedom_ctxt = virXPathBuildContext(&inactivedom_buf); + if (!(inactivedom_str = virXPathRenameNode("/domain", "inactiveDomain", + inactivedom_ctxt))) + goto error;
So you format the domain XML, parse it back, rename its root element, and format it again? Please don't do this. The XML should be formatted with the right root element right away. And any refactoring allowing you to do this should be in a separate patch.
+ + virBufferAddStr(buf, inactivedom_str); + virBufferAddLit(buf, "\n"); + } + if (virSaveCookieFormatBuf(buf, def->cookie, virDomainXMLOptionGetSaveCookie(xmlopt)) < 0) goto error; @@ -917,11 +949,17 @@ virDomainSnapshotDefFormatInternal(virBufferPtr buf, if (virBufferCheckError(buf) < 0) goto error;
- return 0; + ret = 0;
error: - virBufferFreeAndReset(buf); - return -1; + VIR_FREE(inactivedom_str); + xmlXPathFreeContext(inactivedom_ctxt); + virBufferFreeAndReset(&inactivedom_buf); + + if (ret < 0) + virBufferFreeAndReset(buf); + + return ret; }
This hunk should not be needed at all if you format the XML properly.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 482f915b67..9b95e9b766 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -15697,6 +15697,13 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain, VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE))) goto endjob;
+ if (vm->newDef) { + def->parent.inactiveDom = virDomainDefCopy(vm->newDef, caps, + driver->xmlopt, NULL, true);
s/NULL/priv->qemuCaps/ otherwise the event look may end up in a deadlock.
+ if (!def->parent.inactiveDom) + goto endjob; + } + if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) { align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL; align_match = false; @@ -16231,6 +16238,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, qemuDomainObjPrivatePtr priv; int rc; virDomainDefPtr config = NULL; + virDomainDefPtr inactiveConfig = NULL; virQEMUDriverConfigPtr cfg = NULL; virCapsPtr caps = NULL; bool was_stopped = false; @@ -16331,17 +16339,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);
You'll get a conflict here because of recent s/NULL/priv->qemuCaps/
if (!config) goto endjob; } + if (snap->def->inactiveDom) { + inactiveConfig = virDomainDefCopy(snap->def->inactiveDom, caps, + driver->xmlopt, NULL, true);
s/NULL/priv->qemuCaps/ again.
+ if (!inactiveConfig) + goto endjob; + }
cookie = (qemuDomainSaveCookiePtr) snapdef->cookie;
@@ -16592,6 +16605,10 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot, goto endjob; }
+ if (inactiveConfig) { + virDomainDefFree(vm->newDef); + VIR_STEAL_PTR(vm->newDef, inactiveConfig); + }
I think you should be able to use virDomainObjAssignDef here. And even though it will result in code duplication, I think it should be done within individual switch branches where appropriate before we send lifecycle events. And yes, I know qemuDomainRevertToSnapshot is one of the worst pieces of code we have in libvirt.
ret = 0;
endjob:
Please drop any changes to virxml.[ch]. Jirka
participants (4)
-
Daniel Henrique Barboza -
Eric Blake -
Jiri Denemark -
Maxiwell S. Garcia