[{"id":1774339,"web_url":"http://patchwork.ozlabs.org/comment/1774339/","msgid":"","list_archive_url":null,"date":"2017-09-25T02:12:10","subject":"Re: [U-Boot] [PATCH 10/10] efi_selftest: check notification of\n\tExitBootServices","submitter":{"id":6170,"url":"http://patchwork.ozlabs.org/api/people/6170/","name":"Simon Glass","email":"sjg@chromium.org"},"content":"Hi Heinrich,\n\nOn 15 September 2017 at 02:06, Heinrich Schuchardt wrote:\n> Check that the notification function of an\n> EVT_SIGNAL_EXIT_BOOT_SERVICES event is called\n> exactly once.\n>\n> Signed-off-by: Heinrich Schuchardt \n> ---\n> lib/efi_selftest/Makefile | 3 +\n> lib/efi_selftest/efi_selftest_exitbootservices.c | 106 +++++++++++++++++++++++\n> 2 files changed, 109 insertions(+)\n> create mode 100644 lib/efi_selftest/efi_selftest_exitbootservices.c\n>\n> diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile\n> index ddf304e1fa..30f1960933 100644\n> --- a/lib/efi_selftest/Makefile\n> +++ b/lib/efi_selftest/Makefile\n> @@ -13,6 +13,8 @@ CFLAGS_efi_selftest_console.o := $(CFLAGS_EFI)\n> CFLAGS_REMOVE_efi_selftest_console.o := $(CFLAGS_NON_EFI)\n> CFLAGS_efi_selftest_events.o := $(CFLAGS_EFI)\n> CFLAGS_REMOVE_efi_selftest_events.o := $(CFLAGS_NON_EFI)\n> +CFLAGS_efi_selftest_exitbootservices.o := $(CFLAGS_EFI)\n> +CFLAGS_REMOVE_efi_selftest_exitbootservices.o := $(CFLAGS_NON_EFI)\n> CFLAGS_efi_selftest_tpl.o := $(CFLAGS_EFI)\n> CFLAGS_REMOVE_efi_selftest_tpl.o := $(CFLAGS_NON_EFI)\n>\n> @@ -20,4 +22,5 @@ obj-$(CONFIG_CMD_BOOTEFI_SELFTEST) += \\\n> efi_selftest.o \\\n> efi_selftest_console.o \\\n> efi_selftest_events.o \\\n> +efi_selftest_exitbootservices.o \\\n> efi_selftest_tpl.o\n> diff --git a/lib/efi_selftest/efi_selftest_exitbootservices.c b/lib/efi_selftest/efi_selftest_exitbootservices.c\n> new file mode 100644\n> index 0000000000..60271e6180\n> --- /dev/null\n> +++ b/lib/efi_selftest/efi_selftest_exitbootservices.c\n> @@ -0,0 +1,106 @@\n> +/*\n> + * efi_selftest_events\n> + *\n> + * Copyright (c) 2017 Heinrich Schuchardt \n> + *\n> + * SPDX-License-Identifier: GPL-2.0+\n> + *\n> + * This unit test checks that the notification function of an\n> + * EVT_SIGNAL_EXIT_BOOT_SERVICES event is called exactly once.\n> + */\n> +\n> +#include \n> +\n> +static struct efi_boot_services *boottime;\n> +static struct efi_event *event_notify;\n> +static unsigned int counter;\n\nI wonder if the solution to the context thin in the notify() function\nis to put all of these in a struct?\n\nIt is nice to group globals into a struct to allow future one-to-many\nconversion, reduce the number of symbols in the map and provide a\nlogical grouping. So this would kill two birds with one stone.\n\nAnother idea is to have setup() return the context (e.g. as a void **\nfinal arg). Then that same context can be passed to execute and\nteardown. This is similar to how the unit tests work in U-Boot.\n\nOther than that, see my comments to the previous patch which also apply here.\n\n> +\n> +/*\n> + * Notification function, increments a counter.\n\nYou don't need the .\n\n> + *\n> + * @event notified event\n> + * @context pointer to the counter\n> + */\n> +static void EFIAPI notify(struct efi_event *event, void *context)\n> +{\n> + if (!context)\n> + return;\n> + ++*(unsigned int *)context;\n> +}\n> +\n> +/*\n> + * Setup unit test.\n> + *\n> + * Create an EVT_SIGNAL_EXIT_BOOT_SERVICES event.\n> + *\n> + * @handle: handle of the loaded image\n> + * @systable: system table\n\n@return...\n\n> + */\n> +static int setup(const efi_handle_t handle,\n> + const struct efi_system_table *systable)\n> +{\n> + efi_status_t ret;\n> +\n> + boottime = systable->boottime;\n> +\n> + counter = 0;\n> + ret = boottime->create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES,\n> + TPL_CALLBACK, notify, (void *)&counter,\n> + &event_notify);\n> + if (ret != EFI_SUCCESS) {\n> + efi_st_error(\"could not create event\\n\");\n> + return 1;\n> + }\n> + return 0;\n> +}\n> +\n> +/*\n> + * Tear down unit test.\n> + *\n> + * Close the event created in setup.\n> + */\n> +static int teardown(void)\n> +{\n> + efi_status_t ret;\n> +\n> + if (event_notify) {\n> + ret = boottime->close_event(event_notify);\n> + event_notify = NULL;\n> + if (ret != EFI_SUCCESS) {\n> + efi_st_error(\"could not close event\\n\");\n> + return 1;\n> + }\n> + }\n> + return 0;\n> +}\n> +\n> +/*\n> + * Execute unit test.\n> + *\n> + * Check that the notification function of the EVT_SIGNAL_EXIT_BOOT_SERVICES\n> + * event has been called.\n> + *\n> + * Call ExitBootServices again and check that the notification function is\n> + * not called again.\n> + */\n> +static int execute(void)\n> +{\n> + if (counter != 1) {\n> + efi_st_error(\"ExitBootServices was not notified\");\n> + return 1;\n> + }\n> + efi_st_exit_boot_services();\n> + if (counter != 1) {\n> + efi_st_error(\"ExitBootServices was notified twice\");\n> + return 1;\n> + }\n> + return 0;\n> +}\n> +\n> +EFI_UNIT_TEST(exitbootservices) = {\n> + .name = \"ExitBootServices\",\n> + .phase = EFI_SETUP_BEFORE_BOOTTIME_EXIT,\n> + .setup = setup,\n> + .execute = execute,\n> + .teardown = teardown,\n> +};\n> --\n> 2.11.0\n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"PShSpNt4\"; \n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"m0yuuOVr\"; dkim-atps=neutral"],"Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3y0nlt68fdz9t3t\n\tfor ;\n\tMon, 25 Sep 2017 12:17:14 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid 9AA42C21D63; Mon, 25 Sep 2017 02:14:54 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id A1A41C2213E;\n\tMon, 25 Sep 2017 02:14:12 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid ABC05C21C45; Mon, 25 Sep 2017 02:12:39 +0000 (UTC)","from mail-qk0-f174.google.com (mail-qk0-f174.google.com\n\t[209.85.220.174])\n\tby lists.denx.de (Postfix) with ESMTPS id 72AB8C2217E\n\tfor ; Mon, 25 Sep 2017 02:12:32 +0000 (UTC)","by mail-qk0-f174.google.com with SMTP id s132so5377676qke.7\n\tfor ; Sun, 24 Sep 2017 19:12:32 -0700 (PDT)","by 10.200.37.200 with HTTP; Sun, 24 Sep 2017 19:12:10 -0700 (PDT)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=-0.0 required=5.0 tests=RCVD_IN_DNSWL_NONE,\n\tRCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL,\n\tT_DKIM_INVALID autolearn=unavailable\n\tautolearn_force=no version=3.4.0","DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;\n\ts=20161025; \n\th=mime-version:sender:in-reply-to:references:from:date:message-id\n\t:subject:to:cc;\n\tbh=J4pHRvNdM4dM97vacZdMnn2rHfFbe6hrQ0225+CeG3M=;\n\tb=PShSpNt48AEC3r6zQRtmTBtuw58UDJmBFMiysszHNpOCY/RUSrvS7dBb7/Fyl5CA+G\n\tUI9zF5v+syQsMrLfyhG9GkmHBCl60MaLCfa5t59AU7FNLVkoepY+aruSy8z+JoaXB3nX\n\tl4SY5TKc6Ws2UP4TGIqcVztKeNx6KQBxv8J3AsSIaN8n8GpD9QvahZcMDcPl73387SjU\n\t9mk/xx1Z7edpOZ5Z6EJ84EN6W0PIqDs5vdw0BPY7/66HTPJil8qyju3Z9olrRlcTNWme\n\thThDNA5bDqGzClmn5OuA6KmqyhNNAl0x9n9J/dqaWSCGdv2D1YxdpK64domExWTB5U8q\n\tZaVg==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:sender:in-reply-to:references:from:date:message-id\n\t:subject:to:cc;\n\tbh=J4pHRvNdM4dM97vacZdMnn2rHfFbe6hrQ0225+CeG3M=;\n\tb=m0yuuOVrl7Rh9YDDZKw04J8ftmUWRN+Z3acLb6P9ABnnRplSG37CQcdg7db4WkRUck\n\twrv8HDgNPYjS1lou5yV5npsRpFJXtESwB2YCbpagGkfPmJIHKyUJeVV4w4nFFi0/+BJf\n\tcyGosi0xrKqh9zcQGDQuWmSxMPyRWgh+6YOrw="],"X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:sender:in-reply-to:references:from\n\t:date:message-id:subject:to:cc;\n\tbh=J4pHRvNdM4dM97vacZdMnn2rHfFbe6hrQ0225+CeG3M=;\n\tb=hUypGlef730jO5ea9rXQ2oA0NR7YopM6rq4wM/9puqz3u/QfP/ILXM8eleUE4lsf8U\n\tPnMMgWxdx7nvy24jnvfLCHFHPq8RXP4IGaWo+fPh0kCuUpX3AvdVMDV9Zj/3pXYOi6M+\n\tN64WfTWqySFSZMDMyiSeAA4PcvjXMtB/K9Jmd+G5Ka1unDj7jinM2QYeyvDZRPMxYu8/\n\tBehaogtlWAapNDeHo26ep44hNgOTZzq6/qYFkFlGyNmSBeD5vufLCHEB/g3AqJVGnhBm\n\tAMBJFUtFzCVTYyWVVoQqqYHPeyGaasEbRQLJkqaWLMuK/d/EjARLzTKykKJTV2FgAKjt\n\tvZ+Q==","X-Gm-Message-State":"AHPjjUg1VOrdx5GwbOjOFdCs5wxnY3BgDEk43C4KkY0Z12WdWgftWx/y\n\tvBrXg35Eti8M8apVfKdPzXIxiwyAsb/v1JRyghqxSg==","X-Google-Smtp-Source":"AOwi7QCvlNsDbJubAs2NtLXd/9UOaikOx+Ky7NrABr394bIUQPYCfOw4RymfyLK6PRGbJscg2+54NIGaSIdYscoHGuA=","X-Received":"by 10.233.216.135 with SMTP id\n\tu129mr8666849qkf.302.1506305551065; \n\tSun, 24 Sep 2017 19:12:31 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<20170915080619.25250-11-xypron.glpk@gmx.de>","References":"<20170915080619.25250-1-xypron.glpk@gmx.de>\n\t<20170915080619.25250-11-xypron.glpk@gmx.de>","From":"Simon Glass ","Date":"Sun, 24 Sep 2017 22:12:10 -0400","X-Google-Sender-Auth":"m_pRthH1XlslJ-IXyJo5pqtGjd0","Message-ID":"","To":"Heinrich Schuchardt ","Cc":"=?utf-8?q?=C5=81ukasz_Majewski?= ,\n\tAndy Shevchenko , U-Boot Mailing List\n\t, Fabio Estevam ,\n\tMaxime Ripard ","Subject":"Re: [U-Boot] [PATCH 10/10] efi_selftest: check notification of\n\tExitBootServices","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1774433,"web_url":"http://patchwork.ozlabs.org/comment/1774433/","msgid":"<5fdf8658-3012-7f19-a6ca-ef29571922a4@gmx.de>","list_archive_url":null,"date":"2017-09-25T06:01:46","subject":"Re: [U-Boot] [PATCH 10/10] efi_selftest: check notification of\n\tExitBootServices","submitter":{"id":61270,"url":"http://patchwork.ozlabs.org/api/people/61270/","name":"Heinrich Schuchardt","email":"xypron.glpk@gmx.de"},"content":"On 09/25/2017 04:12 AM, Simon Glass wrote:\n> Hi Heinrich,\n> \n> On 15 September 2017 at 02:06, Heinrich Schuchardt wrote:\n>> Check that the notification function of an\n>> EVT_SIGNAL_EXIT_BOOT_SERVICES event is called\n>> exactly once.\n>>\n>> Signed-off-by: Heinrich Schuchardt \n>> ---\n>> lib/efi_selftest/Makefile | 3 +\n>> lib/efi_selftest/efi_selftest_exitbootservices.c | 106 +++++++++++++++++++++++\n>> 2 files changed, 109 insertions(+)\n>> create mode 100644 lib/efi_selftest/efi_selftest_exitbootservices.c\n>>\n>> diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile\n>> index ddf304e1fa..30f1960933 100644\n>> --- a/lib/efi_selftest/Makefile\n>> +++ b/lib/efi_selftest/Makefile\n>> @@ -13,6 +13,8 @@ CFLAGS_efi_selftest_console.o := $(CFLAGS_EFI)\n>> CFLAGS_REMOVE_efi_selftest_console.o := $(CFLAGS_NON_EFI)\n>> CFLAGS_efi_selftest_events.o := $(CFLAGS_EFI)\n>> CFLAGS_REMOVE_efi_selftest_events.o := $(CFLAGS_NON_EFI)\n>> +CFLAGS_efi_selftest_exitbootservices.o := $(CFLAGS_EFI)\n>> +CFLAGS_REMOVE_efi_selftest_exitbootservices.o := $(CFLAGS_NON_EFI)\n>> CFLAGS_efi_selftest_tpl.o := $(CFLAGS_EFI)\n>> CFLAGS_REMOVE_efi_selftest_tpl.o := $(CFLAGS_NON_EFI)\n>>\n>> @@ -20,4 +22,5 @@ obj-$(CONFIG_CMD_BOOTEFI_SELFTEST) += \\\n>> efi_selftest.o \\\n>> efi_selftest_console.o \\\n>> efi_selftest_events.o \\\n>> +efi_selftest_exitbootservices.o \\\n>> efi_selftest_tpl.o\n>> diff --git a/lib/efi_selftest/efi_selftest_exitbootservices.c b/lib/efi_selftest/efi_selftest_exitbootservices.c\n>> new file mode 100644\n>> index 0000000000..60271e6180\n>> --- /dev/null\n>> +++ b/lib/efi_selftest/efi_selftest_exitbootservices.c\n>> @@ -0,0 +1,106 @@\n>> +/*\n>> + * efi_selftest_events\n>> + *\n>> + * Copyright (c) 2017 Heinrich Schuchardt \n>> + *\n>> + * SPDX-License-Identifier: GPL-2.0+\n>> + *\n>> + * This unit test checks that the notification function of an\n>> + * EVT_SIGNAL_EXIT_BOOT_SERVICES event is called exactly once.\n>> + */\n>> +\n>> +#include \n>> +\n>> +static struct efi_boot_services *boottime;\n>> +static struct efi_event *event_notify;\n>> +static unsigned int counter;\n> \n> I wonder if the solution to the context thin in the notify() function\n> is to put all of these in a struct?\n\nThis would mean replacing\nboottime->something\nby\nconfig->boottime->something.\n\nThis does not make the code easier to read.\n\n> \n> It is nice to group globals into a struct to allow future one-to-many\n> conversion, reduce the number of symbols in the map and provide a\n> logical grouping. So this would kill two birds with one stone.\n\nI typically do not read map files.\n\nWith your suggestion the code will be slower and the binary will be larger.\n\nHow would putting all private variables into a structure provide logical\ngrouping?\n\n> \n> Another idea is to have setup() return the context (e.g. as a void **\n> final arg). Then that same context can be passed to execute and\n> teardown. This is similar to how the unit tests work in U-Boot.\n\nPassing structures as void* is error prone.\n\nPrivate variables should stay private. There is no reason to make these\naccessible outside the unit test.\n\nPassing a void *this would make sense if we would envision having\nmultiple instances of the same unit test. I can't see that.\n\n> \n> Other than that, see my comments to the previous patch which also apply here.\n\n@Alex\nYou already accepted the patch series to efi-next and afterwards merged\na bunch of other patches.\n\nI could not see any comment by Simon concerning functionality.\nEverything seemed to focus on style.\n\nShall I provide add-on patches covering Simon's comments or should I\ncreate a new version of the patch series.\n\nBest regards\n\nHeinich\n\n> \n>> +\n>> +/*\n>> + * Notification function, increments a counter.\n> \n> You don't need the .\n\nShould we ever decide to use Doxygen we will need a dot to separate the\nfirst line comment from the rest.\n\nI think it is a pity that U-Boot does not use Doxygen for API documentation.\n\nRegards\n\nHeinrich\n\n> \n>> + *\n>> + * @event notified event\n>> + * @context pointer to the counter\n>> + */\n>> +static void EFIAPI notify(struct efi_event *event, void *context)\n>> +{\n>> + if (!context)\n>> + return;\n>> + ++*(unsigned int *)context;\n>> +}\n>> +\n>> +/*\n>> + * Setup unit test.\n>> + *\n>> + * Create an EVT_SIGNAL_EXIT_BOOT_SERVICES event.\n>> + *\n>> + * @handle: handle of the loaded image\n>> + * @systable: system table\n> \n> @return...\n> \n>> + */\n>> +static int setup(const efi_handle_t handle,\n>> + const struct efi_system_table *systable)\n>> +{\n>> + efi_status_t ret;\n>> +\n>> + boottime = systable->boottime;\n>> +\n>> + counter = 0;\n>> + ret = boottime->create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES,\n>> + TPL_CALLBACK, notify, (void *)&counter,\n>> + &event_notify);\n>> + if (ret != EFI_SUCCESS) {\n>> + efi_st_error(\"could not create event\\n\");\n>> + return 1;\n>> + }\n>> + return 0;\n>> +}\n>> +\n>> +/*\n>> + * Tear down unit test.\n>> + *\n>> + * Close the event created in setup.\n>> + */\n>> +static int teardown(void)\n>> +{\n>> + efi_status_t ret;\n>> +\n>> + if (event_notify) {\n>> + ret = boottime->close_event(event_notify);\n>> + event_notify = NULL;\n>> + if (ret != EFI_SUCCESS) {\n>> + efi_st_error(\"could not close event\\n\");\n>> + return 1;\n>> + }\n>> + }\n>> + return 0;\n>> +}\n>> +\n>> +/*\n>> + * Execute unit test.\n>> + *\n>> + * Check that the notification function of the EVT_SIGNAL_EXIT_BOOT_SERVICES\n>> + * event has been called.\n>> + *\n>> + * Call ExitBootServices again and check that the notification function is\n>> + * not called again.\n>> + */\n>> +static int execute(void)\n>> +{\n>> + if (counter != 1) {\n>> + efi_st_error(\"ExitBootServices was not notified\");\n>> + return 1;\n>> + }\n>> + efi_st_exit_boot_services();\n>> + if (counter != 1) {\n>> + efi_st_error(\"ExitBootServices was notified twice\");\n>> + return 1;\n>> + }\n>> + return 0;\n>> +}\n>> +\n>> +EFI_UNIT_TEST(exitbootservices) = {\n>> + .name = \"ExitBootServices\",\n>> + .phase = EFI_SETUP_BEFORE_BOOTTIME_EXIT,\n>> + .setup = setup,\n>> + .execute = execute,\n>> + .teardown = teardown,\n>> +};\n>> --\n>> 2.11.0\n>>\n>","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3y0tm34bPtz9t5b\n\tfor ;\n\tMon, 25 Sep 2017 16:02:42 +1000 (AEST)","by lists.denx.de (Postfix, from userid 105)\n\tid E9D60C21DB2; Mon, 25 Sep 2017 06:02:34 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id CAD0AC21C40;\n\tMon, 25 Sep 2017 06:02:31 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid B8754C21C40; Mon, 25 Sep 2017 06:02:30 +0000 (UTC)","from mout.gmx.net (mout.gmx.net [212.227.15.19])\n\tby lists.denx.de (Postfix) with ESMTPS id 6BC2FC21C34\n\tfor ; Mon, 25 Sep 2017 06:02:30 +0000 (UTC)","from [192.168.123.31] ([84.118.154.110]) by mail.gmx.com (mrgmx002\n\t[212.227.17.190]) with ESMTPSA (Nemesis) id\n\t0MA8hF-1e3GEH2YqP-00BOXR; Mon, 25 Sep 2017 08:01:56 +0200"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=-0.7 required=5.0 tests=FREEMAIL_FROM,\n\tRCVD_IN_DNSWL_LOW autolearn=unavailable autolearn_force=no\n\tversion=3.4.0","To":"Simon Glass , Alexander Graf ","References":"<20170915080619.25250-1-xypron.glpk@gmx.de>\n\t<20170915080619.25250-11-xypron.glpk@gmx.de>\n\t","From":"Heinrich Schuchardt ","Message-ID":"<5fdf8658-3012-7f19-a6ca-ef29571922a4@gmx.de>","Date":"Mon, 25 Sep 2017 08:01:46 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-US","X-Provags-ID":"V03:K0:u00iyJg6iGoxEJFVbTjQwtIdvndtZnJCjwktGA+yJmdqWBRKiWl\n\toqkkUfn2/SixT/TMcnMNJvfi6Hbsrl8QisCaFvvJqlFi6JV9meaui8Wc9VENJUDERK2p68f\n\tN/9dCcPgyAEO7EFnqDKK66gBiU/Y/Ez1OKdF/3pV7t6LUZUtvvQ3nDXSJPXOq8BusnnGkFJ\n\tCJHT9nOqD2kGpHyZwAPzw==","X-UI-Out-Filterresults":"notjunk:1; V01:K0:+lDdgvuBQxY=:izgpUedzrNKGN0RecTnePB\n\t4G8RI/QNYZGmrWdWBwplKvm6KzzYwrNZ/gJqhNICKboQ5F/+X0sDB1Zgu/q8c0w1ryf1k6npk\n\t32PNM0PNBBwP4emVXPuEqPCaJMX5wZoa/a0ROIKzBr24CMyXHjx8dQFYfbWcTd1mGo6QD1Qzt\n\tU2pRU9H9u7iumSGUUEhFmdNEi30DlROp5yTPJWvXT7ovWLQ5/Yfnmo/rgWtHyJkUvzTeqUEf7\n\ts9u0bcD+9TDqFmnTx3/L3hrZD/oEaUILe31fdVer41Hk/8xQ8zjP9KTwhd/cDZIHcjK19+FnP\n\tAYsPI2Z2298UL3Iemf1XWzMQ/Hs7K6O3W4YyRTrXbr8oIvcb3dwwXe1vEeL1KPNYsYggvQ3M7\n\tqqorCslMiau3fYQACYR14RYIdUuWVrycuRQY+vMNqDugEBfePERtzdcNyQaEHlQkAbWOmt/oS\n\tWX3ieqDsR09c98StvLhFE11wC6vuGpQRVfxxgJSNxLxiyeLeKXCX79S6ooI1dx1DDgzO9QC/+\n\tlrgatfPu+1JYz0lzoDXHrp7r9v3aLV8bwbUkIjVQ4hY5m0TRIvGqF92W19xSZGH/9CVUDpxyh\n\tAGDr1XvbO0SLsqc20COtUKi5LwRk9hg4NzYsZkzvC/DF7YKKiD3ew06Cj9Qiqp94ur/hmNGlL\n\t+O94Zc1WfWbUdhZ5Efh3eU4kCuDIVumwf+aa1O3qqePXSWLPRkXxy7l0B5u9OcZGNxVqSRIp9\n\tphtAdyljPfPUamEmri/30PDNEmvTeqYPztPeEH4CnLYgnbnhWmNzsZMhcenvXckTqop4lWNiq\n\tV3fqiCOqQTiBukPKpaFmyC5WNOaNZ0SliuYiX9lYkMFoB9n/PA=","Cc":"=?utf-8?q?=C5=81ukasz_Majewski?= ,\n\tAndy Shevchenko , U-Boot Mailing List\n\t, Fabio Estevam ,\n\tMaxime Ripard ","Subject":"Re: [U-Boot] [PATCH 10/10] efi_selftest: check notification of\n\tExitBootServices","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}},{"id":1782424,"web_url":"http://patchwork.ozlabs.org/comment/1782424/","msgid":"","list_archive_url":null,"date":"2017-10-09T04:41:46","subject":"Re: [U-Boot] [PATCH 10/10] efi_selftest: check notification of\n\tExitBootServices","submitter":{"id":6170,"url":"http://patchwork.ozlabs.org/api/people/6170/","name":"Simon Glass","email":"sjg@chromium.org"},"content":"Hi Heinrich,\n\nOn 25 September 2017 at 00:01, Heinrich Schuchardt wrote:\n>\n> On 09/25/2017 04:12 AM, Simon Glass wrote:\n> > Hi Heinrich,\n> >\n> > On 15 September 2017 at 02:06, Heinrich Schuchardt wrote:\n> >> Check that the notification function of an\n> >> EVT_SIGNAL_EXIT_BOOT_SERVICES event is called\n> >> exactly once.\n> >>\n> >> Signed-off-by: Heinrich Schuchardt \n> >> ---\n> >> lib/efi_selftest/Makefile | 3 +\n> >> lib/efi_selftest/efi_selftest_exitbootservices.c | 106 +++++++++++++++++++++++\n> >> 2 files changed, 109 insertions(+)\n> >> create mode 100644 lib/efi_selftest/efi_selftest_exitbootservices.c\n> >>\n> >> diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile\n> >> index ddf304e1fa..30f1960933 100644\n> >> --- a/lib/efi_selftest/Makefile\n> >> +++ b/lib/efi_selftest/Makefile\n> >> @@ -13,6 +13,8 @@ CFLAGS_efi_selftest_console.o := $(CFLAGS_EFI)\n> >> CFLAGS_REMOVE_efi_selftest_console.o := $(CFLAGS_NON_EFI)\n> >> CFLAGS_efi_selftest_events.o := $(CFLAGS_EFI)\n> >> CFLAGS_REMOVE_efi_selftest_events.o := $(CFLAGS_NON_EFI)\n> >> +CFLAGS_efi_selftest_exitbootservices.o := $(CFLAGS_EFI)\n> >> +CFLAGS_REMOVE_efi_selftest_exitbootservices.o := $(CFLAGS_NON_EFI)\n> >> CFLAGS_efi_selftest_tpl.o := $(CFLAGS_EFI)\n> >> CFLAGS_REMOVE_efi_selftest_tpl.o := $(CFLAGS_NON_EFI)\n> >>\n> >> @@ -20,4 +22,5 @@ obj-$(CONFIG_CMD_BOOTEFI_SELFTEST) += \\\n> >> efi_selftest.o \\\n> >> efi_selftest_console.o \\\n> >> efi_selftest_events.o \\\n> >> +efi_selftest_exitbootservices.o \\\n> >> efi_selftest_tpl.o\n> >> diff --git a/lib/efi_selftest/efi_selftest_exitbootservices.c b/lib/efi_selftest/efi_selftest_exitbootservices.c\n> >> new file mode 100644\n> >> index 0000000000..60271e6180\n> >> --- /dev/null\n> >> +++ b/lib/efi_selftest/efi_selftest_exitbootservices.c\n> >> @@ -0,0 +1,106 @@\n> >> +/*\n> >> + * efi_selftest_events\n> >> + *\n> >> + * Copyright (c) 2017 Heinrich Schuchardt \n> >> + *\n> >> + * SPDX-License-Identifier: GPL-2.0+\n> >> + *\n> >> + * This unit test checks that the notification function of an\n> >> + * EVT_SIGNAL_EXIT_BOOT_SERVICES event is called exactly once.\n> >> + */\n> >> +\n> >> +#include \n> >> +\n> >> +static struct efi_boot_services *boottime;\n> >> +static struct efi_event *event_notify;\n> >> +static unsigned int counter;\n> >\n> > I wonder if the solution to the context thin in the notify() function\n> > is to put all of these in a struct?\n>\n> This would mean replacing\n> boottime->something\n> by\n> config->boottime->something.\n>\n> This does not make the code easier to read.\n\nI don't understand that comment. In general only the outermost\nfunction accesses the global, then passes what is needed to the inner\nfunctions. So in practice you seldom see config->boottime->something.\n\nYou might define a local variable boottime as config->boottime,\nperhaps. But I agree that two levels of access would be bad.\n\n>\n> >\n> > It is nice to group globals into a struct to allow future one-to-many\n> > conversion, reduce the number of symbols in the map and provide a\n> > logical grouping. So this would kill two birds with one stone.\n>\n> I typically do not read map files.\n>\n> With your suggestion the code will be slower and the binary will be larger.\n\nActually it is often faster and smaller. E.g. on ARM every global\naccess requires a literal pool entry and access. You can compare the\ncompiler output and see what you find.\n\n>\n> How would putting all private variables into a structure provide logical\n> grouping?\n\nA structure is a way of grouping.\n\n>\n> >\n> > Another idea is to have setup() return the context (e.g. as a void **\n> > final arg). Then that same context can be passed to execute and\n> > teardown. This is similar to how the unit tests work in U-Boot.\n>\n> Passing structures as void* is error prone.\n>\n> Private variables should stay private. There is no reason to make these\n> accessible outside the unit test.\n\nThat was not my suggestion.\n\n>\n> Passing a void *this would make sense if we would envision having\n> multiple instances of the same unit test. I can't see that.\n\nWell you have set up a test framework of sorts, but it does not use\nthe existing unit test code in U-Boot. That framework uses a test\nstruct which is passed to all functions. It works quite well.\n\n>\n> >\n> > Other than that, see my comments to the previous patch which also apply here.\n>\n> @Alex\n> You already accepted the patch series to efi-next and afterwards merged\n> a bunch of other patches.\n>\n> I could not see any comment by Simon concerning functionality.\n> Everything seemed to focus on style.\n>\n> Shall I provide add-on patches covering Simon's comments or should I\n> create a new version of the patch series.\n\nIf you think there is value in these changes then I suggest doing a\nfollow-on patch.\n\nRegards,\nSimon","headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=lists.denx.de\n\t(client-ip=81.169.180.215; helo=lists.denx.de;\n\tenvelope-from=u-boot-bounces@lists.denx.de;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=google.com header.i=@google.com\n\theader.b=\"CPGCQeet\"; \n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"M4GXK5tW\"; dkim-atps=neutral"],"Received":["from lists.denx.de (dione.denx.de [81.169.180.215])\n\tby ozlabs.org (Postfix) with ESMTP id 3y9SJq2qc9z9tY1\n\tfor ;\n\tMon, 9 Oct 2017 15:42:18 +1100 (AEDT)","by lists.denx.de (Postfix, from userid 105)\n\tid A190DC21CB1; Mon, 9 Oct 2017 04:42:13 +0000 (UTC)","from lists.denx.de (localhost [IPv6:::1])\n\tby lists.denx.de (Postfix) with ESMTP id 7AA1FC21C40;\n\tMon, 9 Oct 2017 04:42:10 +0000 (UTC)","by lists.denx.de (Postfix, from userid 105)\n\tid E33D0C21C40; Mon, 9 Oct 2017 04:42:08 +0000 (UTC)","from mail-qk0-f180.google.com (mail-qk0-f180.google.com\n\t[209.85.220.180])\n\tby lists.denx.de (Postfix) with ESMTPS id 6580FC21C34\n\tfor ; Mon, 9 Oct 2017 04:42:08 +0000 (UTC)","by mail-qk0-f180.google.com with SMTP id r64so22267945qkc.1\n\tfor ; Sun, 08 Oct 2017 21:42:08 -0700 (PDT)","by 10.200.63.170 with HTTP; Sun, 8 Oct 2017 21:41:46 -0700 (PDT)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on lists.denx.de","X-Spam-Level":"","X-Spam-Status":"No, score=-0.0 required=5.0 tests=RCVD_IN_MSPIKE_H3,\n\tRCVD_IN_MSPIKE_WL,\n\tT_DKIM_INVALID autolearn=unavailable autolearn_force=no\n\tversion=3.4.0","DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com;\n\ts=20161025; \n\th=mime-version:sender:in-reply-to:references:from:date:message-id\n\t:subject:to:cc;\n\tbh=l7VnwKhIo9plSmlSZ20z+j8+ljR1wuqSaAZYSgiPgnA=;\n\tb=CPGCQeetXmlquBEg/X7eGlgU6snJ4ZjPwWo+aWU/cwwrZ9i0NiD4GLuTa8zvfUaE+D\n\tEHNCljPOosd/vDFACQ1oZ5P2BZB8H5iYTK8hyrDdr/wIKdOyrKXpjW8WI/I0xiUgcr4k\n\tnXT+M2YOhdeHTXkb3QG0PClFpOVPs1M4EOi1T8weAEjAK4ZM3K4z22WMiy0l2ONqj9C6\n\tcc2MHJJEU7avDnH4ccUThy/wtvnCEfk25WAZ83c56Uo4CJR4DNZVIAxkDagct9+koF9z\n\t2/FSUuMymQh1e4rUXBtmPesKS6IlFxzLAOd+8hMkhRCJg9FME2QNs2yV16Gd9ay+xWQx\n\tessA==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:sender:in-reply-to:references:from:date:message-id\n\t:subject:to:cc;\n\tbh=l7VnwKhIo9plSmlSZ20z+j8+ljR1wuqSaAZYSgiPgnA=;\n\tb=M4GXK5tWWnoPpn59CC3ruc7drXtxDJXWWDLknpoHyuD+aXErtuOBnH/PBK4aiguZpq\n\tCk1sZdUvreAVULdVfVlKNPrJyL8TSmR/zsY7p0cM3SjDEjOOZmSiJfs6WixwAKUx36wD\n\tcZCNraYdUlHpTgH2/apJBYk2Js8FZp9+STG8o="],"X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:sender:in-reply-to:references:from\n\t:date:message-id:subject:to:cc;\n\tbh=l7VnwKhIo9plSmlSZ20z+j8+ljR1wuqSaAZYSgiPgnA=;\n\tb=XwvUCOyMeq0P34F/X1EjkgfCBTxSVsWkPCPUKLWLUQJDg0QlQm56tPkpbe+4b88LkN\n\teLtFwfUQfit3Y6T5pp5UmRpH4OL4LuSuAV2nB0EPlyyi2LlGj2fXGZsVIWQkrRpkBOvN\n\ttBI4BDU9gdEk4TaSaRJddDYuuUydjIbGkN9ACltKtby/apxxvMTSaIm1oYdW+IbyEzAh\n\tJV6axlvUQOJwEktsXrRpEStgRjKwV1eKSrVMr2px2Y2dlhGb4eKdADD4JIVMKImWsh9y\n\tBgu2HTXJg8iaxeL2EDWJyUZl5ugYbPUZXF+sOztMS5MN4jCL+ewLK0D8PJW0wIkDKXTp\n\tcULQ==","X-Gm-Message-State":"AMCzsaVyghX6vl/YmHVBWkaHORhgwSVqI3DTCkaLmz+oe+/SN82mvHAV\n\t0TdWalhRXQSyfs8L08EMf7tLBVsVErtrgCX3YVsDlA==","X-Google-Smtp-Source":"AOwi7QD6QW8GxbbY0/KR1EWAR2+tFg/5Zn1iON4fsMuSW6tSSJRl2SB8pyM48rTjaqgRVdTkq8dtLgvVKWS5yKijOcI=","X-Received":"by 10.55.122.193 with SMTP id v184mr4231779qkc.317.1507524126736;\n\tSun, 08 Oct 2017 21:42:06 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<5fdf8658-3012-7f19-a6ca-ef29571922a4@gmx.de>","References":"<20170915080619.25250-1-xypron.glpk@gmx.de>\n\t<20170915080619.25250-11-xypron.glpk@gmx.de>\n\t\n\t<5fdf8658-3012-7f19-a6ca-ef29571922a4@gmx.de>","From":"Simon Glass ","Date":"Sun, 8 Oct 2017 22:41:46 -0600","X-Google-Sender-Auth":"bHyVKnoZaN3OBjx4TDKR4I02ZtY","Message-ID":"","To":"Heinrich Schuchardt ","Cc":"=?utf-8?q?=C5=81ukasz_Majewski?= ,\n\tAndy Shevchenko , U-Boot Mailing List\n\t, Fabio Estevam ,\n\tMaxime Ripard ","Subject":"Re: [U-Boot] [PATCH 10/10] efi_selftest: check notification of\n\tExitBootServices","X-BeenThere":"u-boot@lists.denx.de","X-Mailman-Version":"2.1.18","Precedence":"list","List-Id":"U-Boot discussion ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"u-boot-bounces@lists.denx.de","Sender":"\"U-Boot\" "}}]