Re: [libvirt] [PATCH v2 10/15] qemufirmwaretest: Test qemuFirmwareFetchConfigs()

On Tue, Mar 12, 2019 at 12:13:46PM +0100, Laszlo Ersek wrote: > (adding Dan) > > On 03/07/19 10:29, Michal Privoznik wrote: >> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >> --- >> tests/Makefile.am | 1 + >> .../etc/qemu/firmware/40-ovmf-sb-keys.json | 1 + >> .../etc/qemu/firmware/60-ovmf-sb.json | 0 >> .../user/.config/qemu/firmware/10-bios.json | 0 >> .../share/qemu/firmware/40-bios.json} | 0 >> .../share/qemu/firmware/50-ovmf-sb-keys.json} | 0 >> .../share/qemu/firmware/60-ovmf-sb.json} | 0 >> .../share/qemu/firmware/61-ovmf.json} | 0 >> .../share/qemu/firmware/70-aavmf.json} | 0 >> tests/qemufirmwaretest.c | 72 +++++++++++++++++-- >> 10 files changed, 68 insertions(+), 6 deletions(-) >> create mode 120000 tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json >> create mode 100644 tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf-sb.json >> create mode 100644 tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json >> rename tests/qemufirmwaredata/{bios.json => usr/share/qemu/firmware/40-bios.json} (100%) >> rename tests/qemufirmwaredata/{ovmf-sb-keys.json => usr/share/qemu/firmware/50-ovmf-sb-keys.json} (100%) >> rename tests/qemufirmwaredata/{ovmf-sb.json => usr/share/qemu/firmware/60-ovmf-sb.json} (100%) >> rename tests/qemufirmwaredata/{ovmf.json => usr/share/qemu/firmware/61-ovmf.json} (100%) >> rename tests/qemufirmwaredata/{aavmf.json => usr/share/qemu/firmware/70-aavmf.json} (100%) >> >> diff --git a/tests/Makefile.am b/tests/Makefile.am >> index b3449fa96b..b18b9e67ae 100644 >> --- a/tests/Makefile.am >> +++ b/tests/Makefile.am >> @@ -705,6 +705,7 @@ qemusecuritytest_LDADD = $(qemu_LDADDS) $(LDADDS) >> qemufirmwaretest_SOURCES = \ >> qemufirmwaretest.c \ >> testutils.h testutils.c \ >> + virfilewrapper.c virfilewrapper.h \ >> $(NULL) >> qemufirmwaretest_LDADD = $(qemu_LDADDS) $(LDADDS) >> >> diff --git a/tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json b/tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json >> new file mode 120000 >> index 0000000000..68e8cbbc2a >> --- /dev/null >> +++ b/tests/qemufirmwaredata/etc/qemu/firmware/40-ovmf-sb-keys.json >> @@ -0,0 +1 @@ >> +../../../usr/share/qemu/firmware/50-ovmf-sb-keys.json >> \ No newline at end of file >> diff --git a/tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf-sb.json b/tests/qemufirmwaredata/etc/qemu/firmware/60-ovmf-sb.json >> new file mode 100644 >> index 0000000000..e69de29bb2 >> diff --git a/tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json b/tests/qemufirmwaredata/home/user/.config/qemu/firmware/10-bios.json >> new file mode 100644 >> index 0000000000..e69de29bb2 >> diff --git a/tests/qemufirmwaredata/bios.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/40-bios.json >> similarity index 100% >> rename from tests/qemufirmwaredata/bios.json >> rename to tests/qemufirmwaredata/usr/share/qemu/firmware/40-bios.json >> diff --git a/tests/qemufirmwaredata/ovmf-sb-keys.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/50-ovmf-sb-keys.json >> similarity index 100% >> rename from tests/qemufirmwaredata/ovmf-sb-keys.json >> rename to tests/qemufirmwaredata/usr/share/qemu/firmware/50-ovmf-sb-keys.json >> diff --git a/tests/qemufirmwaredata/ovmf-sb.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/60-ovmf-sb.json >> similarity index 100% >> rename from tests/qemufirmwaredata/ovmf-sb.json >> rename to tests/qemufirmwaredata/usr/share/qemu/firmware/60-ovmf-sb.json >> diff --git a/tests/qemufirmwaredata/ovmf.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/61-ovmf.json >> similarity index 100% >> rename from tests/qemufirmwaredata/ovmf.json >> rename to tests/qemufirmwaredata/usr/share/qemu/firmware/61-ovmf.json >> diff --git a/tests/qemufirmwaredata/aavmf.json b/tests/qemufirmwaredata/usr/share/qemu/firmware/70-aavmf.json >> similarity index 100% >> rename from tests/qemufirmwaredata/aavmf.json >> rename to tests/qemufirmwaredata/usr/share/qemu/firmware/70-aavmf.json >> diff --git a/tests/qemufirmwaretest.c b/tests/qemufirmwaretest.c >> index 176cf0920d..cbf92f2689 100644 >> --- a/tests/qemufirmwaretest.c >> +++ b/tests/qemufirmwaretest.c >> @@ -1,7 +1,9 @@ >> #include <config.h> >> >> #include "testutils.h" >> +#include "virfilewrapper.h" >> #include "qemu/qemu_firmware.h" >> +#include "configmake.h" >> >> #define VIR_FROM_THIS VIR_FROM_QEMU >> >> @@ -50,11 +52,66 @@ testParseFormatFW(const void *opaque) >> } >> >> >> +static int >> +testFWPrecedence(const void *opaque ATTRIBUTE_UNUSED) >> +{ >> + VIR_AUTOFREE(char *) fakehome = NULL; >> + VIR_AUTOSTRINGLIST fwList = NULL; >> + size_t nfwList; >> + size_t i; >> + const char *expected[] = { >> + PREFIX "/share/qemu/firmware/40-bios.json", >> + SYSCONFDIR "/qemu/firmware/40-ovmf-sb-keys.json", >> + PREFIX "/share/qemu/firmware/50-ovmf-sb-keys.json", >> + PREFIX "/share/qemu/firmware/61-ovmf.json", >> + PREFIX "/share/qemu/firmware/70-aavmf.json", >> + }; >> + >> + if (VIR_STRDUP(fakehome, abs_srcdir "/qemufirmwaredata/home/user/.config") < 0) >> + return -1; >> + >> + setenv("XDG_CONFIG_HOME", fakehome, 1); >> + >> + if (qemuFirmwareFetchConfigs(&fwList) < 0) >> + return -1; >> + >> + if (!fwList) { >> + fprintf(stderr, "Expected result, got nothing\n"); >> + return -1; >> + } > > This error message is extremely confusing. You intend to say "I expected > a non-NULL result, but got a NULL result". > > But the way I read it was: "oh nice, I got the expected result!, which > is nothing". > > Please clean this up. > >> + >> + nfwList = virStringListLength((const char **)fwList); >> + if (nfwList != ARRAY_CARDINALITY(expected)) { >> + fprintf(stderr, "Expected %zu paths, got %zu\n", >> + ARRAY_CARDINALITY(expected), nfwList); >> + return -1; >> + } >> + >> + for (i = 0; i < ARRAY_CARDINALITY(expected); i++) { >> + if (STRNEQ_NULLABLE(expected[i], fwList[i])) { >> + fprintf(stderr, >> + "Unexpected path (i=%zu). Expected %s got %s \n", >> + i, expected[i], NULLSTR(fwList[i])); >> + return -1; >> + } >> + } >> + >> + return 0; >> +} >> + >> + >> static int >> mymain(void) >> { >> int ret = 0; >> >> + virFileWrapperAddPrefix(SYSCONFDIR "/qemu/firmware", >> + abs_srcdir "/qemufirmwaredata/etc/qemu/firmware"); >> + virFileWrapperAddPrefix(PREFIX "/share/qemu/firmware", >> + abs_srcdir "/qemufirmwaredata/usr/share/qemu/firmware"); >> + virFileWrapperAddPrefix("/home/user/.config/qemu/firmware", >> + abs_srcdir "/qemufirmwaredata/home/user/.config/qemu/firmware"); >> + > > I don't understand what virFileWrapperAddPrefix() does. I checked the > header and the C file, but I'm none the wiser. (There are no comments.) > See below why this might matter. > >> #define DO_PARSE_TEST(filename) \ >> do { \ >> if (virTestRun("QEMU FW " filename, \ >> @@ -62,14 +119,17 @@ mymain(void) >> ret = -1; \ >> } while (0) >> >> - DO_PARSE_TEST("bios.json"); >> - DO_PARSE_TEST("ovmf-sb-keys.json"); >> - DO_PARSE_TEST("ovmf-sb.json"); >> - DO_PARSE_TEST("ovmf.json"); >> - DO_PARSE_TEST("aavmf.json"); >> + DO_PARSE_TEST("usr/share/qemu/firmware/40-bios.json"); >> + DO_PARSE_TEST("usr/share/qemu/firmware/50-ovmf-sb-keys.json"); >> + DO_PARSE_TEST("usr/share/qemu/firmware/60-ovmf-sb.json"); >> + DO_PARSE_TEST("usr/share/qemu/firmware/61-ovmf.json"); >> + DO_PARSE_TEST("usr/share/qemu/firmware/70-aavmf.json"); >> + >> + if (virTestRun("QEMU FW precedence test", testFWPrecedence, NULL) < 0) >> + ret = -1; >> >> return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; >> } >> >> >> -VIR_TEST_MAIN(mymain); >> +VIR_TEST_MAIN(mymain) >> > > After this patch, we have the following json files / symlinks, under > "tests/qemufirmwaredata": > > etc/qemu/firmware/40-ovmf-sb-keys.json > etc/qemu/firmware/60-ovmf-sb.json > home/user/.config/qemu/firmware/10-bios.json > usr/share/qemu/firmware/40-bios.json > usr/share/qemu/firmware/50-ovmf-sb-keys.json > usr/share/qemu/firmware/60-ovmf-sb.json > usr/share/qemu/firmware/61-ovmf.json > usr/share/qemu/firmware/70-aavmf.json > > Are we establishing the precedence between *all* of them? (Again, I > don't understand what virFileWrapperAddPrefix() does.) > > "firmware.json" says, > > # Management software should build a list of files from all three > # locations, then sort the list by filename (i.e., last pathname > # component). Management software should choose the first JSON file on > # the sorted list that matches the search criteria. If a more specific > # directory has a file with same name as a less specific directory, then > # the file in the more specific directory takes effect. If the more > # specific file is zero length, it hides the less specific one. > > Assuming we intend to cover all of these files, the list sorted by > filename (= last pathname component) is: > > home/user/.config/qemu/firmware/10-bios.json > usr/share/qemu/firmware/40-bios.json > etc/qemu/firmware/40-ovmf-sb-keys.json > usr/share/qemu/firmware/50-ovmf-sb-keys.json > etc/qemu/firmware/60-ovmf-sb.json > usr/share/qemu/firmware/60-ovmf-sb.json > usr/share/qemu/firmware/61-ovmf.json > usr/share/qemu/firmware/70-aavmf.json > > In any given set, defined by last pathname component, "home" takes > precedence over "etc" takes precedence over "usr". > > There is only one set with more than one elements -- the set represented > by "60-ovmf-sb.json". In that set, we have > > etc/qemu/firmware/60-ovmf-sb.json > usr/share/qemu/firmware/60-ovmf-sb.json > > and there "etc" takes precedence. So the final list should be > > home/user/.config/qemu/firmware/10-bios.json This is a zero length file, so it is a blackout > usr/share/qemu/firmware/40-bios.json > etc/qemu/firmware/40-ovmf-sb-keys.json > usr/share/qemu/firmware/50-ovmf-sb-keys.json > etc/qemu/firmware/60-ovmf-sb.json So is this one. > usr/share/qemu/firmware/61-ovmf.json > usr/share/qemu/firmware/70-aavmf.json > > This is not what the test case contains however -- "expected" contains 5 > elements only --, so I'm thinking that the input dataset is not what I > think it is. You just missed that two of the files are zero-length.

> Which, again, could be because I have no clue what > virFileWrapperAddPrefix() does. That is a mock override to setup a virtual root directory, so when the code tries to load from "/etc/qemu" it gets redirected to instead load files from $GIT/tests/....