Re: [PATCH V2 1/7] env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set
Am 23.10.2020 um 11:52 schrieb Marek Vasut: > On 10/23/20 10:58 AM, Simon Goldschmidt wrote: >> Am 31.07.2020 um 23:40 schrieb Tom Rini: >>> On Tue, Jul 07, 2020 at 08:51:33PM +0200, Marek Vasut wrote: >>> If the ENV_ACCESS_IGNORE_FORCE is set, inform user that the variable cannot be force-set if such attempt happens. Signed-off-by: Marek Vasut Reviewed-by: Tom Rini >>> >>> Applied to u-boot/master, thanks! >>> >> >> Sorry I haven't followed this and wasn't able to report this earlier, >> but I think this is wrong: This warning is issued when 0 is returned, >> which means the change is accepted, not rejected. > > I think there was a subsequent discussion on this topic in the ML, > [PATCH] Revert "env: Warn on force access if ENV_ACCESS_IGNORE_FORCE > set", I think we reached a conclusion this patch was OK. But if you did > more digging and found a problem, please send a patch / provide details. > I use a script that reads ethaddrs from external storage and then use "env set -f ethaddr ". With v2020.10, I now get a warning that this can't be written, but I still see the value later with 'env print'. I think this should just be reverted. I'll try to find the thread discussing the revert patch you mentioned. Regards, Simon
Re: [PATCH V2 1/7] env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set
On 10/23/20 10:58 AM, Simon Goldschmidt wrote: > Am 31.07.2020 um 23:40 schrieb Tom Rini: >> On Tue, Jul 07, 2020 at 08:51:33PM +0200, Marek Vasut wrote: >> >>> If the ENV_ACCESS_IGNORE_FORCE is set, inform user that the variable >>> cannot be force-set if such attempt happens. >>> >>> Signed-off-by: Marek Vasut >>> Reviewed-by: Tom Rini >> >> Applied to u-boot/master, thanks! >> > > Sorry I haven't followed this and wasn't able to report this earlier, > but I think this is wrong: This warning is issued when 0 is returned, > which means the change is accepted, not rejected. I think there was a subsequent discussion on this topic in the ML, [PATCH] Revert "env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set", I think we reached a conclusion this patch was OK. But if you did more digging and found a problem, please send a patch / provide details.
Re: [PATCH V2 1/7] env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set
Am 31.07.2020 um 23:40 schrieb Tom Rini: > On Tue, Jul 07, 2020 at 08:51:33PM +0200, Marek Vasut wrote: > >> If the ENV_ACCESS_IGNORE_FORCE is set, inform user that the variable >> cannot be force-set if such attempt happens. >> >> Signed-off-by: Marek Vasut >> Reviewed-by: Tom Rini > > Applied to u-boot/master, thanks! > Sorry I haven't followed this and wasn't able to report this earlier, but I think this is wrong: This warning is issued when 0 is returned, which means the change is accepted, not rejected. Regards, Simon
Re: [PATCH V2 1/7] env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set
On Tue, Jul 7, 2020 at 7:52 PM Marek Vasut wrote:
>
> If the ENV_ACCESS_IGNORE_FORCE is set, inform user that the variable
> cannot be force-set if such attempt happens.
>
> Signed-off-by: Marek Vasut
> ---
> V2: No change
> ---
> env/flags.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/env/flags.c b/env/flags.c
> index b88fe7ba9c..f7a53775c4 100644
> --- a/env/flags.c
> +++ b/env/flags.c
> @@ -524,8 +524,10 @@ int env_flags_validate(const struct env_entry *item,
> const char *newval,
>
> /* check for access permission */
> #ifndef CONFIG_ENV_ACCESS_IGNORE_FORCE
> - if (flag & H_FORCE)
> + if (flag & H_FORCE) {
> + printf("## Error: Can't force access to \"%s\"\n", name);
> return 0;
> + }
> #endif
> switch (op) {
> case env_op_delete:
AFAICT this is wrong - you get the message when you have
CONFIG_ENV_ACCESS_IGNORE_FORCE disabled and use force:
=> env print ethaddr
ethaddr=00:1C:2B:08:AF:65
=> env set ethaddr 00:00:00:00:00:00
## Error: Can't overwrite "ethaddr"
## Error inserting "ethaddr" variable, errno=1
=> env print ethaddr
ethaddr=00:1C:2B:08:AF:65
=> env set -f ethaddr 00:00:00:00:00:00
## Error: Can't force access to "ethaddr"
=> env print ethaddr
ethaddr=00:00:00:00:00:00
Just staring at the code, I don't see a good way to capture this
behaviour, other than wiring it into each of the branches of the
switch - I started off with something like this:
diff --git a/env/flags.c b/env/flags.c
index df4aed26b2c6..70621dff4434 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) {
- printf("## Error: Can't force access to \"%s\"\n", name);
- return 0;
+ if (CONFIG_IS_ENABLED(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) {
But I think with that you'll get the message for variables which can
be overwritten, so still not what's intended.
--
Alex Kiernan
Re: [PATCH V2 1/7] env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set
On Tue, Jul 07, 2020 at 08:51:33PM +0200, Marek Vasut wrote: > If the ENV_ACCESS_IGNORE_FORCE is set, inform user that the variable > cannot be force-set if such attempt happens. > > Signed-off-by: Marek Vasut > Reviewed-by: Tom Rini Applied to u-boot/master, thanks! -- Tom signature.asc Description: PGP signature
Re: [PATCH V2 1/7] env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set
On Tue, Jul 07, 2020 at 08:51:33PM +0200, Marek Vasut wrote: > If the ENV_ACCESS_IGNORE_FORCE is set, inform user that the variable > cannot be force-set if such attempt happens. > > Signed-off-by: Marek Vasut Reviewed-by: Tom Rini -- Tom signature.asc Description: PGP signature
[PATCH V2 1/7] env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set
If the ENV_ACCESS_IGNORE_FORCE is set, inform user that the variable
cannot be force-set if such attempt happens.
Signed-off-by: Marek Vasut
---
V2: No change
---
env/flags.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/env/flags.c b/env/flags.c
index b88fe7ba9c..f7a53775c4 100644
--- a/env/flags.c
+++ b/env/flags.c
@@ -524,8 +524,10 @@ int env_flags_validate(const struct env_entry *item, const
char *newval,
/* check for access permission */
#ifndef CONFIG_ENV_ACCESS_IGNORE_FORCE
- if (flag & H_FORCE)
+ if (flag & H_FORCE) {
+ printf("## Error: Can't force access to \"%s\"\n", name);
return 0;
+ }
#endif
switch (op) {
case env_op_delete:
--
2.27.0
