[SLOF] [PATCH 1/2] envvar: Set properties in /options during "(set-defaults)"

[Thomas Huth]

On 27.10.2016 03:38, Alexey Kardashevskiy wrote:
> On 25/10/16 22:43, Thomas Huth wrote:
>> The properties in /options are currently only populated from
>> the NVRAM common partition or if the user explicitely sets and
>> environment variable with "setenv". This causes two problems:
>>
>> 1) The properties in /options are not reset when the user runs
>> the "set-defaults" Forth word, e.g. like this:
>>
>>     setenv auto-boot? false
>>     dev /options
>>     printenv auto-boot?
>>     s" auto-boot?" get-node get-property drop type
>>     set-defaults
>>     printenv auto-boot?
>>     s" auto-boot?" get-node get-property drop type
>>
>> After the "set-defaults", the property in /options has the
>> wrong value and is not in sync with the environment variable
>> anymore.
>>
>> 2) If the common NVRAM partition is not containing all the
>> required variables, SLOF currently also does not create
>> default values in /options for the missing entries. This
>> causes problems for example when we want to initialize the
>> NVRAM from QEMU instead (to support the "-prom-env" parameter
>> of QEMU). Boot loaders like grub2 depend on the availability
>> of certain properties in the /options node and thus refuse
>> to work if the NVRAM did not contain all the variables.
>>
>> To fix both issues, let's always populate the /options
>> properties during "(set-default)" already.
>>
>> Signed-off-by: Thomas Huth <thuth at redhat.com>
>> ---
>>  slof/fs/envvar.fs | 16 ++++++++++------
>>  1 file changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/slof/fs/envvar.fs b/slof/fs/envvar.fs
>> index 3364313..4a4d237 100644
>> --- a/slof/fs/envvar.fs
>> +++ b/slof/fs/envvar.fs
>> @@ -186,12 +186,16 @@ DEFER old-emit
>>  
>>  \ set envvar(s) to default value
>>  : (set-default)  ( def-xt -- )
>> -   dup >name name>string $CREATE dup >body c@ >r execute r> CASE
>> -   1 OF env-int ENDOF
>> -   2 OF env-bytes ENDOF
>> -   3 OF env-string ENDOF
>> -   4 OF env-flag ENDOF
>> -   5 OF env-secmode ENDOF ENDCASE
>> +    dup >name name>string 2dup $CREATE
>> +    rot dup >body c@ >r
>> +    execute
>> +    r> CASE
>> +        1 OF dup env-int (.d) 2swap set-option ENDOF
>> +        2 OF 2dup env-bytes 2swap set-option ENDOF
>> +        3 OF 2dup env-string 2swap set-option ENDOF
>> +        4 OF dup env-flag IF s" true" ELSE s" false" THEN 2swap set-option ENDOF
>> +        5 OF dup env-secmode (.d) 2swap set-option ENDOF
> 
> 
> Having default branch to "2drop exit" and having a single "2swap
> set-option" after ENDCASE would
> - make code simpler as you would not have to change cases;

Not sure what you mean with "not have to change cases" here ... I've got
to touch that code anyway since preparing the string is different for
each case, e.g. for case 1 I've added a "(.d)", for case 4 the
s" true" / s" false" etc.

> - would handle (very unlikely but still) case of CASE other than 1..5.

That should never happen™. The environment XTs are only created with
numbers 1 ... 5, so there's no way we can get another number here unless
we've got a memory corruption somewhere. But in case of a memory
corruption we're likely pretty much dead here anyway. Also not sure what
to do in such a case ... ABORT? Silently ignore it?

> Or I am missing something here?

I agree that moving the "2swap set-option" after the ENDCASE is a nice
optimization and I'll prepare a v2 with that. Not sure what to do about
the "default case" ... if you insist, I can add it, but then please also
recommend whether to ABORT, simply print an error message or silently
ignore it.

 Thomas