On 03/12/19 12:20, Daniel P. Berrangé wrote:
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@redhat.com> --- 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.
Ahh, indeed. :)
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/....
Thanks for the explanation! Acked-by: Laszlo Ersek <lersek@redhat.com> Laszlo