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@xxxxxxxxxxxxxxxxxx>
> ---
> 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);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
