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

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

[Alex Kiernan]

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(-)

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:
-- 
2.17.1

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

[Marek Vasut]

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.

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

[Tom Rini]

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.

-- 
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/20200827/92d6e52d/attachment.sig>

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

[Alex Kiernan]

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.

-- 
Alex Kiernan

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

[Alex Kiernan]

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.

-- 
Alex Kiernan

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

[Marek Vasut]

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 ?

[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@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


-- 
Alex Kiernan

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

[Marek Vasut]

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.