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

[Heinrich Schuchardt]

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.

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

How would putting all private variables into a structure provide logical
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.

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

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

Best regards

Heinich

> 
>> +
>> +/*
>> + * Notification function, increments a counter.
> 
> You don't need the .

Should we ever decide to use Doxygen we will need a dot to separate the
first line comment from the rest.

I think it is a pity that U-Boot does not use Doxygen for API documentation.

Regards

Heinrich

> 
>> + *
>> + * @event      notified event
>> + * @context    pointer to the counter
>> + */
>> +static void EFIAPI notify(struct efi_event *event, void *context)
>> +{
>> +       if (!context)
>> +               return;
>> +       ++*(unsigned int *)context;
>> +}
>> +
>> +/*
>> + * Setup unit test.
>> + *
>> + * Create an EVT_SIGNAL_EXIT_BOOT_SERVICES event.
>> + *
>> + * @handle:    handle of the loaded image
>> + * @systable:  system table
> 
> @return...
> 
>> + */
>> +static int setup(const efi_handle_t handle,
>> +                const struct efi_system_table *systable)
>> +{
>> +       efi_status_t ret;
>> +
>> +       boottime = systable->boottime;
>> +
>> +       counter = 0;
>> +       ret = boottime->create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES,
>> +                                    TPL_CALLBACK, notify, (void *)&counter,
>> +                                    &event_notify);
>> +       if (ret != EFI_SUCCESS) {
>> +               efi_st_error("could not create event\n");
>> +               return 1;
>> +       }
>> +       return 0;
>> +}
>> +
>> +/*
>> + * Tear down unit test.
>> + *
>> + * Close the event created in setup.
>> + */
>> +static int teardown(void)
>> +{
>> +       efi_status_t ret;
>> +
>> +       if (event_notify) {
>> +               ret = boottime->close_event(event_notify);
>> +               event_notify = NULL;
>> +               if (ret != EFI_SUCCESS) {
>> +                       efi_st_error("could not close event\n");
>> +                       return 1;
>> +               }
>> +       }
>> +       return 0;
>> +}
>> +
>> +/*
>> + * Execute unit test.
>> + *
>> + * Check that the notification function of the EVT_SIGNAL_EXIT_BOOT_SERVICES
>> + * event has been called.
>> + *
>> + * Call ExitBootServices again and check that the notification function is
>> + * not called again.
>> + */
>> +static int execute(void)
>> +{
>> +       if (counter != 1) {
>> +               efi_st_error("ExitBootServices was not notified");
>> +               return 1;
>> +       }
>> +       efi_st_exit_boot_services();
>> +       if (counter != 1) {
>> +               efi_st_error("ExitBootServices was notified twice");
>> +               return 1;
>> +       }
>> +       return 0;
>> +}
>> +
>> +EFI_UNIT_TEST(exitbootservices) = {
>> +       .name = "ExitBootServices",
>> +       .phase = EFI_SETUP_BEFORE_BOOTTIME_EXIT,
>> +       .setup = setup,
>> +       .execute = execute,
>> +       .teardown = teardown,
>> +};
>> --
>> 2.11.0
>>
>