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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Thanks
Laszlo

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux