Re: [PATCH v3 03/25] mbedtls: add mbedtls into the build system

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, Jun 05, 2024 at 12:35:37PM +0300, Ilias Apalodimas wrote:
> On Wed, 5 Jun 2024 at 12:30, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Tue, Jun 04, 2024 at 05:50:08PM -0400, Raymond Mao wrote:
> > > On Tue, 4 Jun 2024 at 16:17, Andy Shevchenko <
> > > andriy.shevchenko@linux.intel.com> wrote:
> > > > On Tue, May 28, 2024 at 07:09:14AM -0700, Raymond Mao wrote:

...

> > > > > This patch series requires mbedtls git repo to be added as a
> > > > > subtree to the main U-Boot repo via:
> > > > >
> > > > > $ git subtree add --prefix lib/mbedtls/external/mbedtls \
> > > > >       https://github.com/Mbed-TLS/mbedtls.git \
> > > > >       v3.6.0 --squash
> > > >
> > > > Is this approach maintainable?
> > > > I don't remember if we have similar in Linux kernel, for example.
> > > > (There are few candidates like compression algorithms that are usually
> > > > being
> > > >  hosted elsewhere)
> >
> > No answer?
> 
> subtrees is what was decided on OF_UPSRTEAM as well. If you have a
> better idea feel free to propose it, but for the sake of conformance
> we are better off doing the same thing on every external tree we pull
> in

How do they will (or already do) maintain this?

At least it's a good to have a few words on the choice made in the cover
letter, so we will have no questions on it.

> > > > > Moreover, due to the Windows-style files from mbedtls git repo,
> > > > > we need to convert the CRLF endings to LF and do a commit manually:
> > > > >
> > > > > $ git add --renormalize .
> > > > > $ git commit

...

> > > > >  lib/mbedtls/mbedtls_def_config.h | 4262 ++++++++++++++++++++++++++++++
> > > >
> > > > This is ridiculously HUGE! This is unreviewable. Moreover, this is even
> > > > hard to
> > > > configure by the user! Can you rather make it modular and maybe create a
> > > > separate documentation for the most important options (I do not believe one
> > > > needs _all_ of them to be set / tuned)?
> > > >
> > > > This is a file from MbedTLS and follows its own style.
> > > And this is how MbedTLS is configured - with all features listed in a
> > > config file and
> > > commenting out the unused features with "//").
> > > The modification here is just to control those existing options with
> > > Kconfigs.
> >
> > And why should we blindly follow this nonsense?
> 
> It's easier to follow up future changes tbh. But I do agree the config
> file is huge. Perhaps splitting in 2 files is going to be easier
> mbedtls_def_config.h -> contains all the options that rarely need
> tuning, which I assume is the majority of the header
> mbedtls_usef_config.h -> contains the imporant options, crypto,
> checksum algorithms etc
> 
> Thoughts?

The problem is on who decides which are "rarely need". The feasible (to me)
approach is to split per domain. Like you listed at the very end of your reply.
We can also learn from managing MTA configurations, such as Exim4 where user
may decide if they want a single file or split version.

-- 
With Best Regards,
Andy Shevchenko