| Message ID | 1477395825-6930-2-git-send-email-thuth@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
Thomas Huth <thuth@redhat.com> writes: > 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@redhat.com> Reviewed-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
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@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; - would handle (very unlikely but still) case of CASE other than 1..5. Or I am missing something here? > + ENDCASE > ; > > \ Environment variables might be board specific >
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@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
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 + ENDCASE ; \ Environment variables might be board specific
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@redhat.com> --- slof/fs/envvar.fs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)