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@xxxxxxxxxx>
> > ---
> > 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/....
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
