Re: [libvirt] [PATCH 2/2] Add tests to xmconfigtest
Chun Yan Liu wrote:
On 12/23/2014 at 09:42 AM, in message <5498C888.6000903@suse.com>, Jim Fehlig
<jfehlig@suse.com> wrote:
Chunyan Liu wrote:
Hi Chunyan,
Thanks for the fix, and the test! But I have a few questions regarding the latter...
Add tests to testing HVM default features (pae, acpi, apic) conversion from xm config to libvirt xml and from libvirt xml to xm config.
Signed-off-by: Chunyan Liu <cyliu@suse.com> --- .../xmconfigdata/test-fullvirt-default-feature.cfg | 23 +++++++++++ .../test-fullvirt-default-feature.cfg.out | 26 ++++++++++++ .../xmconfigdata/test-fullvirt-default-feature.xml | 48
++++++++++++++++++++++
tests/xmconfigtest.c | 9 +++- 4 files changed, 105 insertions(+), 1 deletion(-) create mode 100644 tests/xmconfigdata/test-fullvirt-default-feature.cfg create mode 100644 tests/xmconfigdata/test-fullvirt-default-feature.cfg.out create mode 100644 tests/xmconfigdata/test-fullvirt-default-feature.xml
diff --git a/tests/xmconfigdata/test-fullvirt-default-feature.cfg
b/tests/xmconfigdata/test-fullvirt-default-feature.cfg
new file mode 100644 index 0000000..5ce234f --- /dev/null +++ b/tests/xmconfigdata/test-fullvirt-default-feature.cfg
Why is this file needed?
" Here we are testing default value conversion. That is: if there is no apci/pae/apic specified in xm config, after conversion, libvirt xml should include: <features> <apic/> <acpi/> <pae/> </features>
Ah, ok. Agreed that this test is missing.
So, from xm config -> xml, the cfg file should be like this.
@@ -0,0 +1,23 @@ +name = "XenGuest2" +uuid = "c7a5fdb2-cdaf-9455-926a-d65c16db1809 +maxmem = 579 +memory = 394 +vcpus = 1 +builder = "hvm" +kernel = "/usr/lib/xen/boot/hvmloader" +boot = "d" +hpet = 1 +localtime = 0 +on_poweroff = "destroy" +on_reboot = "restart" +on_crash = "restart" +device_model = "/usr/lib/xen/bin/qemu-dm" +sdl = 0 +vnc = 1 +vncunused = 1 +vnclisten = "127.0.0.1" +vncpasswd = "123poi" +vif = [
"mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000,type=ioem u" ]
+parallel = "none" +serial = "none" +disk = [ "phy:/dev/HostVG/XenGuest2,hda,w",
"file:/root/boot.iso,hdc:cdrom,r" ]
diff --git a/tests/xmconfigdata/test-fullvirt-default-feature.cfg.out
b/tests/xmconfigdata/test-fullvirt-default-feature.cfg.out
new file mode 100644 index 0000000..97a9827 --- /dev/null +++ b/tests/xmconfigdata/test-fullvirt-default-feature.cfg.out
IMO, this file should be renamed to 'test-fullvirt-default-feature.cfg'.
And from xml -> xm config, if in xml there is: <features> <apic/> <acpi/> <pae/> </features> Then after conversion, in xm config out file there will be explicitly: acpi=1 apic=1 pae=1
So, thlis is a little different from test-fullvirt-default-feature.cfg.
This is actually tested in all of the other test-fullvirt-* tests. I don't think we need an explicit test for it.
@@ -0,0 +1,26 @@ +name = "XenGuest2" +uuid = "c7a5fdb2-cdaf-9455-926a-d65c16db1809" +maxmem = 579 +memory = 394 +vcpus = 1 +builder = "hvm" +kernel = "/usr/lib/xen/boot/hvmloader" +boot = "d" +pae = 1 +acpi = 1 +apic = 1 +hpet = 1 +localtime = 0 +on_poweroff = "destroy" +on_reboot = "restart" +on_crash = "restart" +device_model = "/usr/lib/xen/bin/qemu-dm" +sdl = 0 +vnc = 1 +vncunused = 1 +vnclisten = "127.0.0.1" +vncpasswd = "123poi" +vif = [
"mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000,type=ioem u" ]
+parallel = "none" +serial = "none" +disk = [ "phy:/dev/HostVG/XenGuest2,hda,w",
"file:/root/boot.iso,hdc:cdrom,r" ]
diff --git a/tests/xmconfigdata/test-fullvirt-default-feature.xml
b/tests/xmconfigdata/test-fullvirt-default-feature.xml
new file mode 100644 index 0000000..57a6531 --- /dev/null +++ b/tests/xmconfigdata/test-fullvirt-default-feature.xml @@ -0,0 +1,48 @@ +<domain type='xen'> + <name>XenGuest2</name> + <uuid>c7a5fdb2-cdaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>592896</memory> + <currentMemory unit='KiB'>403456</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='xenfv'>hvm</type> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> + <boot dev='cdrom'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc' adjustment='reset'> + <timer name='hpet' present='yes'/> + </clock> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/lib/xen/bin/qemu-dm</emulator> + <disk type='block' device='disk'> + <driver name='phy'/> + <source dev='/dev/HostVG/XenGuest2'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='file' device='cdrom'> + <driver name='file'/> + <source file='/root/boot.iso'/> + <target dev='hdc' bus='ide'/> + <readonly/> + </disk> + <interface type='bridge'> + <mac address='00:16:3e:66:92:9c'/> + <source bridge='xenbr1'/> + <script path='vif-bridge'/> + <model type='e1000'/> + </interface> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='vnc' port='-1' autoport='yes' listen='127.0.0.1'
passwd='123poi'>
+ <listen type='address' address='127.0.0.1'/> + </graphics> + </devices> +</domain> diff --git a/tests/xmconfigtest.c b/tests/xmconfigtest.c index 0c6f803..b0b7b30 100644 --- a/tests/xmconfigtest.c +++ b/tests/xmconfigtest.c @@ -176,21 +176,26 @@ testCompareHelper(const void *data) const struct testInfo *info = data; char *xml = NULL; char *cfg = NULL; + char *cfgout = NULL;
if (virAsprintf(&xml, "%s/xmconfigdata/test-%s.xml", abs_srcdir, info->name) < 0 || virAsprintf(&cfg, "%s/xmconfigdata/test-%s.cfg", + abs_srcdir, info->name) < 0 || + virAsprintf(&cfgout, "%s/xmconfigdata/test-%s.cfg.out", abs_srcdir, info->name) < 0) goto cleanup;
if (info->mode == 0) - result = testCompareParseXML(cfg, xml, info->version); + result = testCompareParseXML(virFileExists(cfgout) ? cfgout : cfg, + xml, info->version); else result = testCompareFormatXML(cfg, xml, info->version);
cleanup: VIR_FREE(xml); VIR_FREE(cfg); + VIR_FREE(cfgout);
With the change suggested above, this hunk can be removed from xmconfigtest.c. 'make check' passes for me with these changes.
'make check' could pass, since it explicitly specify acpi|pae|apic=1 in xm config, and explicitly include those features in xml. But our interested case is not tested, what we are trying to test is: if user doesn't specify acpi|pae|apic, xml should by default include those features.
Yep, understood. Unlike the existing tests, in this case we only want to test xm -> xml conversion. I think a better solution would be to introduce DO_TEST_PARSE and DO_TEST_FORMAT macros, calling those in DO_TEST and individually when only needing to test one conversion. Regards, Jim
On 12/23/2014 at 11:13 AM, in message <5498DDCC.2020002@suse.com>, Jim Fehlig <jfehlig@suse.com> wrote: Chun Yan Liu wrote:
On 12/23/2014 at 09:42 AM, in message <5498C888.6000903@suse.com>, Jim Fehlig
<jfehlig@suse.com> wrote:
Chunyan Liu wrote:
Hi Chunyan,
Thanks for the fix, and the test! But I have a few questions regarding the latter...
Add tests to testing HVM default features (pae, acpi, apic) conversion from xm config to libvirt xml and from libvirt xml to xm config.
Signed-off-by: Chunyan Liu <cyliu@suse.com> --- .../xmconfigdata/test-fullvirt-default-feature.cfg | 23 +++++++++++ .../test-fullvirt-default-feature.cfg.out | 26 ++++++++++++ .../xmconfigdata/test-fullvirt-default-feature.xml | 48
++++++++++++++++++++++
tests/xmconfigtest.c | 9 +++- 4 files changed, 105 insertions(+), 1 deletion(-) create mode 100644 tests/xmconfigdata/test-fullvirt-default-feature.cfg create mode 100644 tests/xmconfigdata/test-fullvirt-default-feature.cfg.out create mode 100644 tests/xmconfigdata/test-fullvirt-default-feature.xml
diff --git a/tests/xmconfigdata/test-fullvirt-default-feature.cfg
b/tests/xmconfigdata/test-fullvirt-default-feature.cfg
new file mode 100644 index 0000000..5ce234f --- /dev/null +++ b/tests/xmconfigdata/test-fullvirt-default-feature.cfg
Why is this file needed?
" Here we are testing default value conversion. That is: if there is no apci/pae/apic specified in xm config, after conversion, libvirt xml should include: <features> <apic/> <acpi/> <pae/> </features>
Ah, ok. Agreed that this test is missing.
So, from xm config -> xml, the cfg file should be like this.
@@ -0,0 +1,23 @@ +name = "XenGuest2" +uuid = "c7a5fdb2-cdaf-9455-926a-d65c16db1809 +maxmem = 579 +memory = 394 +vcpus = 1 +builder = "hvm" +kernel = "/usr/lib/xen/boot/hvmloader" +boot = "d" +hpet = 1 +localtime = 0 +on_poweroff = "destroy" +on_reboot = "restart" +on_crash = "restart" +device_model = "/usr/lib/xen/bin/qemu-dm" +sdl = 0 +vnc = 1 +vncunused = 1 +vnclisten = "127.0.0.1" +vncpasswd = "123poi" +vif = [
"mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000,type=ioem
u" ]
+parallel = "none" +serial = "none" +disk = [ "phy:/dev/HostVG/XenGuest2,hda,w",
"file:/root/boot.iso,hdc:cdrom,r" ]
diff --git a/tests/xmconfigdata/test-fullvirt-default-feature.cfg.out
b/tests/xmconfigdata/test-fullvirt-default-feature.cfg.out
new file mode 100644 index 0000000..97a9827 --- /dev/null +++ b/tests/xmconfigdata/test-fullvirt-default-feature.cfg.out
IMO, this file should be renamed to 'test-fullvirt-default-feature.cfg'.
And from xml -> xm config, if in xml there is: <features> <apic/> <acpi/> <pae/> </features> Then after conversion, in xm config out file there will be explicitly: acpi=1 apic=1 pae=1
So, thlis is a little different from test-fullvirt-default-feature.cfg.
This is actually tested in all of the other test-fullvirt-* tests. I don't think we need an explicit test for it.
@@ -0,0 +1,26 @@ +name = "XenGuest2" +uuid = "c7a5fdb2-cdaf-9455-926a-d65c16db1809" +maxmem = 579 +memory = 394 +vcpus = 1 +builder = "hvm" +kernel = "/usr/lib/xen/boot/hvmloader" +boot = "d" +pae = 1 +acpi = 1 +apic = 1 +hpet = 1 +localtime = 0 +on_poweroff = "destroy" +on_reboot = "restart" +on_crash = "restart" +device_model = "/usr/lib/xen/bin/qemu-dm" +sdl = 0 +vnc = 1 +vncunused = 1 +vnclisten = "127.0.0.1" +vncpasswd = "123poi" +vif = [
"mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000,type=ioem
u" ]
+parallel = "none" +serial = "none" +disk = [ "phy:/dev/HostVG/XenGuest2,hda,w",
"file:/root/boot.iso,hdc:cdrom,r" ]
diff --git a/tests/xmconfigdata/test-fullvirt-default-feature.xml
b/tests/xmconfigdata/test-fullvirt-default-feature.xml
new file mode 100644 index 0000000..57a6531 --- /dev/null +++ b/tests/xmconfigdata/test-fullvirt-default-feature.xml @@ -0,0 +1,48 @@ +<domain type='xen'> + <name>XenGuest2</name> + <uuid>c7a5fdb2-cdaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>592896</memory> + <currentMemory unit='KiB'>403456</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='xenfv'>hvm</type> + <loader type='rom'>/usr/lib/xen/boot/hvmloader</loader> + <boot dev='cdrom'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc' adjustment='reset'> + <timer name='hpet' present='yes'/> + </clock> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/lib/xen/bin/qemu-dm</emulator> + <disk type='block' device='disk'> + <driver name='phy'/> + <source dev='/dev/HostVG/XenGuest2'/> + <target dev='hda' bus='ide'/> + </disk> + <disk type='file' device='cdrom'> + <driver name='file'/> + <source file='/root/boot.iso'/> + <target dev='hdc' bus='ide'/> + <readonly/> + </disk> + <interface type='bridge'> + <mac address='00:16:3e:66:92:9c'/> + <source bridge='xenbr1'/> + <script path='vif-bridge'/> + <model type='e1000'/> + </interface> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <graphics type='vnc' port='-1' autoport='yes' listen='127.0.0.1'
passwd='123poi'>
+ <listen type='address' address='127.0.0.1'/> + </graphics> + </devices> +</domain> diff --git a/tests/xmconfigtest.c b/tests/xmconfigtest.c index 0c6f803..b0b7b30 100644 --- a/tests/xmconfigtest.c +++ b/tests/xmconfigtest.c @@ -176,21 +176,26 @@ testCompareHelper(const void *data) const struct testInfo *info = data; char *xml = NULL; char *cfg = NULL; + char *cfgout = NULL;
if (virAsprintf(&xml, "%s/xmconfigdata/test-%s.xml", abs_srcdir, info->name) < 0 || virAsprintf(&cfg, "%s/xmconfigdata/test-%s.cfg", + abs_srcdir, info->name) < 0 || + virAsprintf(&cfgout, "%s/xmconfigdata/test-%s.cfg.out", abs_srcdir, info->name) < 0) goto cleanup;
if (info->mode == 0) - result = testCompareParseXML(cfg, xml, info->version); + result = testCompareParseXML(virFileExists(cfgout) ? cfgout : cfg,
+ xml, info->version); else result = testCompareFormatXML(cfg, xml, info->version);
cleanup: VIR_FREE(xml); VIR_FREE(cfg); + VIR_FREE(cfgout);
With the change suggested above, this hunk can be removed from xmconfigtest.c. 'make check' passes for me with these changes.
'make check' could pass, since it explicitly specify acpi|pae|apic=1 in xm config, and explicitly include those features in xml. But our interested case is not tested, what we are trying to test is: if user doesn't specify acpi|pae|apic, xml should by default include those features.
Yep, understood. Unlike the existing tests, in this case we only want to test xm -> xml conversion. I think a better solution would be to introduce DO_TEST_PARSE and DO_TEST_FORMAT macros, calling those in DO_TEST and individually when only needing to test one conversion.
That's a good idea. I'll update. Thanks very much for reviewing the patch in your holiday! - Chunyan
Regards, Jim
participants (2)
-
Chun Yan Liu -
Jim Fehlig