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

[Marek Vasut]

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.