diff mbox series

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

Message ID 941b4847617426ff7e8cc4ce7ed6d5c5012c2440.1664958832.git.jan.kiszka@siemens.com
State Superseded
Delegated to: Tom Rini
Headers show
Series IOT2050-related enhancements | expand

Commit Message

Jan Kiszka Oct. 5, 2022, 8:33 a.m. UTC 
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(+)

Comments

Stefan Herbrechtsmeier Oct. 27, 2022, 7:49 a.m. UTC | #1 
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()?

> +		} 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
Jan Kiszka Oct. 27, 2022, 12:38 p.m. UTC | #2 
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
>
Stefan Herbrechtsmeier Oct. 28, 2022, 8:34 a.m. UTC | #3 
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@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.

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
diff mbox series

Patch

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;
+		} 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];
 }