()Hi Jan,
On Mon, 6 Feb 2023 at 22:49, Jan Kiszka <jan.kis...@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.kis...@siemens.com> wrote:
> >>
> >> From: Jan Kiszka <jan.kis...@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.hershber...@ni.com>
> >> CC: Marek Vasut <ma...@denx.de>
> >> CC: Stefan Herbrechtsmeier <stefan.herbrechtsmeier-...@weidmueller.com>
> >> Signed-off-by: Jan Kiszka <jan.kis...@siemens.com>
> >> Reviewed-by: Marek Vasut <ma...@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
