diff mbox series

[3/6] env: Fix invalid env handling in env_init()

Message ID 20200603000111.7919-3-marex@denx.de
State Superseded
Headers show
Series [1/6] env: Warn on force access if ENV_ACCESS_IGNORE_FORCE set | expand

Commit Message

Marek Vasut June 3, 2020, 12:01 a.m. UTC
In case the env storage driver marks environment as ENV_INVALID, we must
reset the $ret return value to -ENOENT to let the env init code reset the
environment to the default one a bit further down.

Signed-off-by: Marek Vasut <marex at denx.de>
---
 env/env.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Tom Rini June 5, 2020, 7:07 p.m. UTC | #1
On Wed, Jun 03, 2020 at 02:01:08AM +0200, Marek Vasut wrote:

> In case the env storage driver marks environment as ENV_INVALID, we must
> reset the $ret return value to -ENOENT to let the env init code reset the
> environment to the default one a bit further down.
> 
> Signed-off-by: Marek Vasut <marex at denx.de>
> ---
>  env/env.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/env/env.c b/env/env.c
> index dcc25c030b..024d36fdbe 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -300,6 +300,9 @@ int env_init(void)
>  
>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,
>  		      drv->name, ret);
> +
> +		if (gd->env_valid == ENV_INVALID)
> +			ret = -ENOENT;
>  	}
>  
>  	if (!prio)

Is the storage driver marking the environment as invalid but not
returning ENOENT valid?

How does all of this work in the case of multiple configured storage
drivers?

Thanks!
Marek Vasut June 5, 2020, 8:47 p.m. UTC | #2
On 6/5/20 9:07 PM, Tom Rini wrote:
> On Wed, Jun 03, 2020 at 02:01:08AM +0200, Marek Vasut wrote:
> 
>> In case the env storage driver marks environment as ENV_INVALID, we must
>> reset the $ret return value to -ENOENT to let the env init code reset the
>> environment to the default one a bit further down.
>>
>> Signed-off-by: Marek Vasut <marex at denx.de>
>> ---
>>  env/env.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/env/env.c b/env/env.c
>> index dcc25c030b..024d36fdbe 100644
>> --- a/env/env.c
>> +++ b/env/env.c
>> @@ -300,6 +300,9 @@ int env_init(void)
>>  
>>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,
>>  		      drv->name, ret);
>> +
>> +		if (gd->env_valid == ENV_INVALID)
>> +			ret = -ENOENT;
>>  	}
>>  
>>  	if (!prio)
> 
> Is the storage driver marking the environment as invalid but not
> returning ENOENT valid?

Yes, some are doing that.

> How does all of this work in the case of multiple configured storage
> drivers?

If the env is invalid, then we report it as invalid.
Tom Rini June 5, 2020, 9:11 p.m. UTC | #3
On Fri, Jun 05, 2020 at 10:47:24PM +0200, Marek Vasut wrote:
> On 6/5/20 9:07 PM, Tom Rini wrote:
> > On Wed, Jun 03, 2020 at 02:01:08AM +0200, Marek Vasut wrote:
> > 
> >> In case the env storage driver marks environment as ENV_INVALID, we must
> >> reset the $ret return value to -ENOENT to let the env init code reset the
> >> environment to the default one a bit further down.
> >>
> >> Signed-off-by: Marek Vasut <marex at denx.de>
> >> ---
> >>  env/env.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/env/env.c b/env/env.c
> >> index dcc25c030b..024d36fdbe 100644
> >> --- a/env/env.c
> >> +++ b/env/env.c
> >> @@ -300,6 +300,9 @@ int env_init(void)
> >>  
> >>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,
> >>  		      drv->name, ret);
> >> +
> >> +		if (gd->env_valid == ENV_INVALID)
> >> +			ret = -ENOENT;
> >>  	}
> >>  
> >>  	if (!prio)
> > 
> > Is the storage driver marking the environment as invalid but not
> > returning ENOENT valid?
> 
> Yes, some are doing that.

Why?  Is that correct or incorrect?  It doesn't seem like this is
something that should be inconsistent from storage driver to storage
driver and needs fixing.

> > How does all of this work in the case of multiple configured storage
> > drivers?
> 
> If the env is invalid, then we report it as invalid.

Right.  And what change, if any, does your proposed change have in this
case?  Thanks!
Marek Vasut June 6, 2020, 2:54 p.m. UTC | #4
On 6/5/20 11:11 PM, Tom Rini wrote:
> On Fri, Jun 05, 2020 at 10:47:24PM +0200, Marek Vasut wrote:
>> On 6/5/20 9:07 PM, Tom Rini wrote:
>>> On Wed, Jun 03, 2020 at 02:01:08AM +0200, Marek Vasut wrote:
>>>
>>>> In case the env storage driver marks environment as ENV_INVALID, we must
>>>> reset the $ret return value to -ENOENT to let the env init code reset the
>>>> environment to the default one a bit further down.
>>>>
>>>> Signed-off-by: Marek Vasut <marex at denx.de>
>>>> ---
>>>>  env/env.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/env/env.c b/env/env.c
>>>> index dcc25c030b..024d36fdbe 100644
>>>> --- a/env/env.c
>>>> +++ b/env/env.c
>>>> @@ -300,6 +300,9 @@ int env_init(void)
>>>>  
>>>>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,
>>>>  		      drv->name, ret);
>>>> +
>>>> +		if (gd->env_valid == ENV_INVALID)
>>>> +			ret = -ENOENT;
>>>>  	}
>>>>  
>>>>  	if (!prio)
>>>
>>> Is the storage driver marking the environment as invalid but not
>>> returning ENOENT valid?
>>
>> Yes, some are doing that.
> 
> Why?  Is that correct or incorrect?  It doesn't seem like this is
> something that should be inconsistent from storage driver to storage
> driver and needs fixing.

The default env driver is doing it, whether it's a workaround or correct
behavior, I really don't know. Maybe Joe does ?

>>> How does all of this work in the case of multiple configured storage
>>> drivers?
>>
>> If the env is invalid, then we report it as invalid.
> 
> Right.  And what change, if any, does your proposed change have in this
> case?  Thanks!

Before this patch, that check was missing and the result was random,
depending on which env order you had.
Tom Rini June 6, 2020, 4:24 p.m. UTC | #5
On Sat, Jun 06, 2020 at 04:54:52PM +0200, Marek Vasut wrote:
> On 6/5/20 11:11 PM, Tom Rini wrote:
> > On Fri, Jun 05, 2020 at 10:47:24PM +0200, Marek Vasut wrote:
> >> On 6/5/20 9:07 PM, Tom Rini wrote:
> >>> On Wed, Jun 03, 2020 at 02:01:08AM +0200, Marek Vasut wrote:
> >>>
> >>>> In case the env storage driver marks environment as ENV_INVALID, we must
> >>>> reset the $ret return value to -ENOENT to let the env init code reset the
> >>>> environment to the default one a bit further down.
> >>>>
> >>>> Signed-off-by: Marek Vasut <marex at denx.de>
> >>>> ---
> >>>>  env/env.c | 3 +++
> >>>>  1 file changed, 3 insertions(+)
> >>>>
> >>>> diff --git a/env/env.c b/env/env.c
> >>>> index dcc25c030b..024d36fdbe 100644
> >>>> --- a/env/env.c
> >>>> +++ b/env/env.c
> >>>> @@ -300,6 +300,9 @@ int env_init(void)
> >>>>  
> >>>>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,
> >>>>  		      drv->name, ret);
> >>>> +
> >>>> +		if (gd->env_valid == ENV_INVALID)
> >>>> +			ret = -ENOENT;
> >>>>  	}
> >>>>  
> >>>>  	if (!prio)
> >>>
> >>> Is the storage driver marking the environment as invalid but not
> >>> returning ENOENT valid?
> >>
> >> Yes, some are doing that.
> > 
> > Why?  Is that correct or incorrect?  It doesn't seem like this is
> > something that should be inconsistent from storage driver to storage
> > driver and needs fixing.
> 
> The default env driver is doing it, whether it's a workaround or correct
> behavior, I really don't know. Maybe Joe does ?
> 
> >>> How does all of this work in the case of multiple configured storage
> >>> drivers?
> >>
> >> If the env is invalid, then we report it as invalid.
> > 
> > Right.  And what change, if any, does your proposed change have in this
> > case?  Thanks!
> 
> Before this patch, that check was missing and the result was random,
> depending on which env order you had.

So have you changed the behavior of multiple environments then?  Today
it's indeed link order based, which is not optimal but used.
Marek Vasut June 8, 2020, 9:45 p.m. UTC | #6
On 6/6/20 6:24 PM, Tom Rini wrote:
> On Sat, Jun 06, 2020 at 04:54:52PM +0200, Marek Vasut wrote:
>> On 6/5/20 11:11 PM, Tom Rini wrote:
>>> On Fri, Jun 05, 2020 at 10:47:24PM +0200, Marek Vasut wrote:
>>>> On 6/5/20 9:07 PM, Tom Rini wrote:
>>>>> On Wed, Jun 03, 2020 at 02:01:08AM +0200, Marek Vasut wrote:
>>>>>
>>>>>> In case the env storage driver marks environment as ENV_INVALID, we must
>>>>>> reset the $ret return value to -ENOENT to let the env init code reset the
>>>>>> environment to the default one a bit further down.
>>>>>>
>>>>>> Signed-off-by: Marek Vasut <marex at denx.de>
>>>>>> ---
>>>>>>  env/env.c | 3 +++
>>>>>>  1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/env/env.c b/env/env.c
>>>>>> index dcc25c030b..024d36fdbe 100644
>>>>>> --- a/env/env.c
>>>>>> +++ b/env/env.c
>>>>>> @@ -300,6 +300,9 @@ int env_init(void)
>>>>>>  
>>>>>>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,
>>>>>>  		      drv->name, ret);
>>>>>> +
>>>>>> +		if (gd->env_valid == ENV_INVALID)
>>>>>> +			ret = -ENOENT;
>>>>>>  	}
>>>>>>  
>>>>>>  	if (!prio)
>>>>>
>>>>> Is the storage driver marking the environment as invalid but not
>>>>> returning ENOENT valid?
>>>>
>>>> Yes, some are doing that.
>>>
>>> Why?  Is that correct or incorrect?  It doesn't seem like this is
>>> something that should be inconsistent from storage driver to storage
>>> driver and needs fixing.
>>
>> The default env driver is doing it, whether it's a workaround or correct
>> behavior, I really don't know. Maybe Joe does ?
>>
>>>>> How does all of this work in the case of multiple configured storage
>>>>> drivers?
>>>>
>>>> If the env is invalid, then we report it as invalid.
>>>
>>> Right.  And what change, if any, does your proposed change have in this
>>> case?  Thanks!
>>
>> Before this patch, that check was missing and the result was random,
>> depending on which env order you had.
> 
> So have you changed the behavior of multiple environments then?  Today
> it's indeed link order based, which is not optimal but used.

Yes, I believe this patch makes it work as intended.
Tom Rini June 8, 2020, 10:57 p.m. UTC | #7
On Mon, Jun 08, 2020 at 11:45:18PM +0200, Marek Vasut wrote:
> On 6/6/20 6:24 PM, Tom Rini wrote:
> > On Sat, Jun 06, 2020 at 04:54:52PM +0200, Marek Vasut wrote:
> >> On 6/5/20 11:11 PM, Tom Rini wrote:
> >>> On Fri, Jun 05, 2020 at 10:47:24PM +0200, Marek Vasut wrote:
> >>>> On 6/5/20 9:07 PM, Tom Rini wrote:
> >>>>> On Wed, Jun 03, 2020 at 02:01:08AM +0200, Marek Vasut wrote:
> >>>>>
> >>>>>> In case the env storage driver marks environment as ENV_INVALID, we must
> >>>>>> reset the $ret return value to -ENOENT to let the env init code reset the
> >>>>>> environment to the default one a bit further down.
> >>>>>>
> >>>>>> Signed-off-by: Marek Vasut <marex at denx.de>
> >>>>>> ---
> >>>>>>  env/env.c | 3 +++
> >>>>>>  1 file changed, 3 insertions(+)
> >>>>>>
> >>>>>> diff --git a/env/env.c b/env/env.c
> >>>>>> index dcc25c030b..024d36fdbe 100644
> >>>>>> --- a/env/env.c
> >>>>>> +++ b/env/env.c
> >>>>>> @@ -300,6 +300,9 @@ int env_init(void)
> >>>>>>  
> >>>>>>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,
> >>>>>>  		      drv->name, ret);
> >>>>>> +
> >>>>>> +		if (gd->env_valid == ENV_INVALID)
> >>>>>> +			ret = -ENOENT;
> >>>>>>  	}
> >>>>>>  
> >>>>>>  	if (!prio)
> >>>>>
> >>>>> Is the storage driver marking the environment as invalid but not
> >>>>> returning ENOENT valid?
> >>>>
> >>>> Yes, some are doing that.
> >>>
> >>> Why?  Is that correct or incorrect?  It doesn't seem like this is
> >>> something that should be inconsistent from storage driver to storage
> >>> driver and needs fixing.
> >>
> >> The default env driver is doing it, whether it's a workaround or correct
> >> behavior, I really don't know. Maybe Joe does ?
> >>
> >>>>> How does all of this work in the case of multiple configured storage
> >>>>> drivers?
> >>>>
> >>>> If the env is invalid, then we report it as invalid.
> >>>
> >>> Right.  And what change, if any, does your proposed change have in this
> >>> case?  Thanks!
> >>
> >> Before this patch, that check was missing and the result was random,
> >> depending on which env order you had.
> > 
> > So have you changed the behavior of multiple environments then?  Today
> > it's indeed link order based, which is not optimal but used.
> 
> Yes, I believe this patch makes it work as intended.

Which is what?  Since I believe it works as intended today.  If there's
some behavior change / correction here in this case you need to spell it
out in the commit message.  Thanks!
Marek Vasut June 9, 2020, 1:50 a.m. UTC | #8
On 6/9/20 12:57 AM, Tom Rini wrote:
> On Mon, Jun 08, 2020 at 11:45:18PM +0200, Marek Vasut wrote:
>> On 6/6/20 6:24 PM, Tom Rini wrote:
>>> On Sat, Jun 06, 2020 at 04:54:52PM +0200, Marek Vasut wrote:
>>>> On 6/5/20 11:11 PM, Tom Rini wrote:
>>>>> On Fri, Jun 05, 2020 at 10:47:24PM +0200, Marek Vasut wrote:
>>>>>> On 6/5/20 9:07 PM, Tom Rini wrote:
>>>>>>> On Wed, Jun 03, 2020 at 02:01:08AM +0200, Marek Vasut wrote:
>>>>>>>
>>>>>>>> In case the env storage driver marks environment as ENV_INVALID, we must
>>>>>>>> reset the $ret return value to -ENOENT to let the env init code reset the
>>>>>>>> environment to the default one a bit further down.
>>>>>>>>
>>>>>>>> Signed-off-by: Marek Vasut <marex at denx.de>
>>>>>>>> ---
>>>>>>>>  env/env.c | 3 +++
>>>>>>>>  1 file changed, 3 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/env/env.c b/env/env.c
>>>>>>>> index dcc25c030b..024d36fdbe 100644
>>>>>>>> --- a/env/env.c
>>>>>>>> +++ b/env/env.c
>>>>>>>> @@ -300,6 +300,9 @@ int env_init(void)
>>>>>>>>  
>>>>>>>>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,
>>>>>>>>  		      drv->name, ret);
>>>>>>>> +
>>>>>>>> +		if (gd->env_valid == ENV_INVALID)
>>>>>>>> +			ret = -ENOENT;
>>>>>>>>  	}
>>>>>>>>  
>>>>>>>>  	if (!prio)
>>>>>>>
>>>>>>> Is the storage driver marking the environment as invalid but not
>>>>>>> returning ENOENT valid?
>>>>>>
>>>>>> Yes, some are doing that.
>>>>>
>>>>> Why?  Is that correct or incorrect?  It doesn't seem like this is
>>>>> something that should be inconsistent from storage driver to storage
>>>>> driver and needs fixing.
>>>>
>>>> The default env driver is doing it, whether it's a workaround or correct
>>>> behavior, I really don't know. Maybe Joe does ?
>>>>
>>>>>>> How does all of this work in the case of multiple configured storage
>>>>>>> drivers?
>>>>>>
>>>>>> If the env is invalid, then we report it as invalid.
>>>>>
>>>>> Right.  And what change, if any, does your proposed change have in this
>>>>> case?  Thanks!
>>>>
>>>> Before this patch, that check was missing and the result was random,
>>>> depending on which env order you had.
>>>
>>> So have you changed the behavior of multiple environments then?  Today
>>> it's indeed link order based, which is not optimal but used.
>>
>> Yes, I believe this patch makes it work as intended.
> 
> Which is what?  Since I believe it works as intended today.  If there's
> some behavior change / correction here in this case you need to spell it
> out in the commit message.  Thanks!

I think the commit message is quite clear that if the env is ENV_INVALID
, then we need to set the return value to -ENOENT, not random.
Tom Rini June 9, 2020, 3:06 p.m. UTC | #9
On Tue, Jun 09, 2020 at 03:50:09AM +0200, Marek Vasut wrote:
> On 6/9/20 12:57 AM, Tom Rini wrote:
> > On Mon, Jun 08, 2020 at 11:45:18PM +0200, Marek Vasut wrote:
> >> On 6/6/20 6:24 PM, Tom Rini wrote:
> >>> On Sat, Jun 06, 2020 at 04:54:52PM +0200, Marek Vasut wrote:
> >>>> On 6/5/20 11:11 PM, Tom Rini wrote:
> >>>>> On Fri, Jun 05, 2020 at 10:47:24PM +0200, Marek Vasut wrote:
> >>>>>> On 6/5/20 9:07 PM, Tom Rini wrote:
> >>>>>>> On Wed, Jun 03, 2020 at 02:01:08AM +0200, Marek Vasut wrote:
> >>>>>>>
> >>>>>>>> In case the env storage driver marks environment as ENV_INVALID, we must
> >>>>>>>> reset the $ret return value to -ENOENT to let the env init code reset the
> >>>>>>>> environment to the default one a bit further down.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Marek Vasut <marex at denx.de>
> >>>>>>>> ---
> >>>>>>>>  env/env.c | 3 +++
> >>>>>>>>  1 file changed, 3 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/env/env.c b/env/env.c
> >>>>>>>> index dcc25c030b..024d36fdbe 100644
> >>>>>>>> --- a/env/env.c
> >>>>>>>> +++ b/env/env.c
> >>>>>>>> @@ -300,6 +300,9 @@ int env_init(void)
> >>>>>>>>  
> >>>>>>>>  		debug("%s: Environment %s init done (ret=%d)\n", __func__,
> >>>>>>>>  		      drv->name, ret);
> >>>>>>>> +
> >>>>>>>> +		if (gd->env_valid == ENV_INVALID)
> >>>>>>>> +			ret = -ENOENT;
> >>>>>>>>  	}
> >>>>>>>>  
> >>>>>>>>  	if (!prio)
> >>>>>>>
> >>>>>>> Is the storage driver marking the environment as invalid but not
> >>>>>>> returning ENOENT valid?
> >>>>>>
> >>>>>> Yes, some are doing that.
> >>>>>
> >>>>> Why?  Is that correct or incorrect?  It doesn't seem like this is
> >>>>> something that should be inconsistent from storage driver to storage
> >>>>> driver and needs fixing.
> >>>>
> >>>> The default env driver is doing it, whether it's a workaround or correct
> >>>> behavior, I really don't know. Maybe Joe does ?
> >>>>
> >>>>>>> How does all of this work in the case of multiple configured storage
> >>>>>>> drivers?
> >>>>>>
> >>>>>> If the env is invalid, then we report it as invalid.
> >>>>>
> >>>>> Right.  And what change, if any, does your proposed change have in this
> >>>>> case?  Thanks!
> >>>>
> >>>> Before this patch, that check was missing and the result was random,
> >>>> depending on which env order you had.
> >>>
> >>> So have you changed the behavior of multiple environments then?  Today
> >>> it's indeed link order based, which is not optimal but used.
> >>
> >> Yes, I believe this patch makes it work as intended.
> > 
> > Which is what?  Since I believe it works as intended today.  If there's
> > some behavior change / correction here in this case you need to spell it
> > out in the commit message.  Thanks!
> 
> I think the commit message is quite clear that if the env is ENV_INVALID
> , then we need to set the return value to -ENOENT, not random.

If it was clear that you were fixing a problem, or changing the behavior
of how multiple configured environments work we wouldn't be this far in
to an email thread where I'm asking questions about what changed.  Given
the complex use cases of the area we're talking about here, an overly
verbose commit message is far preferred to a terse one.  Thanks!
diff mbox series

Patch

diff --git a/env/env.c b/env/env.c
index dcc25c030b..024d36fdbe 100644
--- a/env/env.c
+++ b/env/env.c
@@ -300,6 +300,9 @@  int env_init(void)
 
 		debug("%s: Environment %s init done (ret=%d)\n", __func__,
 		      drv->name, ret);
+
+		if (gd->env_valid == ENV_INVALID)
+			ret = -ENOENT;
 	}
 
 	if (!prio)