On Tue, Jan 21, 2020 at 10:40:23AM -0500, Tom Rini wrote: > On Tue, Jan 21, 2020 at 02:48:52PM +0900, AKASHI Takahiro wrote: > > On Fri, Jan 17, 2020 at 06:26:34AM +0100, Heinrich Schuchardt wrote: > > > On 1/17/20 2:53 AM, AKASHI Takahiro wrote: > > > >On Tue, Jan 14, 2020 at 01:04:58PM +0100, Heinrich Schuchardt wrote: > > > >>On 1/14/20 8:33 AM, AKASHI Takahiro wrote: > > > >>>On Wed, Jan 08, 2020 at 06:43:51PM +0100, Heinrich Schuchardt wrote: > > > >>>> > > > >>>> > > > >>>>On 11/21/19 1:11 AM, AKASHI Takahiro wrote: > > > >>>>>In this patch, a very simple test is added to verify that > > > >>>>>rsa_verify() > > > >>>>>using rsa_verify_with_pkey() work correctly. > > > >>>>> > > > >>>>>To keep the code simple, all the test data, either public key and > > > >>>>>verified binary data, are embedded in the source. > > > >>>>> > > > >>>>>Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org> > > > >>>>>--- > > > >>>>> test/Kconfig | 12 +++ > > > >>>>> test/lib/Makefile | 1 + > > > >>>>> test/lib/rsa.c | 206 > > > >>>>> ++++++++++++++++++++++++++++++++++++++++++++++ > > > >>>>> 3 files changed, 219 insertions(+) > > > >>>>> create mode 100644 test/lib/rsa.c > > > >>>>> > > > >>>>>diff --git a/test/Kconfig b/test/Kconfig > > > >>>>>index cb7954041eda..64d76c3b20a5 100644 > > > >>>>>--- a/test/Kconfig > > > >>>>>+++ b/test/Kconfig > > > >>>>>@@ -28,6 +28,18 @@ config UT_LIB_ASN1 > > > >>>>> Enables a test which exercises asn1 compiler and decoder > > > >>>>> function > > > >>>>> via various parsers. > > > >>>>> > > > >>>>>+config UT_LIB_RSA > > > >>>>>+ bool "Unit test for rsa_verify() function" > > > >>>>>+ imply RSA > > > >>>>>+ imply ASYMMETRIC_KEY_TYPE > > > >>>>>+ imply ASYMMETRIC_PUBLIC_KEY_SUBTYPE > > > >>>>>+ imply RSA_PUBLIC_KEY_PARSER > > > >>>>>+ imply RSA_VERIFY_WITH_PKEY > > > >>>> > > > >>>>This test should depend on RSA_VERIFY_WITH_PKEY because is cannot be > > > >>>>executed otherwise. > > > >>> > > > >>>See below. > > > >>> > > > >>>>Best regards > > > >>>> > > > >>>>Heinrich > > > >>>> > > > >>>>>+ default y > > > >>>>>+ help > > > >>>>>+ Enables rsa_verify() test, currently rsa_verify_with_pkey > > > >>>>>only() > > > >>>>>+ only, at the 'ut lib' command. > > > >>>>>+ > > > >>>>> endif > > > >>>>> > > > >>>>> config UT_TIME > > > >>>>>diff --git a/test/lib/Makefile b/test/lib/Makefile > > > >>>>>index 72d2ec74b5f4..2bf6ef3935bb 100644 > > > >>>>>--- a/test/lib/Makefile > > > >>>>>+++ b/test/lib/Makefile > > > >>>>>@@ -8,3 +8,4 @@ obj-y += lmb.o > > > >>>>> obj-y += string.o > > > >>>>> obj-$(CONFIG_ERRNO_STR) += test_errno_str.o > > > >>>>> obj-$(CONFIG_UT_LIB_ASN1) += asn1.o > > > >>>>>+obj-$(CONFIG_UT_LIB_RSA) += rsa.o > > > >>>>>diff --git a/test/lib/rsa.c b/test/lib/rsa.c > > > >>>>>new file mode 100644 > > > >>>>>index 000000000000..44f8ade226f4 > > > >>>>>--- /dev/null > > > >>>>>+++ b/test/lib/rsa.c > > > >>>>>@@ -0,0 +1,206 @@ > > > >>>>>+// SPDX-License-Identifier: GPL-2.0+ > > > >>>>>+/* > > > >>>>>+ * Copyright (c) 2019 Linaro Limited > > > >>>>>+ * Author: AKASHI Takahiro > > > >>>>>+ * > > > >>>>>+ * Unit test for rsa_verify() function > > > >>>>>+ */ > > > >>>>>+ > > > >>>>>+#include <common.h> > > > >>>>>+#include <command.h> > > > >>>>>+#include <image.h> > > > >>>>>+#include <test/lib.h> > > > >>>>>+#include <test/test.h> > > > >>>>>+#include <test/ut.h> > > > >>>>>+#include <u-boot/rsa.h> > > > >>>>>+ > > > >>>>>+#ifdef CONFIG_RSA_VERIFY_WITH_PKEY > > > >>>> > > > >>>>Please, remove this ifdef. Simply do not build this module if > > > >>>>CONFIG_RSA_VERIFY_WITH_PKEY is not defined. > > > >>> > > > >>>That is why I added this #ifdef here to avoid a build error. > > > >>>I think it is a good idea to make test code as generic as possible > > > >>>to easily add more test cases. > > > >> > > > >>It is preferable to move configuration dependencies to the Makefile. > > > >> > > > >>You can create a new C file if you have tests needing a different > > > >>configuration. > > > >> > > > >>test/lib/rsa_verify.c might be a better name for the file. > > > >> > > > >>In test/Kconfig, please, replace > > > >> > > > >>config UT_LIB_RSA > > > >> bool "Unit test for rsa_verify() function" > > > >> imply RSA > > > >> > > > >>by > > > >> > > > >>config UT_LIB_RSA > > > >> bool "Unit test for rsa_verify() function" > > > >> default y > > > >> depends on RSA > > > > > > > >Initially in my development code, I did the exact same thing, > > > >but I didn't adopt this approach. > > > > > > > >If we follow your proposal, UT_LIB_RSA won't be selected > > > >unless RSA is enabled. > > > >So UT_LIB_RSA won't be executed under most existing configurations > > > >in the current CI. This was not what I wanted. > > > > > > > >What I wanted here is that, if UT (and UT_LIB) is selected, all > > > >the configurations needed for enabling all the test cases be > > > >also selected *automatically*. > > > > > > What I want is that if I run 'make menuconfig' and select CONFIG_UT > > > manually I end up with a configuration that compiles and that does what > > > I have configured. > > > > Hmm, that is what I intend to do here, but the code doesn't work > > as I expected. It works only if CONFIG_UT_LIB_RSA is on in *defconfig. > > > > So I have no option other than changing reverse dependencies as follows, > > CONFIG_UT_LIB_RSA > > select IMAGE_SIGN_INFO > > select RSA > > imply RSA_VERIFY_WITH_PKEY > > > > Using 'select' is still safe as CONFIG_RSA has no dependency. > > (Please note we can still use 'imply' for VERIFY_WITH_PKEY here.) > > > > Tom, do you agree to this change? > > OK, my fault for not spotting it before. Using imply (as we do today on > UT_LIB_ASN1) is wrong. All of the tests should "depends on" what they > need in order to function. Thanks!
I know that using "depends on" is an obvious solution here, but in that case, we won't run those tests automatically in CI unless the dependency targets, like CONFIG_RSA, are explicitly or implicitly enabled by *defconfig. Is that OK for you? -Takahiro Akashi > -- > Tom
