| Message ID | 6f9a1738d7d6d824c00c08191e9ff50a2784cdb6.1554137098.git.crobinso@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | tests: qemuxml2xml: add DO_TEST_CAPS* | expand |
On Mon, 2019-04-01 at 12:47 -0400, Cole Robinson wrote: > Add DO_TEST_CAPS* macros, lifted from qemuxml2argvtest. Use it on > a few recently added xml2xml tests that use DO_TEST_CAPS in the argv > test case. The firmware examples require breaking the symlink and > creating our own test file. Also add a test for os-firmware-efi which > seems to have been missed. Please have each of these changes as separate preparatory commits. > One subtle difference compared to qemuxml2argv is output file naming. > qemuxml2xml uses a system where if specially named files > ${basename}-active.xml or ${basename}-inactive.xml exist, those are > used as output, otherwise just ${basename}.xml is used. I'm not quite > sure how to make this fit with the caps suffix naming scheme used > in qemuxml2argv, where for example DO_CAPS_LATEST will always add a > -latest suffix to basename. > > This code by default will store the output in ${basename}.xml with no > -latest suffix. This makes it easier to convert DO_TEST calls to CAPS > variants, because it won't require any test file rename/removal, but if > we ever want to add more than one qemuxml2xml output for a single input, > it will require special file naming to not collide. IMO that's not a > big deal as it follows the existing -active pattern. But it's a > divergence from qemuxml2argv behavior More on this later. [...] > static int > testInfoSetPaths(struct testQemuInfo *info, > const char *name, > - int when) > + int when, > + const char *suffix) 'suffix' should come before 'when', to match the corresponding function in xml2argv. Additionally, we're passing 'name' to the function here but we're storing it inside 'info' in xml2argv - we should be doing the same here. Please make that change as a separate preparatory commit. [...] > if (virAsprintf(&info->outfile, > - "%s/qemuxml2xmloutdata/%s-%s.xml", > + "%s/qemuxml2xmloutdata/%s-%s%s.xml", > abs_srcdir, name, > - when == WHEN_ACTIVE ? "active" : "inactive") < 0) > + when == WHEN_ACTIVE ? "active" : "inactive", > + suffix) < 0) > goto error; > > if (!virFileExists(info->outfile)) { Missing from the context, but immediately after this line we have: if (virAsprintf(&info->outfile, "%s/qemuxml2xmloutdata/%s.xml", abs_srcdir, name) < 0) ... We should format 'suffix' here too. --- Following up from the commit message: I was wondering about the way this test works, and discussing it with Jano (CC'd since he had his own opinion on the matter). There are several situations we can run into with these tests: 1) we test WHEN_BOTH and the output for the WHEN_ACTIVE and WHEN_INACTIVE cases match; 2) same as the above, but the outputs don't match; 3) we test only one of WHEN_ACTIVE and WHEN_INACTIVE. Case 1) covers the vast majority of existing test cases. As for output files, I would expect respectively 1) a single output file, with no suffix; 2) two output files with different suffixes; 3) a single output file with the corresponding -inactive or -active suffix. The problem with the current algorithm is that, when VIR_TEST_REGENERATE_OUTPUT is used, and you're introducing a new test case so no output files exist yet, you'll end up with 1) the expected output, yay!; 2) failure, because both WHEN variants will write the (different) output to the unsuffixed file and step on each other's toes every time. This is annoying but ultimately okay, because the developer can break the loop simply by touching the suffixed files before running the test program; 3) the unsuffixed file being created instead of the suffixed one. The third scenario is suboptimal but not necessarily a very big deal either. One way to get rid of the above would be to pass an extra flags that controls whether falling back to the unsuffixed output files should be considered at all: the default would be to pass it for WHEN_BOTH, so that scenario 1) would be covered, and not to pass it for WHEN_ACTIVE and WHEN_INACTIVE to take care of scenario 3); the few test cases that fall into scenario 2) would have to go for a more verbose macro and *not* pass the flag manually. I feel like that would be acceptable given that most tests cases fall into 1) anyway. --- None of the above is really connected to whether or not we should use 'suffix' as I suggested earlier: we should definitely format it, even if it causes test suite churn. Not only that: you should also make sure... [...] > +# define DO_TEST_CAPS_INTERNAL(name, arch, ver, ...) \ > + DO_TEST_INTERNAL(name, "." arch "-" ver, WHEN_BOTH, \ > + ARG_CAPS_ARCH, arch, \ > + ARG_CAPS_VER, ver, \ > + __VA_ARGS__) ... you copy DO_TEST_CAPS_ARCH_VER() and DO_TEST_CAPS_VER() from xml2argv too, so that you can later introduce... [...] > + DO_TEST_CAPS_LATEST("virtio-transitional"); > + DO_TEST_CAPS_LATEST("virtio-non-transitional"); DO_TEST_CAPS_VER("virtio-transitional", "3.1.0"); DO_TEST_CAPS_VER("virtio-non-transitional", "3.1.0"); and friends like in xml2argv, too. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Apr 11, 2019 at 12:44:16PM +0200, Andrea Bolognani wrote: >On Mon, 2019-04-01 at 12:47 -0400, Cole Robinson wrote: >> Add DO_TEST_CAPS* macros, lifted from qemuxml2argvtest. Use it on >> a few recently added xml2xml tests that use DO_TEST_CAPS in the argv >> test case. The firmware examples require breaking the symlink and >> creating our own test file. Also add a test for os-firmware-efi which >> seems to have been missed. > >Please have each of these changes as separate preparatory commits. > >> One subtle difference compared to qemuxml2argv is output file naming. >> qemuxml2xml uses a system where if specially named files >> ${basename}-active.xml or ${basename}-inactive.xml exist, those are >> used as output, otherwise just ${basename}.xml is used. I'm not quite >> sure how to make this fit with the caps suffix naming scheme used >> in qemuxml2argv, where for example DO_CAPS_LATEST will always add a >> -latest suffix to basename. >> >> This code by default will store the output in ${basename}.xml with no >> -latest suffix. This makes it easier to convert DO_TEST calls to CAPS >> variants, because it won't require any test file rename/removal, but if >> we ever want to add more than one qemuxml2xml output for a single input, >> it will require special file naming to not collide. IMO that's not a >> big deal as it follows the existing -active pattern. But it's a >> divergence from qemuxml2argv behavior > >More on this later. > >[...] >> static int >> testInfoSetPaths(struct testQemuInfo *info, >> const char *name, >> - int when) >> + int when, >> + const char *suffix) > >'suffix' should come before 'when', to match the corresponding >function in xml2argv. > >Additionally, we're passing 'name' to the function here but we're >storing it inside 'info' in xml2argv - we should be doing the same >here. Please make that change as a separate preparatory commit. > >[...] >> if (virAsprintf(&info->outfile, >> - "%s/qemuxml2xmloutdata/%s-%s.xml", >> + "%s/qemuxml2xmloutdata/%s-%s%s.xml", I'd definitely put another minus between these suffixes (also, I'd like to see them in use). >> abs_srcdir, name, >> - when == WHEN_ACTIVE ? "active" : "inactive") < 0) >> + when == WHEN_ACTIVE ? "active" : "inactive", >> + suffix) < 0) >> goto error; >> >> if (!virFileExists(info->outfile)) { As for this virFileExists - I really dislike it. It is on my TODO list(TM) to change this to either: A) specify exactly which output files the test needs by using the appropriate DO_TEST macro or B) add a lot of symlinks for the expected output to the output directory. > >Missing from the context, but immediately after this line we have: > > if (virAsprintf(&info->outfile, > "%s/qemuxml2xmloutdata/%s.xml", > abs_srcdir, name) < 0) > ... > >We should format 'suffix' here too. > >--- > >Following up from the commit message: I was wondering about the way >this test works, and discussing it with Jano (CC'd since he had his >own opinion on the matter). > >There are several situations we can run into with these tests: > > 1) we test WHEN_BOTH and the output for the WHEN_ACTIVE and > WHEN_INACTIVE cases match; > > 2) same as the above, but the outputs don't match; > > 3) we test only one of WHEN_ACTIVE and WHEN_INACTIVE. > >Case 1) covers the vast majority of existing test cases. > >As for output files, I would expect respectively > > 1) a single output file, with no suffix; > > 2) two output files with different suffixes; > > 3) a single output file with the corresponding -inactive or > -active suffix. > >The problem with the current algorithm is that, when >VIR_TEST_REGENERATE_OUTPUT is used, and you're introducing a new >test case so no output files exist yet, you'll end up with > > 1) the expected output, yay!; > > 2) failure, because both WHEN variants will write the (different) > output to the unsuffixed file and step on each other's toes > every time. This is annoying but ultimately okay, because the > developer can break the loop simply by touching the suffixed > files before running the test program; > > 3) the unsuffixed file being created instead of the suffixed one. > >The third scenario is suboptimal but not necessarily a very big deal >either. > >One way to get rid of the above would be to pass an extra flags that >controls whether falling back to the unsuffixed output files should >be considered at all: the default would be to pass it for WHEN_BOTH, >so that scenario 1) would be covered, and not to pass it for >WHEN_ACTIVE and WHEN_INACTIVE to take care of scenario 3); the few >test cases that fall into scenario 2) would have to go for a more >verbose macro and *not* pass the flag manually. I feel like that >would be acceptable given that most tests cases fall into 1) anyway. > >--- > >None of the above is really connected to whether or not we should >use 'suffix' as I suggested earlier: we should definitely format it, >even if it causes test suite churn. Not only that: you should also >make sure... > The reason for the suffix in xml2argv is to allow the CAPS_LATEST tests to coexist with the ones with enumerated capabilities. But it also contains the architecture, so even if -latest would be the prevailing case, I'd rather format it anyway. Jano >[...] >> +# define DO_TEST_CAPS_INTERNAL(name, arch, ver, ...) \ >> + DO_TEST_INTERNAL(name, "." arch "-" ver, WHEN_BOTH, \ >> + ARG_CAPS_ARCH, arch, \ >> + ARG_CAPS_VER, ver, \ >> + __VA_ARGS__) > >... you copy DO_TEST_CAPS_ARCH_VER() and DO_TEST_CAPS_VER() from >xml2argv too, so that you can later introduce... > >[...] >> + DO_TEST_CAPS_LATEST("virtio-transitional"); >> + DO_TEST_CAPS_LATEST("virtio-non-transitional"); > > DO_TEST_CAPS_VER("virtio-transitional", "3.1.0"); > DO_TEST_CAPS_VER("virtio-non-transitional", "3.1.0"); > >and friends like in xml2argv, too. > >-- >Andrea Bolognani / Red Hat / Virtualization > >-- >libvir-list mailing list >libvir-list@redhat.com >https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 2019-04-11 at 14:15 +0200, Ján Tomko wrote: > On Thu, Apr 11, 2019 at 12:44:16PM +0200, Andrea Bolognani wrote: > > On Mon, 2019-04-01 at 12:47 -0400, Cole Robinson wrote: > > > if (virAsprintf(&info->outfile, > > > - "%s/qemuxml2xmloutdata/%s-%s.xml", > > > + "%s/qemuxml2xmloutdata/%s-%s%s.xml", > > I'd definitely put another minus between these suffixes (also, I'd like > to see them in use). Looking at xml2argv, and as I suggested the implementation should be the same in xml2xml, we have # define DO_TEST_CAPS_INTERNAL(name, arch, ver, ...) \ DO_TEST_INTERNAL(name, "." arch "-" ver, \ ARG_CAPS_ARCH, arch, \ ARG_CAPS_VER, ver, \ __VA_ARGS__) # define DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, ...) \ DO_TEST_CAPS_INTERNAL(name, arch, "latest", __VA_ARGS__) # define DO_TEST_CAPS_ARCH_LATEST(name, arch) \ DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, ARG_END) # define DO_TEST_CAPS_LATEST(name) \ DO_TEST_CAPS_ARCH_LATEST(name, "x86_64") so when you use DO_TEST_CAPS_LATEST("virtio-transitional"); 'suffix' will be ".x86_64-latest" and the resulting filename will be qemuxml2xmloutdata/virtio-transitional.x86_64-latest.xml If you had a similar test for WHEN_ACTIVE only, then you'd get qemuxml2xmloutdata/virtio-transitional-active.x86_64-latest.xml It doesn't look to me like any extra separators are necessary. [...] > > None of the above is really connected to whether or not we should > > use 'suffix' as I suggested earlier: we should definitely format it, > > even if it causes test suite churn. Not only that: you should also > > make sure... > > The reason for the suffix in xml2argv is to allow the CAPS_LATEST tests > to coexist with the ones with enumerated capabilities. > > But it also contains the architecture, so even if -latest would be the > prevailing case, I'd rather format it anyway. Looks like we agree then :)
On 4/11/19 8:15 AM, Ján Tomko wrote: > On Thu, Apr 11, 2019 at 12:44:16PM +0200, Andrea Bolognani wrote: >> On Mon, 2019-04-01 at 12:47 -0400, Cole Robinson wrote: >>> Add DO_TEST_CAPS* macros, lifted from qemuxml2argvtest. Use it on >>> a few recently added xml2xml tests that use DO_TEST_CAPS in the argv >>> test case. The firmware examples require breaking the symlink and >>> creating our own test file. Also add a test for os-firmware-efi which >>> seems to have been missed. >> >> Please have each of these changes as separate preparatory commits. >> >>> One subtle difference compared to qemuxml2argv is output file naming. >>> qemuxml2xml uses a system where if specially named files >>> ${basename}-active.xml or ${basename}-inactive.xml exist, those are >>> used as output, otherwise just ${basename}.xml is used. I'm not quite >>> sure how to make this fit with the caps suffix naming scheme used >>> in qemuxml2argv, where for example DO_CAPS_LATEST will always add a >>> -latest suffix to basename. >>> >>> This code by default will store the output in ${basename}.xml with no >>> -latest suffix. This makes it easier to convert DO_TEST calls to CAPS >>> variants, because it won't require any test file rename/removal, but if >>> we ever want to add more than one qemuxml2xml output for a single input, >>> it will require special file naming to not collide. IMO that's not a >>> big deal as it follows the existing -active pattern. But it's a >>> divergence from qemuxml2argv behavior >> >> More on this later. >> >> [...] >>> static int >>> testInfoSetPaths(struct testQemuInfo *info, >>> const char *name, >>> - int when) >>> + int when, >>> + const char *suffix) >> >> 'suffix' should come before 'when', to match the corresponding >> function in xml2argv. >> >> Additionally, we're passing 'name' to the function here but we're >> storing it inside 'info' in xml2argv - we should be doing the same >> here. Please make that change as a separate preparatory commit. >> >> [...] >>> if (virAsprintf(&info->outfile, >>> - "%s/qemuxml2xmloutdata/%s-%s.xml", >>> + "%s/qemuxml2xmloutdata/%s-%s%s.xml", > > I'd definitely put another minus between these suffixes (also, I'd like > to see them in use). > Sure I'll add example usage for this in the next version >>> abs_srcdir, name, >>> - when == WHEN_ACTIVE ? "active" : "inactive") < 0) >>> + when == WHEN_ACTIVE ? "active" : "inactive", >>> + suffix) < 0) >>> goto error; >>> >>> if (!virFileExists(info->outfile)) { > > As for this virFileExists - I really dislike it. It is on my TODO > list(TM) to change this to either: > A) specify exactly which output files the test needs by using the > appropriate DO_TEST macro > or > B) add a lot of symlinks for the expected output to the output > directory. > Agreed that this should go away. I think the whole WHEN_X concept should go away: always test active and inactive paths, but the only DO_TEST distinction is active vs inactive output is expected to be the same, vs expected to be different. The latter case should mandate that -active and -inactive filesnames exist, the former case doesn't look for any state suffix. I think only the few WEHN_INACTIVE cases will need some extra output but otherwise it's pretty compatible with the current state of things. >> >> Missing from the context, but immediately after this line we have: >> >> if (virAsprintf(&info->outfile, >> "%s/qemuxml2xmloutdata/%s.xml", >> abs_srcdir, name) < 0) >> ... >> >> We should format 'suffix' here too. >> >> --- >> >> Following up from the commit message: I was wondering about the way >> this test works, and discussing it with Jano (CC'd since he had his >> own opinion on the matter). >> >> There are several situations we can run into with these tests: >> >> 1) we test WHEN_BOTH and the output for the WHEN_ACTIVE and >> WHEN_INACTIVE cases match; >> >> 2) same as the above, but the outputs don't match; >> >> 3) we test only one of WHEN_ACTIVE and WHEN_INACTIVE. >> >> Case 1) covers the vast majority of existing test cases. >> >> As for output files, I would expect respectively >> >> 1) a single output file, with no suffix; >> >> 2) two output files with different suffixes; >> >> 3) a single output file with the corresponding -inactive or >> -active suffix. >> >> The problem with the current algorithm is that, when >> VIR_TEST_REGENERATE_OUTPUT is used, and you're introducing a new >> test case so no output files exist yet, you'll end up with >> >> 1) the expected output, yay!; >> >> 2) failure, because both WHEN variants will write the (different) >> output to the unsuffixed file and step on each other's toes >> every time. This is annoying but ultimately okay, because the >> developer can break the loop simply by touching the suffixed >> files before running the test program; >> >> 3) the unsuffixed file being created instead of the suffixed one. >> >> The third scenario is suboptimal but not necessarily a very big deal >> either. >> >> One way to get rid of the above would be to pass an extra flags that >> controls whether falling back to the unsuffixed output files should >> be considered at all: the default would be to pass it for WHEN_BOTH, >> so that scenario 1) would be covered, and not to pass it for >> WHEN_ACTIVE and WHEN_INACTIVE to take care of scenario 3); the few >> test cases that fall into scenario 2) would have to go for a more >> verbose macro and *not* pass the flag manually. I feel like that >> would be acceptable given that most tests cases fall into 1) anyway. >> >> --- >> >> None of the above is really connected to whether or not we should >> use 'suffix' as I suggested earlier: we should definitely format it, >> even if it causes test suite churn. Not only that: you should also >> make sure... >> > > The reason for the suffix in xml2argv is to allow the CAPS_LATEST tests > to coexist with the ones with enumerated capabilities. > > But it also contains the architecture, so even if -latest would be the > prevailing case, I'd rather format it anyway. > Hopefully one day we get to a point that DO_TEST always uses latest caps, and we have a DO_TEST_CAPS_LIST for the explicit caps list... Thanks, Cole -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
diff --git a/tests/qemuxml2argvdata/vhost-vsock.xml b/tests/qemuxml2argvdata/vhost-vsock.xml index bc550ace4e..f594c53f48 100644 --- a/tests/qemuxml2argvdata/vhost-vsock.xml +++ b/tests/qemuxml2argvdata/vhost-vsock.xml @@ -15,7 +15,7 @@ <on_crash>restart</on_crash> <devices> <emulator>/usr/bin/qemu-system-x86_64</emulator> - <controller type='usb' index='0'> + <controller type='usb' index='0' model='piix3-uhci'> <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> </controller> <controller type='virtio-serial' index='0'> diff --git a/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml b/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml deleted file mode 120000 index beea6b2955..0000000000 --- a/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml +++ /dev/null @@ -1 +0,0 @@ -../qemuxml2argvdata/aarch64-os-firmware-efi.xml \ No newline at end of file diff --git a/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml b/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml new file mode 100644 index 0000000000..529ce6f3c2 --- /dev/null +++ b/tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml @@ -0,0 +1,31 @@ +<domain type='qemu'> + <name>aarch64test</name> + <uuid>496d7ea8-9739-544b-4ebd-ef08be936e8b</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <type arch='aarch64' machine='virt-4.0'>hvm</type> + <kernel>/aarch64.kernel</kernel> + <initrd>/aarch64.initrd</initrd> + <cmdline>earlyprintk console=ttyAMA0,115200n8 rw root=/dev/vda rootwait</cmdline> + <dtb>/aarch64.dtb</dtb> + <boot dev='hd'/> + </os> + <features> + <apic/> + <pae/> + <gic version='2'/> + </features> + <cpu mode='custom' match='exact' check='none'> + <model fallback='allow'>cortex-a53</model> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-aarch64</emulator> + <controller type='pci' index='0' model='pcie-root'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/os-firmware-bios.xml b/tests/qemuxml2xmloutdata/os-firmware-bios.xml deleted file mode 120000 index 3d36d5df68..0000000000 --- a/tests/qemuxml2xmloutdata/os-firmware-bios.xml +++ /dev/null @@ -1 +0,0 @@ -../qemuxml2argvdata/os-firmware-bios.xml \ No newline at end of file diff --git a/tests/qemuxml2xmloutdata/os-firmware-bios.xml b/tests/qemuxml2xmloutdata/os-firmware-bios.xml new file mode 100644 index 0000000000..63886666dd --- /dev/null +++ b/tests/qemuxml2xmloutdata/os-firmware-bios.xml @@ -0,0 +1,68 @@ +<domain type='kvm'> + <name>fedora</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>8192</memory> + <currentMemory unit='KiB'>8192</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='bios'> + <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> + <loader secure='no'/> + <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <pm> + <suspend-to-mem enabled='yes'/> + <suspend-to-disk enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='ich9-ehci1'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x7'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci1'> + <master startport='0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x0' multifunction='on'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci2'> + <master startport='2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x1'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci3'> + <master startport='4'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x2'/> + </controller> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='2'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='3' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml b/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml deleted file mode 120000 index 93e184e2d2..0000000000 --- a/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml +++ /dev/null @@ -1 +0,0 @@ -../qemuxml2argvdata/os-firmware-efi-secboot.xml \ No newline at end of file diff --git a/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml b/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml new file mode 100644 index 0000000000..a285e06334 --- /dev/null +++ b/tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml @@ -0,0 +1,68 @@ +<domain type='kvm'> + <name>fedora</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>8192</memory> + <currentMemory unit='KiB'>8192</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> + <loader secure='yes'/> + <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <pm> + <suspend-to-mem enabled='yes'/> + <suspend-to-disk enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='ich9-ehci1'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x7'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci1'> + <master startport='0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x0' multifunction='on'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci2'> + <master startport='2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x1'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci3'> + <master startport='4'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x2'/> + </controller> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='2'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='3' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/os-firmware-efi.xml b/tests/qemuxml2xmloutdata/os-firmware-efi.xml deleted file mode 120000 index 15cfad1ea0..0000000000 --- a/tests/qemuxml2xmloutdata/os-firmware-efi.xml +++ /dev/null @@ -1 +0,0 @@ -../qemuxml2argvdata/os-firmware-efi.xml \ No newline at end of file diff --git a/tests/qemuxml2xmloutdata/os-firmware-efi.xml b/tests/qemuxml2xmloutdata/os-firmware-efi.xml new file mode 100644 index 0000000000..46a7b1b780 --- /dev/null +++ b/tests/qemuxml2xmloutdata/os-firmware-efi.xml @@ -0,0 +1,68 @@ +<domain type='kvm'> + <name>fedora</name> + <uuid>63840878-0deb-4095-97e6-fc444d9bc9fa</uuid> + <memory unit='KiB'>8192</memory> + <currentMemory unit='KiB'>8192</currentMemory> + <vcpu placement='static'>1</vcpu> + <os firmware='efi'> + <type arch='x86_64' machine='pc-q35-4.0'>hvm</type> + <loader secure='no'/> + <nvram>/var/lib/libvirt/qemu/nvram/fedora_VARS.fd</nvram> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <pm> + <suspend-to-mem enabled='yes'/> + <suspend-to-disk enabled='no'/> + </pm> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0' model='ich9-ehci1'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x7'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci1'> + <master startport='0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x0' multifunction='on'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci2'> + <master startport='2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x1'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci3'> + <master startport='4'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1d' function='0x2'/> + </controller> + <controller type='sata' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pcie-root'/> + <controller type='pci' index='1' model='dmi-to-pci-bridge'> + <model name='i82801b11-bridge'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/> + </controller> + <controller type='pci' index='2' model='pci-bridge'> + <model name='pci-bridge'/> + <target chassisNr='2'/> + <address type='pci' domain='0x0000' bus='0x01' slot='0x00' function='0x0'/> + </controller> + <controller type='pci' index='3' model='pcie-root-port'> + <model name='ioh3420'/> + <target chassis='3' port='0x8'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x02' slot='0x01' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 3b5314df78..6ae3cbb11f 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -91,7 +91,8 @@ testCompareStatusXMLToXMLFiles(const void *opaque) static int testInfoSetPaths(struct testQemuInfo *info, const char *name, - int when) + int when, + const char *suffix) { VIR_FREE(info->infile); VIR_FREE(info->outfile); @@ -101,9 +102,10 @@ testInfoSetPaths(struct testQemuInfo *info, goto error; if (virAsprintf(&info->outfile, - "%s/qemuxml2xmloutdata/%s-%s.xml", + "%s/qemuxml2xmloutdata/%s-%s%s.xml", abs_srcdir, name, - when == WHEN_ACTIVE ? "active" : "inactive") < 0) + when == WHEN_ACTIVE ? "active" : "inactive", + suffix) < 0) goto error; if (!virFileExists(info->outfile)) { @@ -175,7 +177,7 @@ mymain(void) cfg = virQEMUDriverGetConfig(&driver); -# define DO_TEST_FULL(name, when, ...) \ +# define DO_TEST_INTERNAL(name, suffix, when, ...) \ do { \ if (testQemuInfoSetArgs(&info, capslatest, \ __VA_ARGS__, \ @@ -186,7 +188,7 @@ mymain(void) } \ \ if (when & WHEN_INACTIVE) { \ - if (testInfoSetPaths(&info, name, WHEN_INACTIVE) < 0) { \ + if (testInfoSetPaths(&info, name, WHEN_INACTIVE, suffix) < 0) { \ VIR_TEST_DEBUG("Failed to generate inactive paths for '%s'", name); \ return -1; \ } \ @@ -196,7 +198,7 @@ mymain(void) } \ \ if (when & WHEN_ACTIVE) { \ - if (testInfoSetPaths(&info, name, WHEN_ACTIVE) < 0) { \ + if (testInfoSetPaths(&info, name, WHEN_ACTIVE, suffix) < 0) { \ VIR_TEST_DEBUG("Failed to generate active paths for '%s'", name); \ return -1; \ } \ @@ -209,9 +211,26 @@ mymain(void) # define NONE QEMU_CAPS_LAST +# define DO_TEST_FULL(name, when, ...) \ + DO_TEST_INTERNAL(name, "", when, __VA_ARGS__) + # define DO_TEST(name, ...) \ DO_TEST_FULL(name, WHEN_BOTH, ARG_QEMU_CAPS, __VA_ARGS__, QEMU_CAPS_LAST) +# define DO_TEST_CAPS_INTERNAL(name, arch, ver, ...) \ + DO_TEST_INTERNAL(name, "." arch "-" ver, WHEN_BOTH, \ + ARG_CAPS_ARCH, arch, \ + ARG_CAPS_VER, ver, \ + __VA_ARGS__) + +# define DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, ...) \ + DO_TEST_CAPS_INTERNAL(name, arch, "latest", __VA_ARGS__) + +# define DO_TEST_CAPS_ARCH_LATEST(name, arch) \ + DO_TEST_CAPS_ARCH_LATEST_FULL(name, arch, ARG_END) + +# define DO_TEST_CAPS_LATEST(name) \ + DO_TEST_CAPS_ARCH_LATEST(name, "x86_64") /* Unset or set all envvars here that are copied in qemudBuildCommandLine @@ -979,36 +998,14 @@ mymain(void) DO_TEST("smbios", NONE); DO_TEST("smbios-multiple-type2", NONE); - DO_TEST("os-firmware-bios", - QEMU_CAPS_DEVICE_PCI_BRIDGE, - QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, - QEMU_CAPS_DEVICE_IOH3420, - QEMU_CAPS_ICH9_AHCI, - QEMU_CAPS_ICH9_USB_EHCI1, - QEMU_CAPS_DEVICE_VIDEO_PRIMARY, - QEMU_CAPS_DEVICE_QXL); - DO_TEST("os-firmware-efi", - QEMU_CAPS_DEVICE_PCI_BRIDGE, - QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, - QEMU_CAPS_DEVICE_IOH3420, - QEMU_CAPS_ICH9_AHCI, - QEMU_CAPS_ICH9_USB_EHCI1, - QEMU_CAPS_DEVICE_VIDEO_PRIMARY, - QEMU_CAPS_DEVICE_QXL); - DO_TEST("os-firmware-efi-secboot", - QEMU_CAPS_DEVICE_PCI_BRIDGE, - QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE, - QEMU_CAPS_DEVICE_IOH3420, - QEMU_CAPS_ICH9_AHCI, - QEMU_CAPS_ICH9_USB_EHCI1, - QEMU_CAPS_DEVICE_VIDEO_PRIMARY, - QEMU_CAPS_DEVICE_QXL); + DO_TEST_CAPS_LATEST("os-firmware-bios"); + DO_TEST_CAPS_LATEST("os-firmware-efi"); + DO_TEST_CAPS_LATEST("os-firmware-efi-secboot"); DO_TEST("aarch64-aavmf-virtio-mmio", QEMU_CAPS_DEVICE_VIRTIO_MMIO, QEMU_CAPS_DEVICE_VIRTIO_RNG, QEMU_CAPS_OBJECT_RNG_RANDOM); - DO_TEST("aarch64-os-firmware-efi", - QEMU_CAPS_DEVICE_VIRTIO_MMIO); + DO_TEST_CAPS_ARCH_LATEST("aarch64-os-firmware-efi", "aarch64"); DO_TEST("aarch64-virtio-pci-default", QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY, QEMU_CAPS_DEVICE_VIRTIO_MMIO, @@ -1256,24 +1253,8 @@ mymain(void) DO_TEST("riscv64-virt-pci", QEMU_CAPS_OBJECT_GPEX); - DO_TEST("virtio-transitional", - QEMU_CAPS_DEVICE_VIDEO_PRIMARY, - QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE, - QEMU_CAPS_DEVICE_PCIE_ROOT_PORT, - QEMU_CAPS_DEVICE_VIRTIO_RNG, - QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY, - QEMU_CAPS_DEVICE_VHOST_VSOCK, - QEMU_CAPS_VIRTIO_INPUT_HOST, - QEMU_CAPS_VIRTIO_SCSI); - DO_TEST("virtio-non-transitional", - QEMU_CAPS_DEVICE_VIDEO_PRIMARY, - QEMU_CAPS_DEVICE_PCIE_PCI_BRIDGE, - QEMU_CAPS_DEVICE_PCIE_ROOT_PORT, - QEMU_CAPS_DEVICE_VIRTIO_RNG, - QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY, - QEMU_CAPS_DEVICE_VHOST_VSOCK, - QEMU_CAPS_VIRTIO_INPUT_HOST, - QEMU_CAPS_VIRTIO_SCSI); + DO_TEST_CAPS_LATEST("virtio-transitional"); + DO_TEST_CAPS_LATEST("virtio-non-transitional"); if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) virFileDeleteTree(fakerootdir);
Add DO_TEST_CAPS* macros, lifted from qemuxml2argvtest. Use it on a few recently added xml2xml tests that use DO_TEST_CAPS in the argv test case. The firmware examples require breaking the symlink and creating our own test file. Also add a test for os-firmware-efi which seems to have been missed. One subtle difference compared to qemuxml2argv is output file naming. qemuxml2xml uses a system where if specially named files ${basename}-active.xml or ${basename}-inactive.xml exist, those are used as output, otherwise just ${basename}.xml is used. I'm not quite sure how to make this fit with the caps suffix naming scheme used in qemuxml2argv, where for example DO_CAPS_LATEST will always add a -latest suffix to basename. This code by default will store the output in ${basename}.xml with no -latest suffix. This makes it easier to convert DO_TEST calls to CAPS variants, because it won't require any test file rename/removal, but if we ever want to add more than one qemuxml2xml output for a single input, it will require special file naming to not collide. IMO that's not a big deal as it follows the existing -active pattern. But it's a divergence from qemuxml2argv behavior Signed-off-by: Cole Robinson <crobinso@redhat.com> --- tests/qemuxml2argvdata/vhost-vsock.xml | 2 +- .../aarch64-os-firmware-efi.xml | 32 +++++++- tests/qemuxml2xmloutdata/os-firmware-bios.xml | 69 +++++++++++++++- .../os-firmware-efi-secboot.xml | 69 +++++++++++++++- tests/qemuxml2xmloutdata/os-firmware-efi.xml | 69 +++++++++++++++- tests/qemuxml2xmltest.c | 81 +++++++------------ 6 files changed, 267 insertions(+), 55 deletions(-) mode change 120000 => 100644 tests/qemuxml2xmloutdata/aarch64-os-firmware-efi.xml mode change 120000 => 100644 tests/qemuxml2xmloutdata/os-firmware-bios.xml mode change 120000 => 100644 tests/qemuxml2xmloutdata/os-firmware-efi-secboot.xml mode change 120000 => 100644 tests/qemuxml2xmloutdata/os-firmware-efi.xml -- 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list