* [PATCH] env: Fix warning when forcing environment without ENV_ACCESS_IGNORE_FORCE
2021-01-11 10:27 [PATCH] env: Fix warning when forcing environment without ENV_ACCESS_IGNORE_FORCE Martin Fuzzey
@ 2021-01-15 18:43 ` Tom Rini
2021-01-28 16:25 ` Tom Rini
2021-01-28 9:11 ` Heinrich Schuchardt
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Tom Rini @ 2021-01-15 18:43 UTC (permalink / raw)
To: u-boot
On Mon, Jan 11, 2021 at 11:27:20AM +0100, 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
> + }
> switch (op) {
> case env_op_delete:
> if (item->flags & ENV_FLAGS_VARACCESS_PREVENT_DELETE) {
Marek, does this look right to you? Heinrich, I think this means
there''s a follow-up commit that I made to one of the tests that can
probably be reverted as well? Thanks for digging in to this Martin!
--
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/20210115/15268e04/attachment.sig>
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH] env: Fix warning when forcing environment without ENV_ACCESS_IGNORE_FORCE
2021-01-15 18:43 ` Tom Rini
@ 2021-01-28 16:25 ` Tom Rini
0 siblings, 0 replies; 15+ messages in thread
From: Tom Rini @ 2021-01-28 16:25 UTC (permalink / raw)
To: u-boot
On Fri, Jan 15, 2021 at 01:43:44PM -0500, Tom Rini wrote:
> On Mon, Jan 11, 2021 at 11:27:20AM +0100, 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
> > + }
> > switch (op) {
> > case env_op_delete:
> > if (item->flags & ENV_FLAGS_VARACCESS_PREVENT_DELETE) {
>
> Marek, does this look right to you? Heinrich, I think this means
> there''s a follow-up commit that I made to one of the tests that can
> probably be reverted as well? Thanks for digging in to this Martin!
Marek? Heinrich? I really want a little feedback on this patch since I
think it addresses a tricky problem. Thanks.
--
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/947c0eef/attachment.sig>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] env: Fix warning when forcing environment without ENV_ACCESS_IGNORE_FORCE
2021-01-11 10:27 [PATCH] env: Fix warning when forcing environment without ENV_ACCESS_IGNORE_FORCE Martin Fuzzey
2021-01-15 18:43 ` Tom Rini
@ 2021-01-28 9:11 ` Heinrich Schuchardt
2021-01-28 18:59 ` Marek Vasut
2021-01-28 19:07 ` Marek Vasut
2021-04-18 12:45 ` Tom Rini
3 siblings, 1 reply; 15+ messages in thread
From: Heinrich Schuchardt @ 2021-01-28 9:11 UTC (permalink / raw)
To: u-boot
On 11.01.21 11:27, 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,
Please, add a Sphinx style function description in include/env_flags.h
explaining exactly what this function is meant to do.
Cf.
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation
> 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);
If this is an error, why don't you return 1 (failure)?
Please, use log_err() for error messages.
> +#else
> return 0;
Why shouldn't the other tests be executed?
Best regards
Heinrich
> - }
> #endif
> + }
> switch (op) {
> case env_op_delete:
> if (item->flags & ENV_FLAGS_VARACCESS_PREVENT_DELETE) {
>
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH] env: Fix warning when forcing environment without ENV_ACCESS_IGNORE_FORCE
2021-01-28 9:11 ` Heinrich Schuchardt
@ 2021-01-28 18:59 ` Marek Vasut
0 siblings, 0 replies; 15+ messages in thread
From: Marek Vasut @ 2021-01-28 18:59 UTC (permalink / raw)
To: u-boot
On 1/28/21 10:11 AM, Heinrich Schuchardt wrote:
[...]
>> 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,
>
> Please, add a Sphinx style function description in include/env_flags.h
> explaining exactly what this function is meant to do.
>
> Cf.
> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation
This is irrelevant, it's a bugfix so docs/improvements are a separate patch.
>> 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);
>
> If this is an error, why don't you return 1 (failure)?
> Please, use log_err() for error messages.
>
>> +#else
>> return 0;
>
> Why shouldn't the other tests be executed?
Because the code is a total mess and thus error prone.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] env: Fix warning when forcing environment without ENV_ACCESS_IGNORE_FORCE
2021-01-11 10:27 [PATCH] env: Fix warning when forcing environment without ENV_ACCESS_IGNORE_FORCE Martin Fuzzey
2021-01-15 18:43 ` Tom Rini
2021-01-28 9:11 ` Heinrich Schuchardt
@ 2021-01-28 19:07 ` Marek Vasut
2021-01-28 19:26 ` Tom Rini
2021-01-28 19:37 ` Fuzzey, Martin
2021-04-18 12:45 ` Tom Rini
3 siblings, 2 replies; 15+ messages in thread
From: Marek Vasut @ 2021-01-28 19:07 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH] env: Fix warning when forcing environment without ENV_ACCESS_IGNORE_FORCE
2021-01-28 19:07 ` Marek Vasut
@ 2021-01-28 19:26 ` Tom Rini
2021-01-28 23:03 ` Marek Vasut
2021-01-28 19:37 ` Fuzzey, Martin
1 sibling, 1 reply; 15+ messages in thread
From: Tom Rini @ 2021-01-28 19:26 UTC (permalink / raw)
To: u-boot
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?
--
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>
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH] env: Fix warning when forcing environment without ENV_ACCESS_IGNORE_FORCE
2021-01-28 19:26 ` Tom Rini
@ 2021-01-28 23:03 ` Marek Vasut
2021-02-01 19:31 ` Tom Rini
0 siblings, 1 reply; 15+ messages in thread
From: Marek Vasut @ 2021-01-28 23:03 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH] env: Fix warning when forcing environment without ENV_ACCESS_IGNORE_FORCE
2021-01-28 23:03 ` Marek Vasut
@ 2021-02-01 19:31 ` Tom Rini
2021-02-03 16:39 ` Marek Vasut
0 siblings, 1 reply; 15+ messages in thread
From: Tom Rini @ 2021-02-01 19:31 UTC (permalink / raw)
To: u-boot
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 <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.
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: <https://lists.denx.de/pipermail/u-boot/attachments/20210201/a4b49527/attachment.sig>
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH] env: Fix warning when forcing environment without ENV_ACCESS_IGNORE_FORCE
2021-02-01 19:31 ` Tom Rini
@ 2021-02-03 16:39 ` Marek Vasut
0 siblings, 0 replies; 15+ messages in thread
From: Marek Vasut @ 2021-02-03 16:39 UTC (permalink / raw)
To: u-boot
On 2/1/21 8:31 PM, Tom Rini wrote:
> 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 <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.
>
> 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.
The usecase of the writeable list is to completely block the -f flag.
Maybe some further cleanup of the env config options is required,
because it is poorly documented it seems.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] env: Fix warning when forcing environment without ENV_ACCESS_IGNORE_FORCE
2021-01-28 19:07 ` Marek Vasut
2021-01-28 19:26 ` Tom Rini
@ 2021-01-28 19:37 ` Fuzzey, Martin
2021-01-28 23:05 ` Marek Vasut
1 sibling, 1 reply; 15+ messages in thread
From: Fuzzey, Martin @ 2021-01-28 19:37 UTC (permalink / raw)
To: u-boot
Hi Marek,
On Thu, 28 Jan 2021 at 20:07, Marek Vasut <marex@denx.de> wrote:
>
> 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
I don't think that is right.
If you do that force update will be refused when
CONFIG_ENV_ACCESS_IGNORE_FORCE, regardless if the access would be
valid without the force
The kconfig options says don't allow the -f switch to OVERRIDE
variable access flags, not "don't allow the -f switch to be used"
I think we want this truth table
-f used ENV_ACCESS_IGNORE _FORCE Access valid ====> result
N X
N refused
N X
Y accepted
Y N
Y accepted
Y N
N accepted (forced)
Y Y
Y accepted
Y Y
N refused with warning
Regards,
Martin
--
Martin Fuzzey | Software Architect
Transport Intelligence Division
Parc La Fayette - 6 rue Isaac Newton | Besan?on | 25075 | FR
Office: +33381546880 Mobile: +33677158582
martin.fuzzey at flowbird.group | www.flowbird.group/transport
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH] env: Fix warning when forcing environment without ENV_ACCESS_IGNORE_FORCE
2021-01-28 19:37 ` Fuzzey, Martin
@ 2021-01-28 23:05 ` Marek Vasut
2021-01-29 7:42 ` Fuzzey, Martin
0 siblings, 1 reply; 15+ messages in thread
From: Marek Vasut @ 2021-01-28 23:05 UTC (permalink / raw)
To: u-boot
On 1/28/21 8:37 PM, Fuzzey, Martin wrote:
> Hi Marek,
Hi,
> On Thu, 28 Jan 2021 at 20:07, Marek Vasut <marex@denx.de> wrote:
>>
>> 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
>
> I don't think that is right.
>
> If you do that force update will be refused when
> CONFIG_ENV_ACCESS_IGNORE_FORCE, regardless if the access would be
> valid without the force
That's how I understand the option was intended to work, based on the
Kconfig help text.
> The kconfig options says don't allow the -f switch to OVERRIDE
> variable access flags, not "don't allow the -f switch to be used"
I suspect the help text needs clarification then.
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH] env: Fix warning when forcing environment without ENV_ACCESS_IGNORE_FORCE
2021-01-28 23:05 ` Marek Vasut
@ 2021-01-29 7:42 ` Fuzzey, Martin
2021-01-29 11:16 ` Marek Vasut
0 siblings, 1 reply; 15+ messages in thread
From: Fuzzey, Martin @ 2021-01-29 7:42 UTC (permalink / raw)
To: u-boot
On Fri, 29 Jan 2021 at 00:05, Marek Vasut <marex@denx.de> wrote:
>
> On 1/28/21 8:37 PM, Fuzzey, Martin wrote:
> > Hi Marek,
>
> Hi,
>
> > On Thu, 28 Jan 2021 at 20:07, Marek Vasut <marex@denx.de> wrote:
> >>
> >> 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
> >
> > I don't think that is right.
> >
> > If you do that force update will be refused when
> > CONFIG_ENV_ACCESS_IGNORE_FORCE, regardless if the access would be
> > valid without the force
>
> That's how I understand the option was intended to work, based on the
> Kconfig help text.
>
Ok we are understanding different things about what it's supposed to do then.
But if everyone agrees that the option should completely disable -f
then I'm fine with that, in which case the version of the patch you
propose is good (and easier to understand).
The problem I have is with the current 0f036bf4b87e which leads to log
spam on boards that *don't* have CONFIG_ENV_ACCESS_IGNORE_FORCE set
and do use -f.
> > The kconfig options says don't allow the -f switch to OVERRIDE
> > variable access flags, not "don't allow the -f switch to be used"
>
> I suspect the help text needs clarification then.
Agreed
Martin
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH] env: Fix warning when forcing environment without ENV_ACCESS_IGNORE_FORCE
2021-01-29 7:42 ` Fuzzey, Martin
@ 2021-01-29 11:16 ` Marek Vasut
0 siblings, 0 replies; 15+ messages in thread
From: Marek Vasut @ 2021-01-29 11:16 UTC (permalink / raw)
To: u-boot
On 1/29/21 8:42 AM, Fuzzey, Martin wrote:
> On Fri, 29 Jan 2021 at 00:05, Marek Vasut <marex@denx.de> wrote:
>>
>> On 1/28/21 8:37 PM, Fuzzey, Martin wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> On Thu, 28 Jan 2021 at 20:07, Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> 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
>>>
>>> I don't think that is right.
>>>
>>> If you do that force update will be refused when
>>> CONFIG_ENV_ACCESS_IGNORE_FORCE, regardless if the access would be
>>> valid without the force
>>
>> That's how I understand the option was intended to work, based on the
>> Kconfig help text.
>>
>
> Ok we are understanding different things about what it's supposed to do then.
>
> But if everyone agrees that the option should completely disable -f
> then I'm fine with that, in which case the version of the patch you
> propose is good (and easier to understand).
>
> The problem I have is with the current 0f036bf4b87e which leads to log
> spam on boards that *don't* have CONFIG_ENV_ACCESS_IGNORE_FORCE set
> and do use -f.
Yes, that should be fixed, one way or the other.
(in fact, can you also write a test for this case ?)
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] env: Fix warning when forcing environment without ENV_ACCESS_IGNORE_FORCE
2021-01-11 10:27 [PATCH] env: Fix warning when forcing environment without ENV_ACCESS_IGNORE_FORCE Martin Fuzzey
` (2 preceding siblings ...)
2021-01-28 19:07 ` Marek Vasut
@ 2021-04-18 12:45 ` Tom Rini
3 siblings, 0 replies; 15+ messages in thread
From: Tom Rini @ 2021-04-18 12:45 UTC (permalink / raw)
To: u-boot
On Mon, Jan 11, 2021 at 11:27:20AM +0100, 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>
Applied to u-boot/master, thanks!
--
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/20210418/9f8bc0d5/attachment.sig>
^ permalink raw reply [flat|nested] 15+ messages in thread