On 09/25/2017 04:12 AM, Simon Glass wrote:
Hi Heinrich,
On 15 September 2017 at 02:06, Heinrich Schuchardt xypron.glpk@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@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@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