| Message ID | 20200707185139.2225-1-marex@denx.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V2,1/7] env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set | expand |
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 <marex@denx.de> Reviewed-by: Tom Rini <trini@konsulko.com> -- Tom
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 <marex@denx.de> > Reviewed-by: Tom Rini <trini@konsulko.com> Applied to u-boot/master, thanks! -- Tom
On Tue, Jul 7, 2020 at 7:52 PM Marek Vasut <marex@denx.de> 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 <marex@denx.de> > --- > 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
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 <marex@denx.de> >> Reviewed-by: Tom Rini <trini@konsulko.com> > > 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
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 <marex@denx.de> >>> Reviewed-by: Tom Rini <trini@konsulko.com> >> >> 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.
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 <marex@denx.de> >>>> Reviewed-by: Tom Rini <trini@konsulko.com> >>> >>> 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 <addr>". 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
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:
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 <marex at denx.de> --- V2: No change --- env/flags.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)