On Thu, Jan 28, 2021 at 08:07:54PM +0100, Marek Vasut wrote:
On 1/11/21 11:27 AM, Martin Fuzzey wrote:
Since commit 0f036bf4b87e ("env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set") a warning message is displayed when setenv -f is used WITHOUT CONFIG_ENV_ACCESS_IGNORE_FORCE, but the variable is set anyway, resulting in lots of log pollution.
env_flags_validate() returns 0 if the access is accepted, or non zero if it is refused.
So the original code #ifndef CONFIG_ENV_ACCESS_IGNORE_FORCE if (flag & H_FORCE) return 0; #endif
was correct, it returns 0 (accepts the modification) if forced UNLESS IGNORE_FORCE is set (in which case access checks in the following code are applied). The broken patch just added a printf to the force accepted case.
To obtain the intent of the patch we need this: if (flag & H_FORCE) { #ifdef CONFIG_ENV_ACCESS_IGNORE_FORCE printf("## Error: Can't force access to "%s"\n", name); #else return 0; #endif }
Fixes: 0f036bf4b87e ("env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set")
Signed-off-by: Martin Fuzzey martin.fuzzey@flowbird.group
env/flags.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/env/flags.c b/env/flags.c index df4aed2..e3e833c 100644 --- a/env/flags.c +++ b/env/flags.c @@ -563,12 +563,13 @@ int env_flags_validate(const struct env_entry *item, const char *newval, return 1; #endif -#ifndef CONFIG_ENV_ACCESS_IGNORE_FORCE if (flag & H_FORCE) { +#ifdef CONFIG_ENV_ACCESS_IGNORE_FORCE printf("## Error: Can't force access to "%s"\n", name); +#else return 0;
- } #endif
Based on env/Kconfig description of this option:
config ENV_ACCESS_IGNORE_FORCE bool "Block forced environment operations" default n help If defined, don't allow the -f switch to env set override variable access flags.
I would think the code should look like this:
#ifdef CONFIG_ENV_ACCESS_IGNORE_FORCE if (flag & H_FORCE) { printf("## Error: Can't force access to "%s"\n", name); return 1; } #else if (flag & H_FORCE) return 0; #endif
So, prior to 0f036bf4b87e we had what you're suggesting, and that lead to 8a5cdf601f8d (which is the commit I was trying to think of) which Heinrich did not like, but was what was needed to get things to function again. Wouldn't what you're proposing break the use case you had in the first place?