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 <mpriv...@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 <ler...@redhat.com>
Laszlo
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
