From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tom Rini Date: Mon, 1 Feb 2021 14:31:03 -0500 Subject: [PATCH] env: Fix warning when forcing environment without ENV_ACCESS_IGNORE_FORCE In-Reply-To: <681633b5-3bb1-e90a-56dd-2b2b2ceab1eb@denx.de> References: <1610360847-21890-1-git-send-email-martin.fuzzey@flowbird.group> <692a008f-22d0-e206-9edb-7a5bad124ad5@denx.de> <20210128192633.GY7530@bill-the-cat> <681633b5-3bb1-e90a-56dd-2b2b2ceab1eb@denx.de> Message-ID: <20210201193103.GF7199@bill-the-cat> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Fri, Jan 29, 2021 at 12:03:52AM +0100, Marek Vasut wrote: > 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 > > > > --- > > > > 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. So was this all a "by inspection" bug then and not something you ran in to in use? I'm a bit worried that "how it's implemented" is relied upon more than "how it's documented in the help", esp since the former is probably older than the latter. -- Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 659 bytes Desc: not available URL: