| Message ID | 20200827094342.25780-1-alex.kiernan@gmail.com |
|---|---|
| State | Rejected |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | Revert "env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set" | expand |
On 8/27/20 11:43 AM, Alex Kiernan wrote: > This reverts commit 0f036bf4b87e6416f5c4d23865a62a62d9073c20. > > With this change applied and CONFIG_ENV_ACCESS_IGNORE_FORCE disabled, > the warning appears on every force overwrite, but the variable is then > written to: > > => 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 > > Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com> > --- > As I noted in my email, I can't see an immediately obvious way to make > this work as intended and given we're at -rc3, I think a revert is the > most appropriate way forward. Can you please spend some more time on proper fix, instead of just reverting ? Reverting isn't the way forward most of the time.
On Thu, Aug 27, 2020 at 11:49:25AM +0200, Marek Vasut wrote: > On 8/27/20 11:43 AM, Alex Kiernan wrote: > > This reverts commit 0f036bf4b87e6416f5c4d23865a62a62d9073c20. > > > > With this change applied and CONFIG_ENV_ACCESS_IGNORE_FORCE disabled, > > the warning appears on every force overwrite, but the variable is then > > written to: > > > > => 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 > > > > Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com> > > --- > > As I noted in my email, I can't see an immediately obvious way to make > > this work as intended and given we're at -rc3, I think a revert is the > > most appropriate way forward. > > Can you please spend some more time on proper fix, instead of just > reverting ? Reverting isn't the way forward most of the time. Can you assist in that? So far the original patch broke the EFI tests, but Heinrich and I came up with a compromise (but it's also I suspect that Alex saw). It is probably better to go back to the same behavior as the last release and try again next release once the problem can be addressed by someone.
On Thu, Aug 27, 2020 at 2:03 PM Tom Rini <trini@konsulko.com> wrote: > > On Thu, Aug 27, 2020 at 11:49:25AM +0200, Marek Vasut wrote: > > On 8/27/20 11:43 AM, Alex Kiernan wrote: > > > This reverts commit 0f036bf4b87e6416f5c4d23865a62a62d9073c20. > > > > > > With this change applied and CONFIG_ENV_ACCESS_IGNORE_FORCE disabled, > > > the warning appears on every force overwrite, but the variable is then > > > written to: > > > > > > => 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 > > > > > > Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com> > > > --- > > > As I noted in my email, I can't see an immediately obvious way to make > > > this work as intended and given we're at -rc3, I think a revert is the > > > most appropriate way forward. > > > > Can you please spend some more time on proper fix, instead of just > > reverting ? Reverting isn't the way forward most of the time. > > Can you assist in that? So far the original patch broke the EFI tests, > but Heinrich and I came up with a compromise (but it's also I suspect > that Alex saw). It is probably better to go back to the same behavior > as the last release and try again next release once the problem can be > addressed by someone. > I had another play with it and think I have something that'll work, though I leave my current job at the end of the week, so if it's not right I'm unlikely to be able to fix it. That said, I think there's something which has changed because I now need force in order to set things like ethaddr in a boot script (where it's already been set in the board code) whereas I didn't before.
On Thu, Aug 27, 2020 at 5:20 PM Alex Kiernan <alex.kiernan@gmail.com> wrote: > > On Thu, Aug 27, 2020 at 2:03 PM Tom Rini <trini@konsulko.com> wrote: > > > > On Thu, Aug 27, 2020 at 11:49:25AM +0200, Marek Vasut wrote: > > > On 8/27/20 11:43 AM, Alex Kiernan wrote: > > > > This reverts commit 0f036bf4b87e6416f5c4d23865a62a62d9073c20. > > > > > > > > With this change applied and CONFIG_ENV_ACCESS_IGNORE_FORCE disabled, > > > > the warning appears on every force overwrite, but the variable is then > > > > written to: > > > > > > > > => 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 > > > > > > > > Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com> > > > > --- > > > > As I noted in my email, I can't see an immediately obvious way to make > > > > this work as intended and given we're at -rc3, I think a revert is the > > > > most appropriate way forward. > > > > > > Can you please spend some more time on proper fix, instead of just > > > reverting ? Reverting isn't the way forward most of the time. > > > > Can you assist in that? So far the original patch broke the EFI tests, > > but Heinrich and I came up with a compromise (but it's also I suspect > > that Alex saw). It is probably better to go back to the same behavior > > as the last release and try again next release once the problem can be > > addressed by someone. > > > > I had another play with it and think I have something that'll work, > though I leave my current job at the end of the week, so if it's not > right I'm unlikely to be able to fix it. > Sorry, I ran out of time... so it's highly unlikely I'll get to look at this any time soon. Unfortunately I don't have access to that tree anymore either.
On 8/29/20 4:14 PM, Alex Kiernan wrote: > On Thu, Aug 27, 2020 at 5:20 PM Alex Kiernan <alex.kiernan@gmail.com> wrote: >> >> On Thu, Aug 27, 2020 at 2:03 PM Tom Rini <trini@konsulko.com> wrote: >>> >>> On Thu, Aug 27, 2020 at 11:49:25AM +0200, Marek Vasut wrote: >>>> On 8/27/20 11:43 AM, Alex Kiernan wrote: >>>>> This reverts commit 0f036bf4b87e6416f5c4d23865a62a62d9073c20. >>>>> >>>>> With this change applied and CONFIG_ENV_ACCESS_IGNORE_FORCE disabled, >>>>> the warning appears on every force overwrite, but the variable is then >>>>> written to: >>>>> >>>>> => 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 >>>>> >>>>> Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com> >>>>> --- >>>>> As I noted in my email, I can't see an immediately obvious way to make >>>>> this work as intended and given we're at -rc3, I think a revert is the >>>>> most appropriate way forward. >>>> >>>> Can you please spend some more time on proper fix, instead of just >>>> reverting ? Reverting isn't the way forward most of the time. >>> >>> Can you assist in that? So far the original patch broke the EFI tests, >>> but Heinrich and I came up with a compromise (but it's also I suspect >>> that Alex saw). It is probably better to go back to the same behavior >>> as the last release and try again next release once the problem can be >>> addressed by someone. >>> >> >> I had another play with it and think I have something that'll work, >> though I leave my current job at the end of the week, so if it's not >> right I'm unlikely to be able to fix it. >> > > Sorry, I ran out of time... so it's highly unlikely I'll get to look > at this any time soon. Unfortunately I don't have access to that tree > anymore either. OK, so that means the work is now pushed on me. I will try to schedule this in the near future, because currently I am highly overloaded. Can you please at least share some information on what the approach you took was ?
On Sat, Aug 29, 2020 at 6:51 PM Marek Vasut <marex@denx.de> wrote: > > On 8/29/20 4:14 PM, Alex Kiernan wrote: > > On Thu, Aug 27, 2020 at 5:20 PM Alex Kiernan <alex.kiernan@gmail.com> wrote: > >> > >> On Thu, Aug 27, 2020 at 2:03 PM Tom Rini <trini@konsulko.com> wrote: > >>> > >>> On Thu, Aug 27, 2020 at 11:49:25AM +0200, Marek Vasut wrote: > >>>> On 8/27/20 11:43 AM, Alex Kiernan wrote: > >>>>> This reverts commit 0f036bf4b87e6416f5c4d23865a62a62d9073c20. > >>>>> > >>>>> With this change applied and CONFIG_ENV_ACCESS_IGNORE_FORCE disabled, > >>>>> the warning appears on every force overwrite, but the variable is then > >>>>> written to: > >>>>> > >>>>> => 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 > >>>>> > >>>>> Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com> > >>>>> --- > >>>>> As I noted in my email, I can't see an immediately obvious way to make > >>>>> this work as intended and given we're at -rc3, I think a revert is the > >>>>> most appropriate way forward. > >>>> > >>>> Can you please spend some more time on proper fix, instead of just > >>>> reverting ? Reverting isn't the way forward most of the time. > >>> > >>> Can you assist in that? So far the original patch broke the EFI tests, > >>> but Heinrich and I came up with a compromise (but it's also I suspect > >>> that Alex saw). It is probably better to go back to the same behavior > >>> as the last release and try again next release once the problem can be > >>> addressed by someone. > >>> > >> > >> I had another play with it and think I have something that'll work, > >> though I leave my current job at the end of the week, so if it's not > >> right I'm unlikely to be able to fix it. > >> > > > > Sorry, I ran out of time... so it's highly unlikely I'll get to look > > at this any time soon. Unfortunately I don't have access to that tree > > anymore either. > > OK, so that means the work is now pushed on me. I will try to schedule > this in the near future, because currently I am highly overloaded. Can > you please at least share some information on what the approach you took > was ? Sure, it was something like this, completely untested though (and pasting into gmail seems to insist on eating tabs): diff --git a/env/flags.c b/env/flags.c index df4aed26b2..a9d629c1fc 100644 --- a/env/flags.c +++ b/env/flags.c @@ -527,6 +527,7 @@ int env_flags_validate(const struct env_entry *item, const char *newval, { const char *name; const char *oldval = NULL; + bool ok; if (op != env_op_create) oldval = item->data; @@ -563,23 +564,21 @@ 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); + if (!CONFIG_IS_ENABLED(ENV_ACCESS_IGNORE_FORCE) && (flag & H_FORCE)) return 0; - } -#endif + + ok = true; switch (op) { case env_op_delete: if (item->flags & ENV_FLAGS_VARACCESS_PREVENT_DELETE) { printf("## Error: Can't delete \"%s\"\n", name); - return 1; + ok = false; } break; case env_op_overwrite: if (item->flags & ENV_FLAGS_VARACCESS_PREVENT_OVERWR) { printf("## Error: Can't overwrite \"%s\"\n", name); - return 1; + ok = false; } else if (item->flags & ENV_FLAGS_VARACCESS_PREVENT_NONDEF_OVERWR) { const char *defval = env_get_default(name); @@ -590,19 +589,22 @@ int env_flags_validate(const struct env_entry *item, const char *newval, if (strcmp(oldval, defval) != 0) { printf("## Error: Can't overwrite \"%s\"\n", name); - return 1; + ok = false; } } break; case env_op_create: if (item->flags & ENV_FLAGS_VARACCESS_PREVENT_CREATE) { printf("## Error: Can't create \"%s\"\n", name); - return 1; + ok = false; } break; } - return 0; + if (!ok) + printf("## Error: Can't force access to \"%s\"\n", name); + + return ok ? 0 : 1; } #endif
On 8/27/20 3:03 PM, Tom Rini wrote: > On Thu, Aug 27, 2020 at 11:49:25AM +0200, Marek Vasut wrote: >> On 8/27/20 11:43 AM, Alex Kiernan wrote: >>> This reverts commit 0f036bf4b87e6416f5c4d23865a62a62d9073c20. >>> >>> With this change applied and CONFIG_ENV_ACCESS_IGNORE_FORCE disabled, >>> the warning appears on every force overwrite, but the variable is then >>> written to: >>> >>> => 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 >>> >>> Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com> >>> --- >>> As I noted in my email, I can't see an immediately obvious way to make >>> this work as intended and given we're at -rc3, I think a revert is the >>> most appropriate way forward. >> >> Can you please spend some more time on proper fix, instead of just >> reverting ? Reverting isn't the way forward most of the time. > > Can you assist in that? So far the original patch broke the EFI tests, > but Heinrich and I came up with a compromise (but it's also I suspect > that Alex saw). It is probably better to go back to the same behavior > as the last release and try again next release once the problem can be > addressed by someone. I have to wonder how could the original patch break anything, when it only adds a single printf() statement. The issue described above would manifest either way -- with or without the patch -- and is thus a separate issue altogether. So I believe this patch is wrong.
diff --git a/env/flags.c b/env/flags.c index df4aed26b2c6..4a73c31670f4 100644 --- a/env/flags.c +++ b/env/flags.c @@ -564,10 +564,8 @@ int env_flags_validate(const struct env_entry *item, const char *newval, #endif #ifndef CONFIG_ENV_ACCESS_IGNORE_FORCE - if (flag & H_FORCE) { - printf("## Error: Can't force access to \"%s\"\n", name); + if (flag & H_FORCE) return 0; - } #endif switch (op) { case env_op_delete:
This reverts commit 0f036bf4b87e6416f5c4d23865a62a62d9073c20. With this change applied and CONFIG_ENV_ACCESS_IGNORE_FORCE disabled, the warning appears on every force overwrite, but the variable is then written to: => 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 Signed-off-by: Alex Kiernan <alex.kiernan@gmail.com> --- As I noted in my email, I can't see an immediately obvious way to make this work as intended and given we're at -rc3, I think a revert is the most appropriate way forward. env/flags.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)