Re: [PATCH v1 08/15] test: Introduce qemufirmwaretest

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux