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