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

[Ilias Apalodimas]

Hi Andy,

[...]

> > > > > 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?
>

There's a script to do that in dts/update-dts-subtree.sh

> 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".

Us. Sure it's going to take some time, but eventually we should be
able to isolate options that rarely need a change

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

I am not sure tbh. Let me backpaddle from my previous suggestion :).
Looking at it closer config.h is copied from mbedTLS, commenting out
features we don't want.

What we could do instead is either trim those comments out and only
define the options we need for now. That would make the patch easily
reviewable. If in the future we decide we need a more modular
approach, we can retrofit it without breaking anything.

Alternatively, we can undef the options we don't want in our header,
but I am not sure how often these change or how scalable this is going
to end up being. I prefer option #1

Regards
/Ilias

>
> --
> With Best Regards,
> Andy Shevchenko
>
>