[PATCH V2 01/13] env: Complete generic support for writable list

[Stefan Herbrechtsmeier]

Hi Jan,

Am 27.10.2022 um 14:38 schrieb Jan Kiszka:
> On 27.10.22 09:49, Stefan Herbrechtsmeier wrote:
>> Hi,
>>
>> Am 05.10.2022 um 10:33 schrieb Jan Kiszka:
>>> From: Jan Kiszka <jan.kiszka at siemens.com>
>>>
>>> This completes what 890feecaab72 started by selecting ENV_APPEND and
>>> ENV_IS_NOWHERE and by moving this driver to top if the list. This
>>> ensures that load operations pick up both the default env and the
>>> permitted parts of the next-prio location. When writing though, we must
>>> use the regular ordering.
>>>
>>> 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>
>>> Signed-off-by: Jan Kiszka <jan.kiszka at siemens.com>
>>> ---
>>>    env/Kconfig |  2 ++
>>>    env/env.c   | 22 ++++++++++++++++++++++
>>>    2 files changed, 24 insertions(+)
>>>
>>> diff --git a/env/Kconfig b/env/Kconfig
>>> index 24111dfaf47..05b5fbf17d1 100644
>>> --- a/env/Kconfig
>>> +++ b/env/Kconfig
>>> @@ -715,6 +715,8 @@ config ENV_APPEND
>>>      config ENV_WRITEABLE_LIST
>>>        bool "Permit write access only to listed variables"
>>> +    select ENV_IS_NOWHERE
>>> +    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 69848fb0608..d0649f9540d 100644
>>> --- a/env/env.c
>>> +++ b/env/env.c
>>> @@ -133,6 +133,28 @@ __weak enum env_location
>>> arch_env_get_location(enum env_operation op, int prio)
>>>        if (prio >= ARRAY_SIZE(env_locations))
>>>            return ENVL_UNKNOWN;
>>>    +    if (IS_ENABLED(CONFIG_ENV_WRITEABLE_LIST)) {
>>> +        /*
>>> +         * In writeable-list mode, ENVL_NOWHERE gains highest prio by
>>> +         * virtually injecting it at prio 0.
>>> +         */
>>> +        if (prio == 0) {
>>> +            /*
>>> +             * Avoid the injection for write operations as that
>>> +             * would block it.
>>> +             */
>>> +            if (op != ENVOP_SAVE && op != ENVOP_ERASE)
>>> +                return ENVL_NOWHERE;
>> Is it consensus now to use ENVL_NOWHERE as synonym for default
>> environment? If I remember correct this was rejected in the past and
>> ENVL_NOWHERE  should only be used if no enviroment is available.
>>
>> Why don't you call env_set_default(NULL, 0) in env_load() before
>> env_driver_lookup()?
> Worth to explore... if that should avoid this logic here... Let me try.

Additionally we have remove the `#if !CONFIG_IS_ENABLED(ENV_APPEND) 
arround the `return 0` in env_load() and the following change in env_export

--- a/env/common.c
+++ b/env/common.c
@@ -234,7 +234,7 @@ int env_export(env_t *env_out)
      ssize_t    len;

      res = (char *)env_out->data;
-    len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL);
+    len = hexport_r(&env_htab, '\0', H_EXTERNAL, &res, ENV_SIZE, 0, NULL);
      if (len < 0) {
          pr_err("Cannot export environment: errno = %d\n", errno);
          return 1;

I can't remeber why the H_EXTERNAL in env_export was needed.

Regards

   Stefan