[U-Boot] [PATCH 10/10] efi_selftest: check notification of ExitBootServices

[Simon Glass]

Hi Heinrich,

On 25 September 2017 at 00:01, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 09/25/2017 04:12 AM, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On 15 September 2017 at 02:06, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >> Check that the notification function of an
> >> EVT_SIGNAL_EXIT_BOOT_SERVICES event is called
> >> exactly once.
> >>
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
> >> ---
> >>  lib/efi_selftest/Makefile                        |   3 +
> >>  lib/efi_selftest/efi_selftest_exitbootservices.c | 106 +++++++++++++++++++++++
> >>  2 files changed, 109 insertions(+)
> >>  create mode 100644 lib/efi_selftest/efi_selftest_exitbootservices.c
> >>
> >> diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile
> >> index ddf304e1fa..30f1960933 100644
> >> --- a/lib/efi_selftest/Makefile
> >> +++ b/lib/efi_selftest/Makefile
> >> @@ -13,6 +13,8 @@ CFLAGS_efi_selftest_console.o := $(CFLAGS_EFI)
> >>  CFLAGS_REMOVE_efi_selftest_console.o := $(CFLAGS_NON_EFI)
> >>  CFLAGS_efi_selftest_events.o := $(CFLAGS_EFI)
> >>  CFLAGS_REMOVE_efi_selftest_events.o := $(CFLAGS_NON_EFI)
> >> +CFLAGS_efi_selftest_exitbootservices.o := $(CFLAGS_EFI)
> >> +CFLAGS_REMOVE_efi_selftest_exitbootservices.o := $(CFLAGS_NON_EFI)
> >>  CFLAGS_efi_selftest_tpl.o := $(CFLAGS_EFI)
> >>  CFLAGS_REMOVE_efi_selftest_tpl.o := $(CFLAGS_NON_EFI)
> >>
> >> @@ -20,4 +22,5 @@ obj-$(CONFIG_CMD_BOOTEFI_SELFTEST) += \
> >>  efi_selftest.o \
> >>  efi_selftest_console.o \
> >>  efi_selftest_events.o \
> >> +efi_selftest_exitbootservices.o \
> >>  efi_selftest_tpl.o
> >> diff --git a/lib/efi_selftest/efi_selftest_exitbootservices.c b/lib/efi_selftest/efi_selftest_exitbootservices.c
> >> new file mode 100644
> >> index 0000000000..60271e6180
> >> --- /dev/null
> >> +++ b/lib/efi_selftest/efi_selftest_exitbootservices.c
> >> @@ -0,0 +1,106 @@
> >> +/*
> >> + * efi_selftest_events
> >> + *
> >> + * Copyright (c) 2017 Heinrich Schuchardt <xypron.glpk at gmx.de>
> >> + *
> >> + * SPDX-License-Identifier:     GPL-2.0+
> >> + *
> >> + * This unit test checks that the notification function of an
> >> + * EVT_SIGNAL_EXIT_BOOT_SERVICES event is called exactly once.
> >> + */
> >> +
> >> +#include <efi_selftest.h>
> >> +
> >> +static struct efi_boot_services *boottime;
> >> +static struct efi_event *event_notify;
> >> +static unsigned int counter;
> >
> > I wonder if the solution to the context thin in the notify() function
> > is to put all of these in a struct?
>
> This would mean replacing
> boottime->something
> by
> config->boottime->something.
>
> This does not make the code easier to read.

I don't understand that comment. In general only the outermost
function accesses the global, then passes what is needed to the inner
functions. So in practice you seldom see config->boottime->something.

You might define a local variable boottime as config->boottime,
perhaps. But I agree that two levels of access would be bad.

>
> >
> > It is nice to group globals into a struct to allow future one-to-many
> > conversion, reduce the number of symbols in the map and provide a
> > logical grouping. So this would kill two birds with one stone.
>
> I typically do not read map files.
>
> With your suggestion the code will be slower and the binary will be larger.

Actually it is often faster and smaller. E.g. on ARM every global
access requires a literal pool entry and access. You can compare the
compiler output and see what you find.

>
> How would putting all private variables into a structure provide logical
> grouping?

A structure is a way of grouping.

>
> >
> > Another idea is to have setup() return the context (e.g. as a void **
> > final arg). Then that same context can be passed to execute and
> > teardown. This is similar to how the unit tests work in U-Boot.
>
> Passing structures as void* is error prone.
>
> Private variables should stay private. There is no reason to make these
> accessible outside the unit test.

That was not my suggestion.

>
> Passing a void *this would make sense if we would envision having
> multiple instances of the same unit test. I can't see that.

Well you have set up a test framework of sorts, but it does not use
the existing unit test code in U-Boot. That framework uses a test
struct which is passed to all functions. It works quite well.

>
> >
> > Other than that, see my comments to the previous patch which also apply here.
>
> @Alex
> You already accepted the patch series to efi-next and afterwards merged
> a bunch of other patches.
>
> I could not see any comment by Simon concerning functionality.
> Everything seemed to focus on style.
>
> Shall I provide add-on patches covering Simon's comments or should I
> create a new version of the patch series.

If you think there is value in these changes then I suggest doing a
follow-on patch.

Regards,
Simon