From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Fri, 29 Jan 2021 00:03:52 +0100
Subject: [PATCH] env: Fix warning when forcing environment without
 ENV_ACCESS_IGNORE_FORCE
In-Reply-To: <20210128192633.GY7530@bill-the-cat>
References: <1610360847-21890-1-git-send-email-martin.fuzzey@flowbird.group>
 <692a008f-22d0-e206-9edb-7a5bad124ad5@denx.de>
 <20210128192633.GY7530@bill-the-cat>
Message-ID: <681633b5-3bb1-e90a-56dd-2b2b2ceab1eb@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 1/28/21 8:26 PM, Tom Rini wrote:
> 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?

No, the idea is to completely block the -f flag if 
CONFIG_ENV_ACCESS_IGNORE_FORCE is set from setting anything in the 
environment. That's how I understand the Kconfig entry help text.