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

(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

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/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.

Which, again, could be because I have no clue what virFileWrapperAddPrefix() does.