| Message ID | 20200928092945.584196-1-pieter.degendt@basalte.be |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] package/mbedtls: custom configuration file | expand |
Hello Pieter, Thanks for the proposal. See below for some questions/comments. On Mon, 28 Sep 2020 11:29:45 +0200 Pieter De Gendt <pieter.degendt@gmail.com> wrote: > diff --git a/package/mbedtls/mbedtls.mk b/package/mbedtls/mbedtls.mk > index 5094434e6c..fa74197004 100644 > --- a/package/mbedtls/mbedtls.mk > +++ b/package/mbedtls/mbedtls.mk > @@ -68,4 +68,12 @@ else ifeq ($(BR2_microblaze)$(BR2_MIPS_CPU_MIPS32R6)$(BR2_MIPS_CPU_MIPS64R6),y) > MBEDTLS_POST_CONFIGURE_HOOKS += MBEDTLS_DISABLE_ASM > endif > > +define MBEDTLS_OVERRIDE_CONFIG > + cp $(BR2_PACKAGE_MBEDTLS_CUSTOM_CONFIG) $(@D)/include/mbedtls/config.h > +endef > + > +ifneq ($(call qstrip,$(BR2_PACKAGE_MBEDTLS_CUSTOM_CONFIG)),) > +MBEDTLS_POST_CONFIGURE_HOOKS += MBEDTLS_OVERRIDE_CONFIG > +endif We prefer to enclose everything in the conditional block, and do the qstripping once: MBEDTLS_CUSTOM_CONFIG = $(call qstrip,$(BR2_PACKAGE_MBEDTLS_CUSTOM_CONFIG)) ifneq ($(MBEDTLS_CUSTOM_CONFIG),) define MBEDTLS_OVERRIDE_CONFIG .. endef MBEDTLS_POST_CONFIGURE_HOOKS += MBEDTLS_OVERRIDE_CONFIG endif However, what bothers me a bit is how it completely overrides all the configuration logic that takes place before in mbedtls.mk. For example, if you enable BR2_PACKAGE_MBEDTLS_COMPRESSION=y and use your option to have a custom configuration file, then what BR2_PACKAGE_MBEDTLS_COMPRESSION=y does will be ignored. So, we would have to make BR2_PACKAGE_MBEDTLS_COMPRESSION disappear when BR2_PACKAGE_MBEDTLS_CUSTOM_CONFIG, but then, how do you bring zlib as a build dependency of mbedtls ? We need to decide if the custom configuration file should override all tweaks, or if all tweaks should be applied on top of the custom configuration file. Thomas
Hi Thomas, Thanks for the feedback, see my comments below. On Wed, Sep 30, 2020 at 9:57 PM Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > > Hello Pieter, > > Thanks for the proposal. See below for some questions/comments. > > On Mon, 28 Sep 2020 11:29:45 +0200 > Pieter De Gendt <pieter.degendt@gmail.com> wrote: > > > diff --git a/package/mbedtls/mbedtls.mk b/package/mbedtls/mbedtls.mk > > index 5094434e6c..fa74197004 100644 > > --- a/package/mbedtls/mbedtls.mk > > +++ b/package/mbedtls/mbedtls.mk > > @@ -68,4 +68,12 @@ else ifeq ($(BR2_microblaze)$(BR2_MIPS_CPU_MIPS32R6)$(BR2_MIPS_CPU_MIPS64R6),y) > > MBEDTLS_POST_CONFIGURE_HOOKS += MBEDTLS_DISABLE_ASM > > endif > > > > +define MBEDTLS_OVERRIDE_CONFIG > > + cp $(BR2_PACKAGE_MBEDTLS_CUSTOM_CONFIG) $(@D)/include/mbedtls/config.h > > +endef > > + > > +ifneq ($(call qstrip,$(BR2_PACKAGE_MBEDTLS_CUSTOM_CONFIG)),) > > +MBEDTLS_POST_CONFIGURE_HOOKS += MBEDTLS_OVERRIDE_CONFIG > > +endif > > We prefer to enclose everything in the conditional block, and do the > qstripping once: > > MBEDTLS_CUSTOM_CONFIG = $(call qstrip,$(BR2_PACKAGE_MBEDTLS_CUSTOM_CONFIG)) > > ifneq ($(MBEDTLS_CUSTOM_CONFIG),) > define MBEDTLS_OVERRIDE_CONFIG > .. > endef > MBEDTLS_POST_CONFIGURE_HOOKS += MBEDTLS_OVERRIDE_CONFIG > endif I can adjust that. > > However, what bothers me a bit is how it completely overrides all the > configuration logic that takes place before in mbedtls.mk. For example, > if you enable BR2_PACKAGE_MBEDTLS_COMPRESSION=y and use your option to > have a custom configuration file, then what > BR2_PACKAGE_MBEDTLS_COMPRESSION=y does will be ignored. > > So, we would have to make BR2_PACKAGE_MBEDTLS_COMPRESSION disappear > when BR2_PACKAGE_MBEDTLS_CUSTOM_CONFIG, but then, how do you bring zlib > as a build dependency of mbedtls ? > > We need to decide if the custom configuration file should override all > tweaks, or if all tweaks should be applied on top of the custom > configuration file. I understand why you are bothered with completely overriding the configuration, however I couldn't think of a workable alternative. There are a lot of settings in the config.h file. See: https://tls.mbed.org/api/config_8h.html Agreed, there are more settings that don't require altering the dependency chain, but exposing all in buildroot would be a lot of work and hard to maintain? Applying tweaks on the custom config also feels a bit awkward, since you already took the effort of creating the custom config and the "sed" commands might not even apply. > > Thomas > -- > Thomas Petazzoni, CTO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com Let me know how we can move forward with something you agree with. Best regards, Pieter
Pieter, Thomas, All, On 2020-10-01 09:29 +0200, Pieter De Gendt spake thusly: > On Wed, Sep 30, 2020 at 9:57 PM Thomas Petazzoni > <thomas.petazzoni@bootlin.com> wrote: > > On Mon, 28 Sep 2020 11:29:45 +0200 > > Pieter De Gendt <pieter.degendt@gmail.com> wrote: > > > diff --git a/package/mbedtls/mbedtls.mk b/package/mbedtls/mbedtls.mk > > > index 5094434e6c..fa74197004 100644 > > > --- a/package/mbedtls/mbedtls.mk > > > +++ b/package/mbedtls/mbedtls.mk > > > @@ -68,4 +68,12 @@ else ifeq ($(BR2_microblaze)$(BR2_MIPS_CPU_MIPS32R6)$(BR2_MIPS_CPU_MIPS64R6),y) > > > MBEDTLS_POST_CONFIGURE_HOOKS += MBEDTLS_DISABLE_ASM > > > endif > > > > > > +define MBEDTLS_OVERRIDE_CONFIG > > > + cp $(BR2_PACKAGE_MBEDTLS_CUSTOM_CONFIG) $(@D)/include/mbedtls/config.h > > > +endef > > > + > > > +ifneq ($(call qstrip,$(BR2_PACKAGE_MBEDTLS_CUSTOM_CONFIG)),) > > > +MBEDTLS_POST_CONFIGURE_HOOKS += MBEDTLS_OVERRIDE_CONFIG > > > +endif [--SNIP--] > > However, what bothers me a bit is how it completely overrides all the > > configuration logic that takes place before in mbedtls.mk. For example, > > if you enable BR2_PACKAGE_MBEDTLS_COMPRESSION=y and use your option to > > have a custom configuration file, then what > > BR2_PACKAGE_MBEDTLS_COMPRESSION=y does will be ignored. > > > > So, we would have to make BR2_PACKAGE_MBEDTLS_COMPRESSION disappear > > when BR2_PACKAGE_MBEDTLS_CUSTOM_CONFIG, but then, how do you bring zlib > > as a build dependency of mbedtls ? > > > > We need to decide if the custom configuration file should override all > > tweaks, or if all tweaks should be applied on top of the custom > > configuration file. > > I understand why you are bothered with completely overriding the configuration, > however I couldn't think of a workable alternative. There are a lot of > settings in the > config.h file. See: https://tls.mbed.org/api/config_8h.html > > Agreed, there are more settings that don't require altering the > dependency chain, > but exposing all in buildroot would be a lot of work and hard to maintain? > > Applying tweaks on the custom config also feels a bit awkward, since > you already > took the effort of creating the custom config and the "sed" commands might not > even apply. But that is what we are doing with the user-provided configuration files for: - uClibc-ng - the linux kernel - busybox - uboot - and probably a few others as well... So it would feel natural that we do the same for mbedtls, so that there is a consistent behaviour across the board. Regards, Yann E. MORIN.
Hi Yann, See comment below. On Thu, Oct 1, 2020 at 9:31 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > Pieter, Thomas, All, > > On 2020-10-01 09:29 +0200, Pieter De Gendt spake thusly: > > On Wed, Sep 30, 2020 at 9:57 PM Thomas Petazzoni > > <thomas.petazzoni@bootlin.com> wrote: > > > On Mon, 28 Sep 2020 11:29:45 +0200 > > > Pieter De Gendt <pieter.degendt@gmail.com> wrote: > > > > diff --git a/package/mbedtls/mbedtls.mk b/package/mbedtls/mbedtls.mk > > > > index 5094434e6c..fa74197004 100644 > > > > --- a/package/mbedtls/mbedtls.mk > > > > +++ b/package/mbedtls/mbedtls.mk > > > > @@ -68,4 +68,12 @@ else ifeq ($(BR2_microblaze)$(BR2_MIPS_CPU_MIPS32R6)$(BR2_MIPS_CPU_MIPS64R6),y) > > > > MBEDTLS_POST_CONFIGURE_HOOKS += MBEDTLS_DISABLE_ASM > > > > endif > > > > > > > > +define MBEDTLS_OVERRIDE_CONFIG > > > > + cp $(BR2_PACKAGE_MBEDTLS_CUSTOM_CONFIG) $(@D)/include/mbedtls/config.h > > > > +endef > > > > + > > > > +ifneq ($(call qstrip,$(BR2_PACKAGE_MBEDTLS_CUSTOM_CONFIG)),) > > > > +MBEDTLS_POST_CONFIGURE_HOOKS += MBEDTLS_OVERRIDE_CONFIG > > > > +endif > [--SNIP--] > > > However, what bothers me a bit is how it completely overrides all the > > > configuration logic that takes place before in mbedtls.mk. For example, > > > if you enable BR2_PACKAGE_MBEDTLS_COMPRESSION=y and use your option to > > > have a custom configuration file, then what > > > BR2_PACKAGE_MBEDTLS_COMPRESSION=y does will be ignored. > > > > > > So, we would have to make BR2_PACKAGE_MBEDTLS_COMPRESSION disappear > > > when BR2_PACKAGE_MBEDTLS_CUSTOM_CONFIG, but then, how do you bring zlib > > > as a build dependency of mbedtls ? > > > > > > We need to decide if the custom configuration file should override all > > > tweaks, or if all tweaks should be applied on top of the custom > > > configuration file. > > > > I understand why you are bothered with completely overriding the configuration, > > however I couldn't think of a workable alternative. There are a lot of > > settings in the > > config.h file. See: https://tls.mbed.org/api/config_8h.html > > > > Agreed, there are more settings that don't require altering the > > dependency chain, > > but exposing all in buildroot would be a lot of work and hard to maintain? > > > > Applying tweaks on the custom config also feels a bit awkward, since > > you already > > took the effort of creating the custom config and the "sed" commands might not > > even apply. > > But that is what we are doing with the user-provided configuration files > for: > - uClibc-ng > - the linux kernel > - busybox > - uboot > - and probably a few others as well... > > So it would feel natural that we do the same for mbedtls, so that there > is a consistent behaviour across the board. > > Regards, > Yann E. MORIN. > > -- > .-----------------.--------------------.------------------.--------------------. > | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | > | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | > | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no | > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | > '------------------------------^-------^------------------^--------------------' The packages you list have a kconfig provided by the package itself, for mbedtls it's simply a header file. Do you suggest adding this, and maintaining it within buildroot? Regards, Pieter
Pieter, All, On 2020-10-02 09:16 +0200, Pieter De Gendt spake thusly: > On Thu, Oct 1, 2020 at 9:31 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > On 2020-10-01 09:29 +0200, Pieter De Gendt spake thusly: > > > Applying tweaks on the custom config also feels a bit awkward, since > > > you already > > > took the effort of creating the custom config and the "sed" commands might not > > > even apply. > > But that is what we are doing with the user-provided configuration files > > for: > > - uClibc-ng > > - the linux kernel > > - busybox > > - uboot > > - and probably a few others as well... [--SNIP--] > The packages you list have a kconfig provided by the package itself, > for mbedtls it's > simply a header file. Do you suggest adding this, and maintaining it > within buildroot? What I suggest, and what I think Thomas would also suggest, is that the user-supplied configuration file is copied over the one bundled in the mbedtls source tree, then our tweaks are applied. If the user does not supply any configuration file, we do as we do today. Basically, tht would be that patch (mbedtls.mk only): diff --git a/package/mbedtls/mbedtls.mk b/package/mbedtls/mbedtls.mk index 5094434e6c..487fe9a310 100644 --- a/package/mbedtls/mbedtls.mk +++ b/package/mbedtls/mbedtls.mk @@ -13,6 +13,14 @@ MBEDTLS_INSTALL_STAGING = YES MBEDTLS_LICENSE = Apache-2.0 MBEDTLS_LICENSE_FILES = apache-2.0.txt +MBEDTLS_CONFIG_H = $(call qstrip,$(BR2_PACKAGE_MBEDTLS_CONFIG_H)) +ifneq ($(MBEDTLS_CONFIG_H),) +define MBEDTLS_INSTALL_CONFIG_H + cp $(MBEDTLS_INSTALL_CONFIG_H) $(@D)/include/mbedtls/config.h +endef +MBEDTLS_POST_PATCH_HOOKS += MBEDTLS_INSTALL_CONFIG_H +endif + # This is mandatory for hiawatha ifeq ($(BR2_TOOLCHAIN_HAS_THREADS),y) define MBEDTLS_ENABLE_THREADING Note however, that mbedtls uses a mix of _POST_PATCH and _POST_CONFIGURE hooks to tweak config.h; this is not very sound. Instead, they should all be switched over to using _POST_CONFIGURE hooks, except for hte copy of the user supplied file, which should stay a post-patch hook. Regards, Yann E. MORIN.
diff --git a/package/mbedtls/Config.in b/package/mbedtls/Config.in index a39ba65d98..b5000f422d 100644 --- a/package/mbedtls/Config.in +++ b/package/mbedtls/Config.in @@ -29,4 +29,16 @@ config BR2_PACKAGE_MBEDTLS_COMPRESSION sure CRIME and similar attacks are not applicable to your particular situation. +config BR2_PACKAGE_MBEDTLS_CUSTOM_CONFIG + string "mbedtls custom configuration" + help + Mbed TLS should build out-of-the box on a large variety of + platforms. However, you may need to adjust a few platform- + specific settings or want to customize the set of features + that will be built. You can do all of this in a single + configuration file. + + Provide the location of the custom configuration file or + leave this empty to keep the default. + endif diff --git a/package/mbedtls/mbedtls.mk b/package/mbedtls/mbedtls.mk index 5094434e6c..fa74197004 100644 --- a/package/mbedtls/mbedtls.mk +++ b/package/mbedtls/mbedtls.mk @@ -68,4 +68,12 @@ else ifeq ($(BR2_microblaze)$(BR2_MIPS_CPU_MIPS32R6)$(BR2_MIPS_CPU_MIPS64R6),y) MBEDTLS_POST_CONFIGURE_HOOKS += MBEDTLS_DISABLE_ASM endif +define MBEDTLS_OVERRIDE_CONFIG + cp $(BR2_PACKAGE_MBEDTLS_CUSTOM_CONFIG) $(@D)/include/mbedtls/config.h +endef + +ifneq ($(call qstrip,$(BR2_PACKAGE_MBEDTLS_CUSTOM_CONFIG)),) +MBEDTLS_POST_CONFIGURE_HOOKS += MBEDTLS_OVERRIDE_CONFIG +endif + $(eval $(cmake-package))
mbedlts is customizable with a single configuration header file. Instead of altering the existing config.h file based on the selection of packages, we can simply replace it. Signed-off-by: Pieter De Gendt <pieter.degendt@basalte.be> --- package/mbedtls/Config.in | 12 ++++++++++++ package/mbedtls/mbedtls.mk | 8 ++++++++ 2 files changed, 20 insertions(+)