Hi Jan,

On Fri, 3 Feb 2023 at 05:23, Jan Kiszka jan.kiszka@siemens.com wrote:

From: Jan Kiszka jan.kiszka@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@ni.com CC: Marek Vasut marex@denx.de CC: Stefan Herbrechtsmeier stefan.herbrechtsmeier-oss@weidmueller.com Signed-off-by: Jan Kiszka jan.kiszka@siemens.com Reviewed-by: Marek Vasut marex@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 ?

Regards, SImon

()Hi Jan,

On Mon, 6 Feb 2023 at 22:49, Jan Kiszka jan.kiszka@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@siemens.com wrote:

...

From: Jan Kiszka jan.kiszka@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@ni.com CC: Marek Vasut marex@denx.de CC: Stefan Herbrechtsmeier stefan.herbrechtsmeier-oss@weidmueller.com Signed-off-by: Jan Kiszka jan.kiszka@siemens.com Reviewed-by: Marek Vasut marex@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