[PATCH] Revert "env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set"

[Alex Kiernan]

On Sat, Aug 29, 2020 at 6:51 PM Marek Vasut <marex at 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 at gmail.com> wrote:
> >>
> >> On Thu, Aug 27, 2020 at 2:03 PM Tom Rini <trini at 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 at 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


-- 
Alex Kiernan