[PATCH] env: Fix warning when forcing environment without ENV_ACCESS_IGNORE_FORCE

[Tom Rini]

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 at 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?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210128/3575122d/attachment.sig>