3:51 a.m.
Restoring to a snapshot should not overwrite the persistent XML configuration
of a snapshot as a side effect. This patchset fixes the same. Currently,
virDomainSnapshotDef only saves active domain definition of the guest.
And on restore the active domain definition is used as both active and
inactive domain definitions. This will make the non-persistent changes
persistent in snapshot image. This patchset allows to save inactive domain
definition as well and on snapshot-revert non-persistent configuration is
restored as is.
Currently, snapshot-revert is making non-presistent changes as persistent.
Here are the steps to reproduce.
Step1: virsh define $dom
Step2: virsh attach-device $dom $memory-device.xml --live
Step3: virsh snapshot-create $dom
Step4: virsh destroy $dom
Step5: virsh snapshot-revert $dom $snapshot-name
Step6: virsh destroy $dom
Step7: virsh start $dom
Here we still have $memory-device attached in Step2.
This patchset is attempting to solve this issue. This patchset will also
allow user to dump and edit inactive XML configuration of a snapshot.
Dumping inactive domain definition of a snapshot is important as
--redefine uses snapshot-dumpxml output to redefine a snapshot.
Kothapally Madhu Pavan (7):
qemu: Store inactive domain configuration in snapshot
qemu: Use active and inactive snapshot configuration on restore
conf: Allow editing inactive snapshot configuration
virsh: Dump inactive XML configuration of snapshot using
snapshot-dumpxml
virsh: Edit inactive XML configuration of snapshot using snapshot-edit
virsh: Allow restoring snapshot with non-persistent configuration
tests: docs: Add schema and testcase for domainsnapshot
docs/schemas/domainsnapshot.rng | 19 +++++
include/libvirt/libvirt-domain-snapshot.h | 10 ++-
include/libvirt/libvirt-domain.h | 1 +
src/conf/domain_conf.c | 6 +-
src/conf/domain_conf.h | 2 +
src/conf/snapshot_conf.c | 48 ++++++++++++-
src/conf/snapshot_conf.h | 1 +
src/qemu/qemu_driver.c | 33 ++++++++-
.../full_domain_withinactive.xml | 83 ++++++++++++++++++++++
tests/domainsnapshotxml2xmltest.c | 1 +
tools/virsh-snapshot.c | 20 ++++++
tools/virsh.pod | 37 +++++++++-
12 files changed, 251 insertions(+), 10 deletions(-)
create mode 100644 tests/domainsnapshotxml2xmlout/full_domain_withinactive.xml
--
1.8.3.1
3:51 a.m.
New subject: [libvirt] [PATCH 1/7] qemu: Store inactive domain configuration in snapshot
Inorder to capture the exact state of domain, inactive configuration
is needed along with active configuration. This patch stores inactive
domain configuration when creating snapshot of a running domain. It
also captures the inactive snapshot configuration when a snapshot is
redefined.
Signed-off-by: Kothapally Madhu Pavan <kmp(a)linux.vnet.ibm.com>
---
src/conf/snapshot_conf.c | 13 +++++++++++++
src/conf/snapshot_conf.h | 1 +
src/qemu/qemu_driver.c | 10 ++++++++++
3 files changed, 24 insertions(+)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index f0e852c..bfe3d6c 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -102,6 +102,7 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def)
virDomainSnapshotDiskDefClear(&def->disks[i]);
VIR_FREE(def->disks);
virDomainDefFree(def->dom);
+ virDomainDefFree(def->newDom);
virObjectUnref(def->cookie);
VIR_FREE(def);
}
@@ -1336,6 +1337,18 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
}
}
+ if (other->def->newDom) {
+ if (def->newDom) {
+ if (!virDomainDefCheckABIStability(other->def->newDom,
+ def->newDom, xmlopt))
+ goto cleanup;
+ } else {
+ /* Transfer the inactive domain def */
+ def->newDom = other->def->newDom;
+ other->def->newDom = NULL;
+ }
+ }
+
if (other == vm->current_snapshot) {
*update_current = true;
vm->current_snapshot = NULL;
diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
index 1d663c7..0bc915f 100644
--- a/src/conf/snapshot_conf.h
+++ b/src/conf/snapshot_conf.h
@@ -75,6 +75,7 @@ struct _virDomainSnapshotDef {
virDomainSnapshotDiskDef *disks;
virDomainDefPtr dom;
+ virDomainDefPtr newDom;
virObjectPtr cookie;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 74fdfdb..4ffec70 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15035,6 +15035,16 @@ 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->newDom = virDomainDefParseString(xml, caps, driver->xmlopt,
NULL,
+ VIR_DOMAIN_DEF_PARSE_INACTIVE |
+
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)))
+ goto endjob;
+ }
+
+
if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) {
align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
align_match = false;
--
1.8.3.1
10:27 a.m.
New subject: [libvirt] [PATCH 1/7] qemu: Store inactive domain configuration in snapshot
On 10/30/2017 04:51 AM, Kothapally Madhu Pavan wrote:
Inorder to capture the exact state of domain, inactive configuration
In order
is needed along with active configuration. This patch stores
inactive
domain configuration when creating snapshot of a running domain. It
also captures the inactive snapshot configuration when a snapshot is
redefined.
Signed-off-by: Kothapally Madhu Pavan <kmp(a)linux.vnet.ibm.com>
---
src/conf/snapshot_conf.c | 13 +++++++++++++
src/conf/snapshot_conf.h | 1 +
src/qemu/qemu_driver.c | 10 ++++++++++
3 files changed, 24 insertions(+)
Caveat emptor - I'm not 100% familiar with all/any of the snapshot_conf
code and processing, but I'll look at the patches to provide some
feedback...
Editorial comment... Using "newDef" has always been a bit "confusing"
to
say the least. I know at one point there was a comment on list in some
review that we should rename it to something else to indicate what it
really is, but I have forgotten what the recommendation was. Hopefully,
someone else can remember!
Perhaps we can "avoid" continuing the confusion with this set of
changes. Even more hopefully, someone spends the time to change domain
newDef ;-)! That might even fix what I'm finding confusing about these
patches...
Looking at existing code prior to these changes and thinking about the
context of just these changes as they relate to def/newDef - I'm not
sure you're capturing "everything". For instance, I see code in
qemuDomainSnapshotDiskDataCollect makes specific decisions about which
disks to collect, but there's no adjustment in that code with this series.
There's a comment in qemuDomainRevertToSnapshot from a hunk of code just
above the !REVERT_ACTIVE_ONLY from patch 2:
"
/* Prepare to copy the snapshot inactive xml as the config of this
* domain.
*
* XXX Should domain snapshots track live xml rather
* than inactive xml? */
"
and from qemuDomainSnapshotCreateXML above where you add the newDef copy
in this patch:
"
/* Easiest way to clone inactive portion of vm->def is via
* conversion in and back out of xml. */
"
So what's confusing (to say the least) is - which "def" is really being
used. The code at times says inactive, but IIUC newDef is generally
treated as the persistent def, but that only matters if something has
caused it to be created. Of course, I assume this confusion is what
you're trying to fix based on the cover letter comments!
If the intention of these patches is to essentially "mimic" the domain
def/newDef functionality, then perhaps thinking in terms of something
like virDomainObjGetPersistentDef may be helpful to "hide" which is
which and when one applies over the other. (does that even make sense?)
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index f0e852c..bfe3d6c 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -102,6 +102,7 @@ void virDomainSnapshotDefFree(virDomainSnapshotDefPtr def)
virDomainSnapshotDiskDefClear(&def->disks[i]);
VIR_FREE(def->disks);
virDomainDefFree(def->dom);
+ virDomainDefFree(def->newDom);
virObjectUnref(def->cookie);
VIR_FREE(def);
}
@@ -1336,6 +1337,18 @@ virDomainSnapshotRedefinePrep(virDomainPtr domain,
}
}
+ if (other->def->newDom) {
+ if (def->newDom) {
+ if (!virDomainDefCheckABIStability(other->def->newDom,
+ def->newDom, xmlopt))
+ goto cleanup;
+ } else {
+ /* Transfer the inactive domain def */
+ def->newDom = other->def->newDom;
+ other->def->newDom = NULL;
/* Steal the inactive domain def */
VIR_STEAL_PTR(def->newDom, other->def->newDom);
[yes, I realize this is just stealing the code a few lines above, but
the more recent desire is to use the macro]
+ }
+ }
+
if (other == vm->current_snapshot) {
*update_current = true;
vm->current_snapshot = NULL;
diff --git a/src/conf/snapshot_conf.h b/src/conf/snapshot_conf.h
index 1d663c7..0bc915f 100644
--- a/src/conf/snapshot_conf.h
+++ b/src/conf/snapshot_conf.h
@@ -75,6 +75,7 @@ struct _virDomainSnapshotDef {
virDomainSnapshotDiskDef *disks;
virDomainDefPtr dom;
+ virDomainDefPtr newDom;
virObjectPtr cookie;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 74fdfdb..4ffec70 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15035,6 +15035,16 @@ qemuDomainSnapshotCreateXML(virDomainPtr domain,
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)))
goto endjob;
IIUC, the above hunk is designed to grab from vm->def anything that was
felt to be in the domain def making it as if it was some sort of
inactive definition. But since it's copying the live definition it
perhaps copies portions of the live definition that have changed, such
as via hotplug. That leaves a couple of questions in my mind related to
whether restoration from a snapshot would/should "restore" that hotplug
element especially if it doesn't apply/exist any more.
The subsequent hunk essentially copies the persistent definition, so
does "PARSE_INACTIVE" make sense?
John
BTW: I'll respond individually to other patches, but it may take a few
hours as I have some errands to run today - so I'll be "in and out" -
figured it was better to get this out there first though.
+ if (vm->newDef) {
+ if (!(xml = qemuDomainDefFormatLive(driver, vm->newDef,
priv->origCPU,
+ true, true)) ||
+ !(def->newDom = virDomainDefParseString(xml, caps, driver->xmlopt,
NULL,
+ VIR_DOMAIN_DEF_PARSE_INACTIVE |
+
VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE)))
+ goto endjob;
+ }
+
+
if (flags & VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY) {
align_location = VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL;
align_match = false;
3:51 a.m.
New subject: [libvirt] [PATCH 2/7] qemu: Use active and inactive snapshot configuration on restore
By default, active and inactive XMl snapshot configurations are
assigned to domain definition. This will make sure that all the
non-persistent configurations of the snapshot are restored back
as it is. This patch will also make sure that user has a choice
to choose of using active XML configuration of snapshot as both
active and inactive XML configurations of the restoring domain.
Signed-off-by: Kothapally Madhu Pavan <kmp(a)linux.vnet.ibm.com>
---
include/libvirt/libvirt-domain-snapshot.h | 10 +++++++---
src/qemu/qemu_driver.c | 20 +++++++++++++++++++-
2 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/include/libvirt/libvirt-domain-snapshot.h
b/include/libvirt/libvirt-domain-snapshot.h
index 0f73f24..67ccb59 100644
--- a/include/libvirt/libvirt-domain-snapshot.h
+++ b/include/libvirt/libvirt-domain-snapshot.h
@@ -184,9 +184,13 @@ int virDomainSnapshotHasMetadata(virDomainSnapshotPtr snapshot,
unsigned int flags);
typedef enum {
- VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING = 1 << 0, /* Run after revert */
- VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED = 1 << 1, /* Pause after revert */
- VIR_DOMAIN_SNAPSHOT_REVERT_FORCE = 1 << 2, /* Allow risky reverts */
+ VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING = 1 << 0, /* Run after revert */
+ VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED = 1 << 1, /* Pause after revert */
+ VIR_DOMAIN_SNAPSHOT_REVERT_FORCE = 1 << 2, /* Allow risky reverts */
+ VIR_DOMAIN_SNAPSHOT_REVERT_ACTIVE_ONLY = 1 << 3, /* Use active snapshot
+ configurations as both
+ active and inactive
+ domain configurations*/
} virDomainSnapshotRevertFlags;
/* Revert the domain to a point-in-time snapshot. The
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4ffec70..aecfcff 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15577,6 +15577,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
qemuDomainObjPrivatePtr priv;
int rc;
virDomainDefPtr config = NULL;
+ virDomainDefPtr newConfig = NULL;
virQEMUDriverConfigPtr cfg = NULL;
virCapsPtr caps = NULL;
bool was_running = false;
@@ -15586,7 +15587,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED |
- VIR_DOMAIN_SNAPSHOT_REVERT_FORCE, -1);
+ VIR_DOMAIN_SNAPSHOT_REVERT_FORCE |
+ VIR_DOMAIN_SNAPSHOT_REVERT_ACTIVE_ONLY, -1);
/* We have the following transitions, which create the following events:
* 1. inactive -> inactive: none
@@ -15688,6 +15690,16 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
goto endjob;
}
+ /* Prepare to copy snapshot inactive xml as inactive configuration
+ * of this domain unless user exclusively specify not to copy it */
+ if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_ACTIVE_ONLY) &&
+ snap->def->newDom) {
+ newConfig = virDomainDefCopy(snap->def->newDom, caps,
+ driver->xmlopt, NULL, true);
+ if (!newConfig)
+ goto endjob;
+ }
+
cookie = (qemuDomainSaveCookiePtr) snap->def->cookie;
switch ((virDomainState) snap->def->state) {
@@ -15785,12 +15797,16 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
virCPUDefFree(priv->origCPU);
VIR_STEAL_PTR(priv->origCPU, origCPU);
}
+ if (newConfig)
+ vm->newDef = newConfig;
} else {
/* Transitions 2, 3 */
load:
was_stopped = true;
if (config)
virDomainObjAssignDef(vm, config, false, NULL);
+ if (newConfig)
+ vm->newDef = newConfig;
/* No cookie means libvirt which saved the domain was too old to
* mess up the CPU definitions.
@@ -15884,6 +15900,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
}
if (config)
virDomainObjAssignDef(vm, config, false, NULL);
+ if (newConfig)
+ vm->newDef = newConfig;
if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) {
--
1.8.3.1
10:43 a.m.
New subject: [libvirt] [PATCH 2/7] qemu: Use active and inactive snapshot configuration on restore
On 10/30/2017 04:51 AM, Kothapally Madhu Pavan wrote:
By default, active and inactive XMl snapshot configurations are
*XML
assigned to domain definition. This will make sure that all the
non-persistent configurations of the snapshot are restored back
as it is. This patch will also make sure that user has a choice
s/it is/they were/
??
to choose of using active XML configuration of snapshot as both
active and inactive XML configurations of the restoring domain.
Signed-off-by: Kothapally Madhu Pavan <kmp(a)linux.vnet.ibm.com>
---
include/libvirt/libvirt-domain-snapshot.h | 10 +++++++---
src/qemu/qemu_driver.c | 20 +++++++++++++++++++-
2 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/include/libvirt/libvirt-domain-snapshot.h
b/include/libvirt/libvirt-domain-snapshot.h
index 0f73f24..67ccb59 100644
--- a/include/libvirt/libvirt-domain-snapshot.h
+++ b/include/libvirt/libvirt-domain-snapshot.h
@@ -184,9 +184,13 @@ int virDomainSnapshotHasMetadata(virDomainSnapshotPtr snapshot,
unsigned int flags);
typedef enum {
- VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING = 1 << 0, /* Run after revert */
- VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED = 1 << 1, /* Pause after revert */
- VIR_DOMAIN_SNAPSHOT_REVERT_FORCE = 1 << 2, /* Allow risky reverts */
+ VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING = 1 << 0, /* Run after revert */
+ VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED = 1 << 1, /* Pause after revert */
+ VIR_DOMAIN_SNAPSHOT_REVERT_FORCE = 1 << 2, /* Allow risky reverts */
+ VIR_DOMAIN_SNAPSHOT_REVERT_ACTIVE_ONLY = 1 << 3, /* Use active snapshot
+ configurations as both
+ active and inactive
+ domain configurations*/
Previous def only had 1 space between longest line of "RUNNING" and
"=";
this has 2 spaces between "ONLY" and "=". Just go with 1.
Also the tailing "*/" should gain a space.
The comment doesn't quite make sense though related to the name used for
the enum. I'm having trouble understanding the meaning of "as both
active and inactive" as it relates to REVERT_ACTIVE_ONLY.
I think you're trying to indicate that by setting this flag, when
reverting from the snapshot, that only the "saved" active definition
would be restored and if there was a saved persistent one, then it would
be skipped. IOW: "VIR_DOMAIN_SNAPSHOT_REVERT_SKIP_PERSISTENT"
} virDomainSnapshotRevertFlags;
/* Revert the domain to a point-in-time snapshot. The
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 4ffec70..aecfcff 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15577,6 +15577,7 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
qemuDomainObjPrivatePtr priv;
int rc;
virDomainDefPtr config = NULL;
+ virDomainDefPtr newConfig = NULL;
persistentConfig?
virQEMUDriverConfigPtr cfg = NULL;
virCapsPtr caps = NULL;
bool was_running = false;
@@ -15586,7 +15587,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
virCheckFlags(VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED |
- VIR_DOMAIN_SNAPSHOT_REVERT_FORCE, -1);
+ VIR_DOMAIN_SNAPSHOT_REVERT_FORCE |
+ VIR_DOMAIN_SNAPSHOT_REVERT_ACTIVE_ONLY, -1);
/* We have the following transitions, which create the following events:
* 1. inactive -> inactive: none
@@ -15688,6 +15690,16 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
goto endjob;
}
This is why I ended up writing that long comment in patch 1.
+ /* Prepare to copy snapshot inactive xml as inactive
configuration
+ * of this domain unless user exclusively specify not to copy it */
+ if (!(flags & VIR_DOMAIN_SNAPSHOT_REVERT_ACTIVE_ONLY) &&
+ snap->def->newDom) {
+ newConfig = virDomainDefCopy(snap->def->newDom, caps,
+ driver->xmlopt, NULL, true);
+ if (!newConfig)
+ goto endjob;
+ }
+
cookie = (qemuDomainSaveCookiePtr) snap->def->cookie;
switch ((virDomainState) snap->def->state) {
@@ -15785,12 +15797,16 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
virCPUDefFree(priv->origCPU);
VIR_STEAL_PTR(priv->origCPU, origCPU);
}
+ if (newConfig)
+ vm->newDef = newConfig;
} else {
/* Transitions 2, 3 */
load:
was_stopped = true;
if (config)
virDomainObjAssignDef(vm, config, false, NULL);
+ if (newConfig)
+ vm->newDef = newConfig;
/* No cookie means libvirt which saved the domain was too old to
* mess up the CPU definitions.
@@ -15884,6 +15900,8 @@ qemuDomainRevertToSnapshot(virDomainSnapshotPtr snapshot,
}
if (config)
virDomainObjAssignDef(vm, config, false, NULL);
+ if (newConfig)
+ vm->newDef = newConfig;
Shall I assume you're sure that for all assignments that vm->newDef
isn't already populated with something?
This is why I started thinking about the various existing convenience
API's that managed the def/newDef.
John
if (flags & (VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING |
VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED)) {
3:51 a.m.
New subject: [libvirt] [PATCH 3/7] conf: Allow editing inactive snapshot configuration
This patch will allow user to edit the inactive XML snapshot
configuration when it is available in the snapshot.
Signed-off-by: Kothapally Madhu Pavan <kmp(a)linux.vnet.ibm.com>
---
include/libvirt/libvirt-domain.h | 1 +
src/conf/domain_conf.c | 6 ++++--
src/conf/domain_conf.h | 2 ++
src/conf/snapshot_conf.c | 35 ++++++++++++++++++++++++++++++++++-
src/qemu/qemu_driver.c | 3 ++-
5 files changed, 43 insertions(+), 4 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 4048acf..e70c664 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1546,6 +1546,7 @@ typedef enum {
VIR_DOMAIN_XML_INACTIVE = (1 << 1), /* dump inactive domain information */
VIR_DOMAIN_XML_UPDATE_CPU = (1 << 2), /* update guest CPU requirements
according to host CPU */
VIR_DOMAIN_XML_MIGRATABLE = (1 << 3), /* dump XML suitable for migration */
+ VIR_DOMAIN_XML_ACTIVE_ONLY = (1 << 4), /* dump active XML and avoid inactive
XML in snapshot */
} virDomainXMLFlags;
char * virDomainGetXMLDesc (virDomainPtr domain,
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 77c20c6..36cebe5 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -25687,8 +25687,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,
- -1);
+ VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST |
+ VIR_DOMAIN_DEF_FORMAT_ACTIVE_ONLY, -1);
if (!(type = virDomainVirtTypeToString(def->virtType))) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -26472,6 +26472,8 @@ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int
flags)
formatFlags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE;
if (flags & VIR_DOMAIN_XML_MIGRATABLE)
formatFlags |= VIR_DOMAIN_DEF_FORMAT_MIGRATABLE;
+ if (flags & VIR_DOMAIN_XML_ACTIVE_ONLY)
+ formatFlags |= VIR_DOMAIN_DEF_FORMAT_ACTIVE_ONLY;
return formatFlags;
}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 38de70b..0659220 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2853,6 +2853,8 @@ typedef enum {
VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM = 1 << 6,
VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT = 1 << 7,
VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST = 1 << 8,
+ /* format active XML and avoid inactive XML */
+ VIR_DOMAIN_DEF_FORMAT_ACTIVE_ONLY = 1 << 9,
} virDomainDefFormatFlags;
/* Use these flags to skip specific domain ABI consistency checks done
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index bfe3d6c..3cb7cd4 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -290,6 +290,29 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
} else {
VIR_WARN("parsing older snapshot that lacks domain");
}
+
+ /* Older snapshots were created without inactive domain configuration.
+ * In that case, leave the newDom NULL. */
+ if ((tmp = virXPathString("string(./inactiveDomain/domain/@type)",
ctxt))) {
+ int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
+ VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
+ if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL)
+ domainflags |= VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS;
+ xmlNodePtr domainNode = virXPathNode("./inactiveDomain/domain",
ctxt);
+
+ VIR_FREE(tmp);
+ if (domainNode) {
+ def->newDom = virDomainDefParseNode(ctxt->node->doc,
domainNode,
+ caps, xmlopt, NULL, domainflags);
+ if (!def->newDom)
+ goto cleanup;
+ } else {
+ VIR_WARN("missing inactive domain in snapshot");
+ }
+ } else {
+ VIR_WARN("parsing older snapshot that lacks inactive domain");
+ }
+
} else {
def->creationTime = tv.tv_sec;
}
@@ -705,7 +728,8 @@ virDomainSnapshotDefFormat(const char *domain_uuid,
virBuffer buf = VIR_BUFFER_INITIALIZER;
size_t i;
- virCheckFlags(VIR_DOMAIN_DEF_FORMAT_SECURE, NULL);
+ virCheckFlags(VIR_DOMAIN_DEF_FORMAT_SECURE |
+ VIR_DOMAIN_DEF_FORMAT_ACTIVE_ONLY, NULL);
flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE;
@@ -757,6 +781,15 @@ virDomainSnapshotDefFormat(const char *domain_uuid,
virBufferAddLit(&buf, "</domain>\n");
}
+ if (def->newDom && !(flags & VIR_DOMAIN_DEF_FORMAT_ACTIVE_ONLY)) {
+ virBufferAddLit(&buf, "<inactiveDomain>\n");
+ virBufferAdjustIndent(&buf, 2);
+ if (virDomainDefFormatInternal(def->newDom, caps, flags, &buf) < 0)
+ goto error;
+ virBufferAdjustIndent(&buf, -2);
+ virBufferAddLit(&buf, "</inactiveDomain>\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 aecfcff..a0a4384 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15469,7 +15469,8 @@ qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
virDomainSnapshotObjPtr snap = NULL;
char uuidstr[VIR_UUID_STRING_BUFLEN];
- virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL);
+ virCheckFlags(VIR_DOMAIN_XML_SECURE |
+ VIR_DOMAIN_XML_ACTIVE_ONLY, NULL);
if (!(vm = qemuDomObjFromSnapshot(snapshot)))
return NULL;
--
1.8.3.1
11:06 a.m.
New subject: [libvirt] [PATCH 3/7] conf: Allow editing inactive snapshot configuration
On 10/30/2017 04:51 AM, Kothapally Madhu Pavan wrote:
This patch will allow user to edit the inactive XML snapshot
configuration when it is available in the snapshot.
Signed-off-by: Kothapally Madhu Pavan <kmp(a)linux.vnet.ibm.com>
---
include/libvirt/libvirt-domain.h | 1 +
src/conf/domain_conf.c | 6 ++++--
src/conf/domain_conf.h | 2 ++
src/conf/snapshot_conf.c | 35 ++++++++++++++++++++++++++++++++++-
src/qemu/qemu_driver.c | 3 ++-
5 files changed, 43 insertions(+), 4 deletions(-)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 4048acf..e70c664 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1546,6 +1546,7 @@ typedef enum {
VIR_DOMAIN_XML_INACTIVE = (1 << 1), /* dump inactive domain information
*/
VIR_DOMAIN_XML_UPDATE_CPU = (1 << 2), /* update guest CPU requirements
according to host CPU */
VIR_DOMAIN_XML_MIGRATABLE = (1 << 3), /* dump XML suitable for migration */
+ VIR_DOMAIN_XML_ACTIVE_ONLY = (1 << 4), /* dump active XML and avoid inactive
XML in snapshot */
Not liking the name - makes me wonder why the negation of the existing
VIR_DOMAIN_XML_INACTIVE wasn't used in some way...
It doesn't scream use this only for SNAPSHOT manipulation.
Consider: VIR_DOMAIN_XML_SNAPSHOT_ACTIVE
} virDomainXMLFlags;
char * virDomainGetXMLDesc (virDomainPtr domain,
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 77c20c6..36cebe5 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -25687,8 +25687,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,
- -1);
+ VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST |
+ VIR_DOMAIN_DEF_FORMAT_ACTIVE_ONLY, -1);
if (!(type = virDomainVirtTypeToString(def->virtType))) {
virReportError(VIR_ERR_INTERNAL_ERROR,
@@ -26472,6 +26472,8 @@ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int
flags)
formatFlags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE;
if (flags & VIR_DOMAIN_XML_MIGRATABLE)
formatFlags |= VIR_DOMAIN_DEF_FORMAT_MIGRATABLE;
+ if (flags & VIR_DOMAIN_XML_ACTIVE_ONLY)
+ formatFlags |= VIR_DOMAIN_DEF_FORMAT_ACTIVE_ONLY;
return formatFlags;
}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 38de70b..0659220 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2853,6 +2853,8 @@ typedef enum {
VIR_DOMAIN_DEF_FORMAT_ALLOW_ROM = 1 << 6,
VIR_DOMAIN_DEF_FORMAT_ALLOW_BOOT = 1 << 7,
VIR_DOMAIN_DEF_FORMAT_CLOCK_ADJUST = 1 << 8,
+ /* format active XML and avoid inactive XML */
+ VIR_DOMAIN_DEF_FORMAT_ACTIVE_ONLY = 1 << 9,
} virDomainDefFormatFlags;
/* Use these flags to skip specific domain ABI consistency checks done
diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c
index bfe3d6c..3cb7cd4 100644
--- a/src/conf/snapshot_conf.c
+++ b/src/conf/snapshot_conf.c
@@ -290,6 +290,29 @@ virDomainSnapshotDefParse(xmlXPathContextPtr ctxt,
} else {
VIR_WARN("parsing older snapshot that lacks domain");
}
+
+ /* Older snapshots were created without inactive domain configuration.
+ * In that case, leave the newDom NULL. */
+ if ((tmp = virXPathString("string(./inactiveDomain/domain/@type)",
ctxt))) {
+ int domainflags = VIR_DOMAIN_DEF_PARSE_INACTIVE |
+ VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE;
+ if (flags & VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL)
+ domainflags |= VIR_DOMAIN_DEF_PARSE_SKIP_OSTYPE_CHECKS;
+ xmlNodePtr domainNode = virXPathNode("./inactiveDomain/domain",
ctxt);
+
+ VIR_FREE(tmp);
+ if (domainNode) {
+ def->newDom = virDomainDefParseNode(ctxt->node->doc,
domainNode,
+ caps, xmlopt, NULL, domainflags);
+ if (!def->newDom)
+ goto cleanup;
+ } else {
+ VIR_WARN("missing inactive domain in snapshot");
But if dom->newDef never existed, then why would snapshotDef->newDom exist?
+ }
+ } else {
+ VIR_WARN("parsing older snapshot that lacks inactive domain");
So? You need to be able to handle a NULL snapshotDef->newDom being NULL
anyway, so not sure the WARN is appropriate here.
+ }
+
} else {
def->creationTime = tv.tv_sec;
}
@@ -705,7 +728,8 @@ virDomainSnapshotDefFormat(const char *domain_uuid,
virBuffer buf = VIR_BUFFER_INITIALIZER;
size_t i;
- virCheckFlags(VIR_DOMAIN_DEF_FORMAT_SECURE, NULL);
+ virCheckFlags(VIR_DOMAIN_DEF_FORMAT_SECURE |
+ VIR_DOMAIN_DEF_FORMAT_ACTIVE_ONLY, NULL);
flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE;
@@ -757,6 +781,15 @@ virDomainSnapshotDefFormat(const char *domain_uuid,
virBufferAddLit(&buf, "</domain>\n");
}
+ if (def->newDom && !(flags & VIR_DOMAIN_DEF_FORMAT_ACTIVE_ONLY)) {
+ virBufferAddLit(&buf, "<inactiveDomain>\n");
+ virBufferAdjustIndent(&buf, 2);
+ if (virDomainDefFormatInternal(def->newDom, caps, flags, &buf) < 0)
+ goto error;
+ virBufferAdjustIndent(&buf, -2);
+ virBufferAddLit(&buf, "</inactiveDomain>\n");
+ }
+
So after all this, it's not clear why there needs to be a special flag
for snapshot and why having this flag allows editing (IOW: The commit
message perhaps is misleading).
It's even more problematic since domain->newDef isn't necessarily
defined and thus def->newDom wouldn't be either.
I think perhaps the Parse and Format code should be their own patch
anyway, so that part is useful; however, the new flag, I'm not sure yet.
If your goal is to mimic the domain inactive flag for snapshot, then
follow how domain command line processing goes. Inventing an
"active-only" (subsequent patch) is not something I'm sure is desired
(yet). Consistency between various commands is (to me at least) far more
desirable. Currently it seems usage if "--inactive" on the dumpxml
command line is preferred (domain, net-*, iface-*, pool-*, etc).
If "today" what someone is doing is editing/dumping the active XML, then
what you're allowing in the future is to allow someone to edit/dump the
inactive XML. For that purpose, there are existing flags.
John
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 aecfcff..a0a4384 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -15469,7 +15469,8 @@ qemuDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
virDomainSnapshotObjPtr snap = NULL;
char uuidstr[VIR_UUID_STRING_BUFLEN];
- virCheckFlags(VIR_DOMAIN_XML_SECURE, NULL);
+ virCheckFlags(VIR_DOMAIN_XML_SECURE |
+ VIR_DOMAIN_XML_ACTIVE_ONLY, NULL);
if (!(vm = qemuDomObjFromSnapshot(snapshot)))
return NULL;
3:51 a.m.
New subject: [libvirt] [PATCH 4/7] virsh: Dump inactive XML configuration of snapshot using snapshot-dumpxml
Now, snapshot-dumpxml will display inactive XML configuration of snapshot
along with active XML configuration. When --active-only flag is used the
inactive XML configuration will not be displayed.
Signed-off-by: Kothapally Madhu Pavan <kmp(a)linux.vnet.ibm.com>
---
tools/virsh-snapshot.c | 7 +++++++
tools/virsh.pod | 20 ++++++++++++++++++++
2 files changed, 27 insertions(+)
diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index 24cd4ab..4b0a18d 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -1667,6 +1667,10 @@ static const vshCmdOptDef opts_snapshot_dumpxml[] = {
.type = VSH_OT_BOOL,
.help = N_("include security sensitive information in XML dump")
},
+ {.name = "active-only",
+ .type = VSH_OT_BOOL,
+ .help = N_("dump only active XML configuration and avoid inactive XML")
+ },
{.name = NULL}
};
@@ -1683,6 +1687,9 @@ cmdSnapshotDumpXML(vshControl *ctl, const vshCmd *cmd)
if (vshCommandOptBool(cmd, "security-info"))
flags |= VIR_DOMAIN_XML_SECURE;
+ if (vshCommandOptBool(cmd, "active-only"))
+ flags |= VIR_DOMAIN_XML_ACTIVE_ONLY;
+
if (vshCommandOptStringReq(ctl, cmd, "snapshotname", &name) < 0)
return false;
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 69cc423..f899da7 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -4489,11 +4489,31 @@ is specified, the list will be filtered to snapshots that use
external
files for disk images or memory state.
=item B<snapshot-dumpxml> I<domain> I<snapshot>
[I<--security-info>]
+[I<--active-only>]
Output the snapshot XML for the domain's snapshot named I<snapshot>.
Using I<--security-info> will also include security sensitive information.
Use B<snapshot-current> to easily access the XML of the current snapshot.
+If I<--active-only> is specified, only active XML configuration of the
+snapshot is displayed. Otherwise, both active and inactive XML
+configuration of the snapshot will be displayed.
+
+When both active and inactive XML snapshot configurations are displayed,
+as inactive XML configuration will have same structure as active XML
+it is embedded within <inactiveDomain> tag as shown below:
+
+ <domainsnapshot>
+ ....
+ ....
+ <inactiveDomain>
+ <domain>
+ ....
+ ....
+ </domain>
+ </inactiveDomain>
+ </domainsnapshot>
+
=item B<snapshot-parent> I<domain> {I<snapshot> | I<--current>}
Output the name of the parent snapshot, if any, for the given
--
1.8.3.1
11:34 a.m.
New subject: [libvirt] [PATCH 4/7] virsh: Dump inactive XML configuration of snapshot using snapshot-dumpxml
On 10/30/2017 04:51 AM, Kothapally Madhu Pavan wrote:
Now, snapshot-dumpxml will display inactive XML configuration of
snapshot
along with active XML configuration. When --active-only flag is used the
inactive XML configuration will not be displayed.
Signed-off-by: Kothapally Madhu Pavan <kmp(a)linux.vnet.ibm.com>
---
tools/virsh-snapshot.c | 7 +++++++
tools/virsh.pod | 20 ++++++++++++++++++++
2 files changed, 27 insertions(+)
Hmm... ahhh... so patch 3 is dumping the inactive XML in the middle of
the active XML... And we're doing that for what reason? Why would the
snapshot XML need to be different?
If one can use --inactive flags for dumpxml to specifically dump the
"newDom" if it exists (or the "dom" if it doesn't), then it would
essentially follow how the domain does this (see qemuDomainFormatXML).
Looking back at patch 3 now, I guess generating XML for the inactive
within the context of active just doesn't seem right.
John
diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index 24cd4ab..4b0a18d 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -1667,6 +1667,10 @@ static const vshCmdOptDef opts_snapshot_dumpxml[] = {
.type = VSH_OT_BOOL,
.help = N_("include security sensitive information in XML dump")
},
+ {.name = "active-only",
+ .type = VSH_OT_BOOL,
+ .help = N_("dump only active XML configuration and avoid inactive XML")
+ },
{.name = NULL}
};
@@ -1683,6 +1687,9 @@ cmdSnapshotDumpXML(vshControl *ctl, const vshCmd *cmd)
if (vshCommandOptBool(cmd, "security-info"))
flags |= VIR_DOMAIN_XML_SECURE;
+ if (vshCommandOptBool(cmd, "active-only"))
+ flags |= VIR_DOMAIN_XML_ACTIVE_ONLY;
+
if (vshCommandOptStringReq(ctl, cmd, "snapshotname", &name) < 0)
return false;
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 69cc423..f899da7 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -4489,11 +4489,31 @@ is specified, the list will be filtered to snapshots that use
external
files for disk images or memory state.
=item B<snapshot-dumpxml> I<domain> I<snapshot>
[I<--security-info>]
+[I<--active-only>]
Output the snapshot XML for the domain's snapshot named I<snapshot>.
Using I<--security-info> will also include security sensitive information.
Use B<snapshot-current> to easily access the XML of the current snapshot.
+If I<--active-only> is specified, only active XML configuration of the
+snapshot is displayed. Otherwise, both active and inactive XML
+configuration of the snapshot will be displayed.
+
+When both active and inactive XML snapshot configurations are displayed,
+as inactive XML configuration will have same structure as active XML
+it is embedded within <inactiveDomain> tag as shown below:
+
+ <domainsnapshot>
+ ....
+ ....
+ <inactiveDomain>
+ <domain>
+ ....
+ ....
+ </domain>
+ </inactiveDomain>
+ </domainsnapshot>
+
=item B<snapshot-parent> I<domain> {I<snapshot> | I<--current>}
Output the name of the parent snapshot, if any, for the given
3:51 a.m.
New subject: [libvirt] [PATCH 5/7] virsh: Edit inactive XML configuration of snapshot using snapshot-edit
Now, snapshot-edit will allow editing inactive XML configuration of snapshot
along with active XML configuration. When --active-only flag is used the
inactive XML will not be displayed and will be removed from snapshot.
--active-only flag is used when user doesn't what any non-persistent
configuration in domain after restoring the snapshot.
Signed-off-by: Kothapally Madhu Pavan <kmp(a)linux.vnet.ibm.com>
---
tools/virsh-snapshot.c | 7 +++++++
tools/virsh.pod | 8 +++++++-
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index 4b0a18d..48fc034 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -522,6 +522,10 @@ static const vshCmdOptDef opts_snapshot_edit[] = {
.type = VSH_OT_BOOL,
.help = N_("allow cloning to new name")
},
+ {.name = "active-only",
+ .type = VSH_OT_BOOL,
+ .help = N_("allow editing active XML configuration and remove inactive
XML")
+ },
{.name = NULL}
};
@@ -545,6 +549,9 @@ cmdSnapshotEdit(vshControl *ctl, const vshCmd *cmd)
vshCommandOptBool(cmd, "snapshotname"))
define_flags |= VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT;
+ if (vshCommandOptBool(cmd, "active-only"))
+ getxml_flags |= VIR_DOMAIN_XML_ACTIVE_ONLY;
+
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
return false;
diff --git a/tools/virsh.pod b/tools/virsh.pod
index f899da7..0578f8f 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -4400,7 +4400,7 @@ With I<snapshotname>, this is a request to make the existing
named
snapshot become the current snapshot, without reverting the domain.
=item B<snapshot-edit> I<domain> [I<snapshotname>]
[I<--current>]
-{[I<--rename>] | [I<--clone>]}
+{[I<--rename>] | [I<--clone>]} [I<--active-only>]
Edit the XML configuration file for I<snapshotname> of a domain. If
both I<snapshotname> and I<--current> are specified, also force the
@@ -4427,6 +4427,12 @@ a snapshot name must be done with care, since the contents of some
snapshots, such as internal snapshots within a single qcow2 file, are
accessible only from the original name.
+If I<--active-only> is specified, only active XML configuration of the
+snapshot is displayed to edit. Otherwise, both active and inactive XML
+configuration of the snapshot will be displayed to edit. When domain
+snapshot is edited with I<--active-only> flag, inactive XML configuration
+will be removed from snapshot.
+
=item B<snapshot-info> I<domain> {I<snapshot> | I<--current>}
Output basic information about a named <snapshot>, or the current snapshot
--
1.8.3.1
11:35 a.m.
New subject: [libvirt] [PATCH 5/7] virsh: Edit inactive XML configuration of snapshot using snapshot-edit
On 10/30/2017 04:51 AM, Kothapally Madhu Pavan wrote:
Now, snapshot-edit will allow editing inactive XML configuration of
snapshot
along with active XML configuration. When --active-only flag is used the
inactive XML will not be displayed and will be removed from snapshot.
--active-only flag is used when user doesn't what any non-persistent
configuration in domain after restoring the snapshot.
Signed-off-by: Kothapally Madhu Pavan <kmp(a)linux.vnet.ibm.com>
---
tools/virsh-snapshot.c | 7 +++++++
tools/virsh.pod | 8 +++++++-
2 files changed, 14 insertions(+), 1 deletion(-)
Similar comment here as in patch 4... Also note additional minor edit
from below...
diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index 4b0a18d..48fc034 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -522,6 +522,10 @@ static const vshCmdOptDef opts_snapshot_edit[] = {
.type = VSH_OT_BOOL,
.help = N_("allow cloning to new name")
},
+ {.name = "active-only",
+ .type = VSH_OT_BOOL,
+ .help = N_("allow editing active XML configuration and remove inactive
XML")
+ },
{.name = NULL}
};
@@ -545,6 +549,9 @@ cmdSnapshotEdit(vshControl *ctl, const vshCmd *cmd)
vshCommandOptBool(cmd, "snapshotname"))
define_flags |= VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT;
+ if (vshCommandOptBool(cmd, "active-only"))
+ getxml_flags |= VIR_DOMAIN_XML_ACTIVE_ONLY;
+
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
return false;
diff --git a/tools/virsh.pod b/tools/virsh.pod
index f899da7..0578f8f 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -4400,7 +4400,7 @@ With I<snapshotname>, this is a request to make the existing
named
snapshot become the current snapshot, without reverting the domain.
=item B<snapshot-edit> I<domain> [I<snapshotname>]
[I<--current>]
-{[I<--rename>] | [I<--clone>]}
+{[I<--rename>] | [I<--clone>]} [I<--active-only>]
Edit the XML configuration file for I<snapshotname> of a domain. If
both I<snapshotname> and I<--current> are specified, also force the
@@ -4427,6 +4427,12 @@ a snapshot name must be done with care, since the contents of
some
snapshots, such as internal snapshots within a single qcow2 file, are
accessible only from the original name.
+If I<--active-only> is specified, only active XML configuration of the
+snapshot is displayed to edit. Otherwise, both active and inactive XML
+configuration of the snapshot will be displayed to edit. When domain
+snapshot is edited with I<--active-only> flag, inactive XML configuration
+will be removed from snapshot.
FWIW: git am tells me above line had whitespace at the end...
John
+
=item B<snapshot-info> I<domain> {I<snapshot> | I<--current>}
Output basic information about a named <snapshot>, or the current snapshot
3:51 a.m.
New subject: [libvirt] [PATCH 6/7] virsh: Allow restoring snapshot with non-persistent configuration
Now, snapshot-restore will allow restoring snapshots with non-persistent
configuration as both active and inactive XML configurations are saved
in snapshot. User can discard non-persistent configuratin of a domain
using --active-only flag. When --active-only flag is used, active XML
configuration of the snapshot is used as both the active and inactive
XML configuration of the domain after restore.
Signed-off-by: Kothapally Madhu Pavan <kmp(a)linux.vnet.ibm.com>
---
tools/virsh-snapshot.c | 6 ++++++
tools/virsh.pod | 9 ++++++++-
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index 48fc034..7f6a231 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -1811,6 +1811,10 @@ static const vshCmdOptDef opts_snapshot_revert[] = {
.type = VSH_OT_BOOL,
.help = N_("try harder on risky reverts")
},
+ {.name = "active-only",
+ .type = VSH_OT_BOOL,
+ .help = N_("use only active snapshot configuration when restoring")
+ },
{.name = NULL}
};
@@ -1835,6 +1839,8 @@ cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd)
* when the error says it will make a difference. */
if (vshCommandOptBool(cmd, "force"))
force = true;
+ if (vshCommandOptBool(cmd, "active-only"))
+ flags |= VIR_DOMAIN_SNAPSHOT_REVERT_ACTIVE_ONLY;
dom = virshCommandOptDomain(ctl, cmd, NULL);
if (dom == NULL)
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 0578f8f..791014e 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -4526,7 +4526,7 @@ Output the name of the parent snapshot, if any, for the given
I<snapshot>, or for the current snapshot with I<--current>.
=item B<snapshot-revert> I<domain> {I<snapshot> | I<--current>}
-[{I<--running> | I<--paused>}] [I<--force>]
+[{I<--running> | I<--paused>}] [I<--force>] [I<--active-only>]
Revert the given domain to the snapshot specified by I<snapshot>, or to
the current snapshot with I<--current>. Be aware
@@ -4559,6 +4559,13 @@ snapshot that uses a provably incompatible configuration, as well
as
with an inactive snapshot that is combined with the I<--start> or
I<--pause> flag.
+When inactive XML configuration of a snapshot is available along with
+active XML configuration both the inactive and active XMl configurations
+are used to restore the snapshot. This will keep the non-persistent
+configuration alive after restoring a snapshot. User can kill the
+non-persistent configuration by issuing I<--active-only> flag. This will
+use active XML configuraton alone to revert the snapshot.
+
=item B<snapshot-delete> I<domain> {I<snapshot> | I<--current>}
[I<--metadata>]
[{I<--children> | I<--children-only>}]
--
1.8.3.1
3:48 p.m.
New subject: [libvirt] [PATCH 6/7] virsh: Allow restoring snapshot with non-persistent configuration
On 10/30/2017 04:51 AM, Kothapally Madhu Pavan wrote:
Now, snapshot-restore will allow restoring snapshots with
non-persistent
configuration as both active and inactive XML configurations are saved
in snapshot. User can discard non-persistent configuratin of a domain
configuration
using --active-only flag. When --active-only flag is used, active
XML
configuration of the snapshot is used as both the active and inactive
XML configuration of the domain after restore.
really?
My recollection is, newDom is generated based on whether newDef exists.
There isn't any code that would generate newDom from dom unless I'm
missing something subtle.
Signed-off-by: Kothapally Madhu Pavan <kmp(a)linux.vnet.ibm.com>
---
tools/virsh-snapshot.c | 6 ++++++
tools/virsh.pod | 9 ++++++++-
2 files changed, 14 insertions(+), 1 deletion(-)
Wish this was "closer to" patch 2 - short term memory loss already ;-)
So what would be the reason to only restore ->dom and not ->newDom?
Still, consider "remove-inactive" as a flag or "remove-persistent".
In the long run it's all about describing things consistently - if a
domain is active and persistent, then shouldn't the snapshot follow
suit? The more recent domain flags make consistent use of "config" and
"live". And there's a lot of "behavio[u]r is different depending on
hypervisor"..
I would think "historically" consumers would be thinking they were
managing the active snapshot - to have active switches which end up
being negative logic for the persistent portion is a bit confusing.
diff --git a/tools/virsh-snapshot.c b/tools/virsh-snapshot.c
index 48fc034..7f6a231 100644
--- a/tools/virsh-snapshot.c
+++ b/tools/virsh-snapshot.c
@@ -1811,6 +1811,10 @@ static const vshCmdOptDef opts_snapshot_revert[] = {
.type = VSH_OT_BOOL,
.help = N_("try harder on risky reverts")
},
+ {.name = "active-only",
+ .type = VSH_OT_BOOL,
+ .help = N_("use only active snapshot configuration when restoring")
+ },
{.name = NULL}
};
@@ -1835,6 +1839,8 @@ cmdDomainSnapshotRevert(vshControl *ctl, const vshCmd *cmd)
* when the error says it will make a difference. */
if (vshCommandOptBool(cmd, "force"))
force = true;
+ if (vshCommandOptBool(cmd, "active-only"))
+ flags |= VIR_DOMAIN_SNAPSHOT_REVERT_ACTIVE_ONLY;
dom = virshCommandOptDomain(ctl, cmd, NULL);
if (dom == NULL)
diff --git a/tools/virsh.pod b/tools/virsh.pod
index 0578f8f..791014e 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -4526,7 +4526,7 @@ Output the name of the parent snapshot, if any, for the given
I<snapshot>, or for the current snapshot with I<--current>.
=item B<snapshot-revert> I<domain> {I<snapshot> | I<--current>}
-[{I<--running> | I<--paused>}] [I<--force>]
+[{I<--running> | I<--paused>}] [I<--force>] [I<--active-only>]
Revert the given domain to the snapshot specified by I<snapshot>, or to
the current snapshot with I<--current>. Be aware
@@ -4559,6 +4559,13 @@ snapshot that uses a provably incompatible configuration, as well
as
with an inactive snapshot that is combined with the I<--start> or
I<--pause> flag.
+When inactive XML configuration of a snapshot is available along with
+active XML configuration both the inactive and active XMl configurations
+are used to restore the snapshot. This will keep the non-persistent
+configuration alive after restoring a snapshot. User can kill the
s/kill/remove/
+non-persistent configuration by issuing I<--active-only> flag.
This will
"non-persistent" - now I'm getting more confused. This is why it's
nice to have the code together, but looking back at patch 2 the flag
would mean that newDom is not restored and thus newDef not filled in
which to me says, the persistent def wouldn't be restored.
+use active XML configuraton alone to revert the snapshot.
configuration (at least you were consistent!)
John
+
=item B<snapshot-delete> I<domain> {I<snapshot> | I<--current>}
[I<--metadata>]
[{I<--children> | I<--children-only>}]
3:51 a.m.
New subject: [libvirt] [PATCH 7/7] tests: docs: Add schema and testcase for domainsnapshot
Alter the schema of domainsnapshot to add inactive XML of a snapshot.
As, snapshot already has active XML configuration of domain, the inactive
XMl is embedded in <inactiveDomain> tags. Sample XML is:
<domainsnapshot>
....
....
<domain>
</domain>
<inactiveDomain>
<domain>
</domain>
</inactiveDomain>
</domainsnapshot>
Alter the domainsnapshotxml2xmltest to validate the format.
Signed-off-by: Kothapally Madhu Pavan <kmp(a)linux.vnet.ibm.com>
---
docs/schemas/domainsnapshot.rng | 19 +++++
.../full_domain_withinactive.xml | 83 ++++++++++++++++++++++
tests/domainsnapshotxml2xmltest.c | 1 +
3 files changed, 103 insertions(+)
create mode 100644 tests/domainsnapshotxml2xmlout/full_domain_withinactive.xml
diff --git a/docs/schemas/domainsnapshot.rng b/docs/schemas/domainsnapshot.rng
index 2680887..2a58a84 100644
--- a/docs/schemas/domainsnapshot.rng
+++ b/docs/schemas/domainsnapshot.rng
@@ -84,6 +84,25 @@
</choice>
</optional>
<optional>
+ <choice>
+ <element name='inactiveDomain'>
+ <choice>
+ <element name='domain'>
+ <element name='uuid'>
+ <ref name="UUID"/>
+ </element>
+ </element>
+ <!-- Nested grammar ensures that any of our overrides of
+ storagecommon/domaincommon defines do not conflict
+ with any domain.rng overrides. -->
+ <grammar>
+ <include href='domain.rng'/>
+ </grammar>
+ </choice>
+ </element>
+ </choice>
+ </optional>
+ <optional>
<element name='parent'>
<element name='name'>
<text/>
diff --git a/tests/domainsnapshotxml2xmlout/full_domain_withinactive.xml
b/tests/domainsnapshotxml2xmlout/full_domain_withinactive.xml
new file mode 100644
index 0000000..d6d1b39
--- /dev/null
+++ b/tests/domainsnapshotxml2xmlout/full_domain_withinactive.xml
@@ -0,0 +1,83 @@
+<domainsnapshot>
+ <name>my snap name</name>
+ <description>!@#$%^</description>
+ <state>running</state>
+ <parent>
+ <name>earlier_snap</name>
+ </parent>
+ <creationTime>1272917631</creationTime>
+ <memory snapshot='internal'/>
+ <domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-i686</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x03' function='0x0'/>
+ </memballoon>
+ </devices>
+ </domain>
+ <inactiveDomain>
+ <domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'
cpuset='1-4,8-20,525'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-i686</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='pci' index='0'
model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x03' function='0x0'/>
+ </memballoon>
+ </devices>
+ </domain>
+ </inactiveDomain>
+ <active>1</active>
+</domainsnapshot>
diff --git a/tests/domainsnapshotxml2xmltest.c b/tests/domainsnapshotxml2xmltest.c
index 3a6f86b..ebec2de 100644
--- a/tests/domainsnapshotxml2xmltest.c
+++ b/tests/domainsnapshotxml2xmltest.c
@@ -205,6 +205,7 @@ mymain(void)
DO_TEST_OUT("all_parameters",
"9d37b878-a7cc-9f9a-b78f-49b3abad25a8", true);
DO_TEST_OUT("disk_snapshot_redefine",
"c7a5fdbd-edaf-9455-926a-d65c16db1809", true);
DO_TEST_OUT("full_domain",
"c7a5fdbd-edaf-9455-926a-d65c16db1809", true);
+ DO_TEST_OUT("full_domain_withinactive",
"c7a5fdbd-edaf-9455-926a-d65c16db1809", true);
DO_TEST_OUT("noparent_nodescription_noactive", NULL, false);
DO_TEST_OUT("noparent_nodescription", NULL, true);
DO_TEST_OUT("noparent", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8",
false);
--
1.8.3.1
5:10 p.m.
New subject: [libvirt] [PATCH 7/7] tests: docs: Add schema and testcase for domainsnapshot
On 10/30/2017 04:51 AM, Kothapally Madhu Pavan wrote:
Alter the schema of domainsnapshot to add inactive XML of a
snapshot.
As, snapshot already has active XML configuration of domain, the inactive
XMl is embedded in <inactiveDomain> tags. Sample XML is:
<domainsnapshot>
....
....
<domain>
</domain>
<inactiveDomain>
<domain>
</domain>
</inactiveDomain>
</domainsnapshot>
Alter the domainsnapshotxml2xmltest to validate the format.
Signed-off-by: Kothapally Madhu Pavan <kmp(a)linux.vnet.ibm.com>
---
docs/schemas/domainsnapshot.rng | 19 +++++>
.../full_domain_withinactive.xml | 83
++++++++++++++++++++++
probably should be "with_inactive" because a quick read I got "within
active" ;-)
tests/domainsnapshotxml2xmltest.c | 1 +
3 files changed, 103 insertions(+)
create mode 100644 tests/domainsnapshotxml2xmlout/full_domain_withinactive.xml
This should have been paired with the snapshot_conf.c change(s) to parse
and format the XML. I think it would have helped me a lot when reading
the virsh: patches.
Typically a series also includes a docs/formatsnapshot.html.in change to
describe the new XML on the website.
A subsequent patch would change "docs/news.xml" to describe the
adjustments in general.
Now that I've read everything, it would seem to me that this "inactive"
(or persistent) is the new piece. So given that, I would think it's what
should be used to formulate various surrounding flags and command
switches. Before this code, the dump/edit would display/edit the "live"
or "active" or "primary" def data. That doesn't necessarily
change, but
the ability to specify the 'inactive' (persistent/new) data would.
Some more thoughts...
1. Always save off the persistent def in the output and always restore
with it if it exists.
2. If someone wants to remove the inactive/persistent, then they can
edit it out.
3. It's too bad virDomainSnapshotDelete couldn't do something with a
flag to remove the inactive/persistent for the named snapshot if it existed.
4. Continue to only format the active (or non persistent) data unless
someone uses the --inactive switch or some INACTIVE flag. That means
the current format code would need to check the INACTIVE flag before
always printing ->dom, but there's a gotcha with the UUID option.
5. Perhaps add an --all flag/ALL option to format *both*.
6. Not sure I agree with having Revert be able to remove the inactive/
persistent (newDom) if it existed. That's one of those trying to do too
much with one command that will end up biting us later on, somehow.
7. It may be interesting to look at the how the disks are handled in
the current code in order to think about them from the viewpoint of
having both dom and newDom type data.
8. Again - think in terms of how the domain has handled "hiding" def
and newDef at least w/r/t using def for persistent in the event that
newDef is NULL. That model perhaps will help you through the thought
process of snapshot history for active vs. inactive data. If inactive
isn't there, then all we can assume is it wasn't saved or necessary at
save time.
Hopefully someone else will also jump in and provide some feedback. I
don't believe the patches are far off, but I do believe need some
tweaking. Designing by patch submission is always difficult.
diff --git a/docs/schemas/domainsnapshot.rng
b/docs/schemas/domainsnapshot.rng
index 2680887..2a58a84 100644
--- a/docs/schemas/domainsnapshot.rng
+++ b/docs/schemas/domainsnapshot.rng
@@ -84,6 +84,25 @@
</choice>
</optional>
<optional>
+ <choice>
+ <element name='inactiveDomain'>
+ <choice>
+ <element name='domain'>
+ <element name='uuid'>
+ <ref name="UUID"/>
+ </element>
Is this used for the inactive - don't believe so from how I read the
Parse and Format code.
If supplied one would hope it's the same!
John
+ </element>
+ <!-- Nested grammar ensures that any of our overrides of
+ storagecommon/domaincommon defines do not conflict
+ with any domain.rng overrides. -->
+ <grammar>
+ <include href='domain.rng'/>
+ </grammar>
+ </choice>
+ </element>
+ </choice>
+ </optional>
+ <optional>
<element name='parent'>
<element name='name'>
<text/>
diff --git a/tests/domainsnapshotxml2xmlout/full_domain_withinactive.xml
b/tests/domainsnapshotxml2xmlout/full_domain_withinactive.xml
new file mode 100644
index 0000000..d6d1b39
--- /dev/null
+++ b/tests/domainsnapshotxml2xmlout/full_domain_withinactive.xml
@@ -0,0 +1,83 @@
+<domainsnapshot>
+ <name>my snap name</name>
+ <description>!@#$%^</description>
+ <state>running</state>
+ <parent>
+ <name>earlier_snap</name>
+ </parent>
+ <creationTime>1272917631</creationTime>
+ <memory snapshot='internal'/>
+ <domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-i686</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='pci' index='0' model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x03' function='0x0'/>
+ </memballoon>
+ </devices>
+ </domain>
+ <inactiveDomain>
+ <domain type='qemu'>
+ <name>QEMUGuest1</name>
+ <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+ <memory unit='KiB'>219100</memory>
+ <currentMemory unit='KiB'>219100</currentMemory>
+ <vcpu placement='static'
cpuset='1-4,8-20,525'>1</vcpu>
+ <os>
+ <type arch='i686' machine='pc'>hvm</type>
+ <boot dev='hd'/>
+ </os>
+ <clock offset='utc'/>
+ <on_poweroff>destroy</on_poweroff>
+ <on_reboot>restart</on_reboot>
+ <on_crash>destroy</on_crash>
+ <devices>
+ <emulator>/usr/bin/qemu-system-i686</emulator>
+ <disk type='block' device='disk'>
+ <source dev='/dev/HostVG/QEMUGuest1'/>
+ <target dev='hda' bus='ide'/>
+ <address type='drive' controller='0' bus='0'
target='0' unit='0'/>
+ </disk>
+ <controller type='usb' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x2'/>
+ </controller>
+ <controller type='ide' index='0'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x01' function='0x1'/>
+ </controller>
+ <controller type='pci' index='0'
model='pci-root'/>
+ <input type='mouse' bus='ps2'/>
+ <input type='keyboard' bus='ps2'/>
+ <memballoon model='virtio'>
+ <address type='pci' domain='0x0000' bus='0x00'
slot='0x03' function='0x0'/>
+ </memballoon>
+ </devices>
+ </domain>
+ </inactiveDomain>
+ <active>1</active>
+</domainsnapshot>
diff --git a/tests/domainsnapshotxml2xmltest.c b/tests/domainsnapshotxml2xmltest.c
index 3a6f86b..ebec2de 100644
--- a/tests/domainsnapshotxml2xmltest.c
+++ b/tests/domainsnapshotxml2xmltest.c
@@ -205,6 +205,7 @@ mymain(void)
DO_TEST_OUT("all_parameters",
"9d37b878-a7cc-9f9a-b78f-49b3abad25a8", true);
DO_TEST_OUT("disk_snapshot_redefine",
"c7a5fdbd-edaf-9455-926a-d65c16db1809", true);
DO_TEST_OUT("full_domain",
"c7a5fdbd-edaf-9455-926a-d65c16db1809", true);
+ DO_TEST_OUT("full_domain_withinactive",
"c7a5fdbd-edaf-9455-926a-d65c16db1809", true);
DO_TEST_OUT("noparent_nodescription_noactive", NULL, false);
DO_TEST_OUT("noparent_nodescription", NULL, true);
DO_TEST_OUT("noparent", "9d37b878-a7cc-9f9a-b78f-49b3abad25a8",
false);
1:41 p.m.
On Mon, 2017-10-30 at 14:21 +0530, Kothapally Madhu Pavan wrote:
Restoring to a snapshot should not overwrite the persistent XML
configuration
of a snapshot as a side effect. This patchset fixes the same.
Currently,
virDomainSnapshotDef only saves active domain definition of the
guest.
And on restore the active domain definition is used as both active
and
inactive domain definitions. This will make the non-persistent
changes
persistent in snapshot image. This patchset allows to save inactive
domain
definition as well and on snapshot-revert non-persistent
configuration is
restored as is.
Currently, snapshot-revert is making non-presistent changes as
persistent.
Here are the steps to reproduce.
Step1: virsh define $dom
Step2: virsh attach-device $dom $memory-device.xml --live
Step3: virsh snapshot-create $dom
Step4: virsh destroy $dom
Step5: virsh snapshot-revert $dom $snapshot-name
Step6: virsh destroy $dom
Step7: virsh start $dom
Here we still have $memory-device attached in Step2.
This patchset is attempting to solve this issue. This patchset will
also
allow user to dump and edit inactive XML configuration of a snapshot.
Dumping inactive domain definition of a snapshot is important as
--redefine uses snapshot-dumpxml output to redefine a snapshot.
Kothapally Madhu Pavan (7):
qemu: Store inactive domain configuration in snapshot
qemu: Use active and inactive snapshot configuration on restore
conf: Allow editing inactive snapshot configuration
virsh: Dump inactive XML configuration of snapshot using
snapshot-dumpxml
virsh: Edit inactive XML configuration of snapshot using snapshot-
edit
virsh: Allow restoring snapshot with non-persistent configuration
tests: docs: Add schema and testcase for domainsnapshot
docs/schemas/domainsnapshot.rng | 19 +++++
include/libvirt/libvirt-domain-snapshot.h | 10 ++-
include/libvirt/libvirt-domain.h | 1 +
src/conf/domain_conf.c | 6 +-
src/conf/domain_conf.h | 2 +
src/conf/snapshot_conf.c | 48
++++++++++++-
src/conf/snapshot_conf.h | 1 +
src/qemu/qemu_driver.c | 33 ++++++++-
.../full_domain_withinactive.xml | 83
++++++++++++++++++++++
tests/domainsnapshotxml2xmltest.c | 1 +
tools/virsh-snapshot.c | 20 ++++++
tools/virsh.pod | 37 +++++++++-
12 files changed, 251 insertions(+), 10 deletions(-)
create mode 100644
tests/domainsnapshotxml2xmlout/full_domain_withinactive.xml
Could someone please review the patch set? Jiri mentioned in early
December that it was on his list; perhaps this fell through the cracks
or got displaced by the (admittedly more urgent) Spectre/Meltdown
patches.
8:17 a.m.
On Thu, Jan 11, 2018 at 13:41:39 -0600, Scott Garfinkle wrote:
On Mon, 2017-10-30 at 14:21 +0530, Kothapally Madhu Pavan wrote:
> Restoring to a snapshot should not overwrite the persistent XML
> configuration
> of a snapshot as a side effect. This patchset fixes the same.
> Currently,
> virDomainSnapshotDef only saves active domain definition of the
> guest.
> And on restore the active domain definition is used as both active
> and
> inactive domain definitions. This will make the non-persistent
> changes
> persistent in snapshot image. This patchset allows to save inactive
> domain
> definition as well and on snapshot-revert non-persistent
> configuration is
> restored as is.
>
> Currently, snapshot-revert is making non-presistent changes as
> persistent.
> Here are the steps to reproduce.
> Step1: virsh define $dom
> Step2: virsh attach-device $dom $memory-device.xml --live
> Step3: virsh snapshot-create $dom
> Step4: virsh destroy $dom
> Step5: virsh snapshot-revert $dom $snapshot-name
> Step6: virsh destroy $dom
> Step7: virsh start $dom
> Here we still have $memory-device attached in Step2.
>
> This patchset is attempting to solve this issue. This patchset will
> also
> allow user to dump and edit inactive XML configuration of a snapshot.
> Dumping inactive domain definition of a snapshot is important as
> --redefine uses snapshot-dumpxml output to redefine a snapshot.
>
> Kothapally Madhu Pavan (7):
> qemu: Store inactive domain configuration in snapshot
> qemu: Use active and inactive snapshot configuration on restore
> conf: Allow editing inactive snapshot configuration
> virsh: Dump inactive XML configuration of snapshot using
> snapshot-dumpxml
> virsh: Edit inactive XML configuration of snapshot using snapshot-
> edit
> virsh: Allow restoring snapshot with non-persistent configuration
> tests: docs: Add schema and testcase for domainsnapshot
>
> docs/schemas/domainsnapshot.rng | 19 +++++
> include/libvirt/libvirt-domain-snapshot.h | 10 ++-
> include/libvirt/libvirt-domain.h | 1 +
> src/conf/domain_conf.c | 6 +-
> src/conf/domain_conf.h | 2 +
> src/conf/snapshot_conf.c | 48
> ++++++++++++-
> src/conf/snapshot_conf.h | 1 +
> src/qemu/qemu_driver.c | 33 ++++++++-
> .../full_domain_withinactive.xml | 83
> ++++++++++++++++++++++
> tests/domainsnapshotxml2xmltest.c | 1 +
> tools/virsh-snapshot.c | 20 ++++++
> tools/virsh.pod | 37 +++++++++-
> 12 files changed, 251 insertions(+), 10 deletions(-)
> create mode 100644
> tests/domainsnapshotxml2xmlout/full_domain_withinactive.xml
Could someone please review the patch set? Jiri mentioned in early
December that it was on his list; perhaps this fell through the cracks
or got displaced by the (admittedly more urgent) Spectre/Meltdown
Yeah, more urgent stuff appeared at that time. However, John replied
with several comments and the author didn't address any of them by
either sending a v2 or replying to John's emails or (preferably) both.
Jirka
2508
days inactive
2609
days old
16 comments
4 participants
participants (4)
-
Jiri Denemark -
John Ferlan -
Kothapally Madhu Pavan -
Scott Garfinkle