On 02/27/19 11:04, Michal Privoznik wrote:
> Test firmware description parsing so far.
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> tests/Makefile.am | 9 ++++
> tests/qemufirmwaredata/40-bios.json | 35 +++++++++++++
> tests/qemufirmwaredata/50-ovmf-sb.json | 36 +++++++++++++
> tests/qemufirmwaredata/60-ovmf.json | 35 +++++++++++++
> tests/qemufirmwaredata/70-aavmf.json | 35 +++++++++++++
> tests/qemufirmwaretest.c | 70 ++++++++++++++++++++++++++
> 6 files changed, 220 insertions(+)
> create mode 100644 tests/qemufirmwaredata/40-bios.json
> create mode 100644 tests/qemufirmwaredata/50-ovmf-sb.json
> create mode 100644 tests/qemufirmwaredata/60-ovmf.json
> create mode 100644 tests/qemufirmwaredata/70-aavmf.json
> create mode 100644 tests/qemufirmwaretest.c
If the test data files added in this patch are from the examples in
QEMU's "docs/interop/firmware.json", I suggest stating so in the commit
message, also identifying the QEMU commit that added those examples.
In addition, I wonder if the filenames should carry the double-digit
priority prefixes at once in this patch.
Even if they do, I'm thinking that aavmf shouldn't be ordered (by
priority prefix) behind ovmf, given that the qemu targets / machine
types they are suitable for are mutually exclusive.
Other than that, I have two superficial comments (questions) below,
because my knowledge of the libvirt test infrastructure is nonexistent
to minimal:
>
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index c3f633cee0..d23a8c2812 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -132,6 +132,7 @@ EXTRA_DIST = \
> qemuxml2xmloutdata \
> qemustatusxml2xmldata \
> qemumemlockdata \
> + qemufirmwaredata \
> secretxml2xmlin \
> securityselinuxhelperdata \
> securityselinuxlabeldata \
> @@ -291,6 +292,7 @@ test_programs += qemuxml2argvtest qemuxml2xmltest \
> qemublocktest \
> qemumigparamstest \
> qemusecuritytest \
> + qemufirmwaretest \
> $(NULL)
> test_helpers += qemucapsprobe
> test_libraries += libqemumonitortestutils.la \
> @@ -698,6 +700,12 @@ qemusecuritytest_SOURCES = \
> testutilsqemu.h testutilsqemu.c
> qemusecuritytest_LDADD = $(qemu_LDADDS) $(LDADDS)
>
> +qemufirmwaretest_SOURCES = \
> + qemufirmwaretest.c \
> + testutils.h testutils.c \
> + $(NULL)
> +qemufirmwaretest_LDADD = $(qemu_LDADDS) $(LDADDS)
> +
> else ! WITH_QEMU
> EXTRA_DIST += qemuxml2argvtest.c qemuxml2xmltest.c qemuargv2xmltest.c \
> domainsnapshotxml2xmltest.c \
> @@ -711,6 +719,7 @@ EXTRA_DIST += qemuxml2argvtest.c qemuxml2xmltest.c qemuargv2xmltest.c \
> qemumigparamstest.c \
> qemusecuritytest.c qemusecuritytest.h \
> qemusecuritymock.c \
> + qemufirmwaretest.c \
> $(QEMUMONITORTESTUTILS_SOURCES)
> endif ! WITH_QEMU
>
> diff --git a/tests/qemufirmwaredata/40-bios.json b/tests/qemufirmwaredata/40-bios.json
> new file mode 100644
> index 0000000000..137ff70779
> --- /dev/null
> +++ b/tests/qemufirmwaredata/40-bios.json
> @@ -0,0 +1,35 @@
> +{
> + "description": "SeaBIOS",
> + "interface-types": [
> + "bios"
> + ],
> + "mapping": {
> + "device": "memory",
> + "filename": "/usr/share/seabios/bios-256k.bin"
> + },
> + "targets": [
> + {
> + "architecture": "i386",
> + "machines": [
> + "pc-i440fx-*",
> + "pc-q35-*"
> + ]
> + },
> + {
> + "architecture": "x86_64",
> + "machines": [
> + "pc-i440fx-*",
> + "pc-q35-*"
> + ]
> + }
> + ],
> + "features": [
> + "acpi-s3",
> + "acpi-s4"
> + ],
> + "tags": [
> + "CONFIG_BOOTSPLASH=n",
> + "CONFIG_ROM_SIZE=256",
> + "CONFIG_USE_SMM=n"
> + ]
> +}
> diff --git a/tests/qemufirmwaredata/50-ovmf-sb.json b/tests/qemufirmwaredata/50-ovmf-sb.json
> new file mode 100644
> index 0000000000..c804ac1038
> --- /dev/null
> +++ b/tests/qemufirmwaredata/50-ovmf-sb.json
> @@ -0,0 +1,36 @@
> +{
> + "description": "OVMF with SB+SMM, SB enabled, MS certs enrolled",
> + "interface-types": [
> + "uefi"
> + ],
> + "mapping": {
> + "device": "flash",
> + "executable": {
> + "filename": "/usr/share/OVMF/OVMF_CODE.secboot.fd",
> + "format": "raw"
> + },
> + "nvram-template": {
> + "filename": "/usr/share/OVMF/OVMF_VARS.secboot.fd",
> + "format": "raw"
> + }
> + },
> + "targets": [
> + {
> + "architecture": "x86_64",
> + "machines": [
> + "pc-q35-*"
> + ]
> + }
> + ],
> + "features": [
> + "acpi-s3",
> + "amd-sev",
> + "enrolled-keys",
> + "requires-smm",
> + "secure-boot",
> + "verbose-dynamic"
> + ],
> + "tags": [
> +
> + ]
> +}
> diff --git a/tests/qemufirmwaredata/60-ovmf.json b/tests/qemufirmwaredata/60-ovmf.json
> new file mode 100644
> index 0000000000..5e8a94ae78
> --- /dev/null
> +++ b/tests/qemufirmwaredata/60-ovmf.json
> @@ -0,0 +1,35 @@
> +{
> + "description": "OVMF with SB+SMM, empty varstore",
> + "interface-types": [
> + "uefi"
> + ],
> + "mapping": {
> + "device": "flash",
> + "executable": {
> + "filename": "/usr/share/OVMF/OVMF_CODE.secboot.fd",
> + "format": "raw"
> + },
> + "nvram-template": {
> + "filename": "/usr/share/OVMF/OVMF_VARS.fd",
> + "format": "raw"
> + }
> + },
> + "targets": [
> + {
> + "architecture": "x86_64",
> + "machines": [
> + "pc-q35-*"
> + ]
> + }
> + ],
> + "features": [
> + "acpi-s3",
> + "amd-sev",
> + "requires-smm",
> + "secure-boot",
> + "verbose-dynamic"
> + ],
> + "tags": [
> +
> + ]
> +}
> diff --git a/tests/qemufirmwaredata/70-aavmf.json b/tests/qemufirmwaredata/70-aavmf.json
> new file mode 100644
> index 0000000000..114d1475a2
> --- /dev/null
> +++ b/tests/qemufirmwaredata/70-aavmf.json
> @@ -0,0 +1,35 @@
> +{
> + "description": "UEFI firmware for ARM64 virtual machines",
> + "interface-types": [
> + "uefi"
> + ],
> + "mapping": {
> + "device": "flash",
> + "executable": {
> + "filename": "/usr/share/AAVMF/AAVMF_CODE.fd",
> + "format": "raw"
> + },
> + "nvram-template": {
> + "filename": "/usr/share/AAVMF/AAVMF_VARS.fd",
> + "format": "raw"
> + }
> + },
> + "targets": [
> + {
> + "architecture": "aarch64",
> + "machines": [
> + "virt-*"
> + ]
> + }
> + ],
> + "features": [
> +
> + ],
> + "tags": [
> + "-a AARCH64",
> + "-p ArmVirtPkg/ArmVirtQemu.dsc",
> + "-t GCC48",
> + "-b DEBUG",
> + "-D DEBUG_PRINT_ERROR_LEVEL=0x80000000"
> + ]
> +}
> diff --git a/tests/qemufirmwaretest.c b/tests/qemufirmwaretest.c
> new file mode 100644
> index 0000000000..0c5fb1e55a
> --- /dev/null
> +++ b/tests/qemufirmwaretest.c
> @@ -0,0 +1,70 @@
> +#include <config.h>
> +
> +#include "testutils.h"
> +#include "qemu/qemu_firmware.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_QEMU
> +
> +static int
> +testParseFormatFW(const void *opaque)
> +{
> + const char *filename = opaque;
> + VIR_AUTOFREE(char *) path = NULL;
> + VIR_AUTOPTR(qemuFirmware) fw = NULL;
> + VIR_AUTOFREE(char *) buf = NULL;
> + VIR_AUTOPTR(virJSONValue) json = NULL;
> + VIR_AUTOFREE(char *) expected = NULL;
> + VIR_AUTOFREE(char *) actual = NULL;
> +
> + if (virAsprintf(&path, "%s/qemufirmwaredata/%s",
> + abs_srcdir, filename) < 0)
> + return -1;
> +
> + if (!(fw = qemuFirmwareParse(path)))
> + return -1;
> +
> + if (virFileReadAll(path,
> + 1024 * 1024, /* 1MiB */
> + &buf) < 0)
> + return -1;
> +
> + if (!(json = virJSONValueFromString(buf)))
> + return -1;
> +
> + /* Description and tags are not parsed. */
> + if (virJSONValueObjectRemoveKey(json, "description", NULL) < 0 ||
> + virJSONValueObjectRemoveKey(json, "tags", NULL) < 0)
> + return -1;
> +
> + if (!(expected = virJSONValueToString(json, true)))
> + return -1;
> +
> + if (!(actual = qemuFirmwareFormat(fw)))
> + return -1;
> +
> + return virTestCompareToString(expected, actual);
> +}
Can you add a comment to the function about the general idea? Or is this
approach common for libvirt tests? I mean, to make heads or tails of
this function, I have to decipher what each step does, and what the
final comparison compares. It would be easier to read if you explained
the steps in a comment, and then we'd only have to verify the steps
against that documentation.
My other question: when does virJSONValueObjectRemoveKey() fail? I
assume it doesn't fail when the key isn't present.
Thanks,
Laszlo
> +
> +
> +static int
> +mymain(void)
> +{
> + int ret = 0;
> +
> +#define DO_PARSE_TEST(filename) \
> + do { \
> + if (virTestRun("QEMU FW " filename, \
> + testParseFormatFW, filename) < 0) \
> + ret = -1; \
> + } while (0)
> +
> + DO_PARSE_TEST("40-bios.json");
> + DO_PARSE_TEST("50-ovmf-sb.json");
> + DO_PARSE_TEST("60-ovmf.json");
> + DO_PARSE_TEST("70-aavmf.json");
> +
> + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
> +}
> +
> +
> +VIR_TEST_MAIN(mymain);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
