Hi Heinrich,
On 25 September 2017 at 00:01, Heinrich Schuchardt xypron.glpk@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@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.
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