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

[Jan Kiszka <jan.kiszka@siemens.com>]

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@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@ni.com>
>> CC: Marek Vasut <marex@denx.de>
>> Signed-off-by: Jan Kiszka <jan.kiszka@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.

Jan

> 
>> +        } else {
>> +            /*
>> +             * always subtract 1, also for writing because
>> +             * env_load_prio, which is used for writing, was
>> +             * initialized with that offset.
>> +             */
>> +            prio--;
>> +        }
>> +    }
>> +
>>       return env_locations[prio];
>>   }
>>   
> 
> Regards
> 
>   Stefan
> 

-- 
Siemens AG, Technology
Competence Center Embedded Linux