>>> On 12/23/2014 at 11:13 AM, in message <5498DDCC.2020002@xxxxxxxx>, Jim Fehlig <jfehlig@xxxxxxxx> wrote: > Chun Yan Liu wrote: > > > >>>> On 12/23/2014 at 09:42 AM, in message <5498C888.6000903@xxxxxxxx>, Jim Fehlig > >>>> > > <jfehlig@xxxxxxxx> 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@xxxxxxxx> > >>> --- > >>> .../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 > > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list