[PATCH 1/2] env: Complete generic support for writable list

[Simon Glass]

()Hi Jan,

On Mon, 6 Feb 2023 at 22:49, Jan Kiszka <jan.kiszka at siemens.com> wrote:
>
> On 07.02.23 05:02, Simon Glass wrote:
> > Hi Jan,
> >
> > On Fri, 3 Feb 2023 at 05:23, Jan Kiszka <jan.kiszka at siemens.com> wrote:
> >>
> >> From: Jan Kiszka <jan.kiszka at siemens.com>
> >>
> >> This completes what 890feecaab72 started by selecting ENV_APPEND and
> >> loading the default env before any other sources. This ensures that load
> >> operations pick up all non-writable vars from the default env and only
> >> permitted parts from other locations according to the regular
> >> priorities.
> >>
> >> With this change, boards only need to define the list of writable
> >> variables but no longer have to provide a custom env_get_location
> >> implementation.
> >>
> >> CC: Joe Hershberger <joe.hershberger at ni.com>
> >> CC: Marek Vasut <marex at denx.de>
> >> CC: Stefan Herbrechtsmeier <stefan.herbrechtsmeier-oss at weidmueller.com>
> >> Signed-off-by: Jan Kiszka <jan.kiszka at siemens.com>
> >> Reviewed-by: Marek Vasut <marex at denx.de>
> >> ---
> >>  env/Kconfig | 1 +
> >>  env/env.c   | 8 ++++++++
> >>  2 files changed, 9 insertions(+)
> >>
> >> diff --git a/env/Kconfig b/env/Kconfig
> >> index c409ea71fe5..6e24eee55f2 100644
> >> --- a/env/Kconfig
> >> +++ b/env/Kconfig
> >> @@ -733,6 +733,7 @@ config ENV_APPEND
> >>
> >>  config ENV_WRITEABLE_LIST
> >>         bool "Permit write access only to listed variables"
> >> +       select ENV_APPEND
> >>         help
> >>           If defined, only environment variables which explicitly set the 'w'
> >>           writeable flag can be written and modified at runtime. No variables
> >> diff --git a/env/env.c b/env/env.c
> >> index 06078c7f374..45e638fcd1f 100644
> >> --- a/env/env.c
> >> +++ b/env/env.c
> >> @@ -195,6 +195,14 @@ int env_load(void)
> >>         int best_prio = -1;
> >>         int prio;
> >>
> >> +       if (CONFIG_IS_ENABLED(ENV_WRITEABLE_LIST)) {
> >> +               /*
> >> +                * When using a list of writeable variables, the baseline comes
> >> +                * from the built-in default env. So load this first.
> >> +                */
> >> +               env_set_default(NULL, 0);
> >> +       }
> >> +
> >>         for (prio = 0; (drv = env_driver_lookup(ENVOP_LOAD, prio)); prio++) {
> >>                 int ret;
> >>
> >> --
> >> 2.35.3
> >>
> >
> > This looks OK, but please can you write some tests in test/env ?
>
> Looking at those, it seems there is nothing at all regarding access
> flags yet. Any suggestions how to first of all build up that
> infrastructure are welcome. Then I could add this aspect here on top.

Yes, starting something a bit new is always harder. My approach is
normally to just start small. Anything is better than nothing, and it
provides something for others to build on.

There are a few tests for hashtable which could be a way to start this.

I suggest starting a new env.c file in there with a few env_get() and
env_set() calls. You could also use himport_r() to test out different
flags.

I'm not sure how easy it would be to call env_load() in a test, but it
might work.

The trickier thing (perhaps to worry about later) is that you are
(correctly) using a CONFIG option to enable the feature. For sandbox
tests we typically want all features to be enabled at build time (e.g.
default y if SANDBOX), then select them at runtime, so we can test
them. See test_eth_enabled() for an example of how this is done.

Regards,
Simon