[SLOF] [PATCH 4/4] bootmenu: Wire up the new boot menu in the Forth code

[Alexey Kardashevskiy]

On 07/06/17 02:21, Thomas Huth wrote:
> On 06.06.2017 11:20, Alexey Kardashevskiy wrote:
>> On 02/06/17 01:25, Thomas Huth wrote:
>>> Remove the old Forth-based boot menu code and use the
>>> new libbootmenu module instead.
>>>
>>> Signed-off-by: Thomas Huth <thuth at redhat.com>
>>> ---
>>>  slof/fs/start-up.fs | 71 +++++++----------------------------------------------
>>>  1 file changed, 9 insertions(+), 62 deletions(-)
>>>
>>> diff --git a/slof/fs/start-up.fs b/slof/fs/start-up.fs
>>> index dc5d1ed..7020f5c 100644
>>> --- a/slof/fs/start-up.fs
>>> +++ b/slof/fs/start-up.fs
>>> @@ -65,70 +65,17 @@
>>>  \ Watchdog will be rearmed during load if use-load-watchdog variable is TRUE
>>>  TRUE VALUE use-load-watchdog?
>>>  
>>> -1 value my-boot-dev
>>> -1 value digit-val
>>> -0 value boot-dev-no
>>> -
>>> -: boot-selected
>>> -   1 to my-boot-dev
>>> -   BEGIN parse-word dup WHILE
>>> -      boot-dev-no my-boot-dev = IF
>>> -         s" boot " 2swap $cat
>>> -         ['] evaluate catch ?dup IF     \ and execute it
>>> -            ." boot attempt returned: "
>>> -            abort"-str @ count type cr
>>> -            throw
>>> -         THEN
>>> -         0 0 load-list 2!
>>> -         UNLOOP EXIT
>>> -      ELSE
>>> -         2drop
>>> -      THEN
>>> -      my-boot-dev 1 + to my-boot-dev
>>> -   REPEAT 2drop 0 0 load-list 2!
>>> -
>>> -   (boot)
>>> -;
>>> -
>>> -: boot-start
>>> -   decimal
>>> -   BEGIN parse-word dup WHILE
>>> -      my-boot-dev (u.) s" . " $cat type 2dup type ." : " de-alias type cr
>>> -      my-boot-dev 1 + to my-boot-dev
>>
>> If I read the patchset correctly, what you wanted was to fix this loop to
>> walk through all disk*/cdrom*/net* aliases instead of what contributed to
>> $bootdev....
>>
>>
>>> -   REPEAT 2drop 0 0 load-list 2!
>>> -
>>> -   \ Clear pending keys (to remove multiple F12 key presses for example)
>>> -   BEGIN key? WHILE
>>> -      key drop
>>> -   REPEAT
>>> -
>>> -   cr
>>> -   BEGIN
>>> -      KEY
>>> -      dup 1b = IF         \ ESC sequence ... could be yet another F12 key press
>>> -         BEGIN key? WHILE
>>> -            key drop
>>
>>> -         REPEAT
>>> -      ELSE
>>> -         dup emit
>>> -      THEN
>>> -      dup isdigit IF
>>> -         dup 30 - to digit-val
>>> -         boot-dev-no a * digit-val + to boot-dev-no
>>> -      THEN
>>> -   d = UNTIL
>>
>>
>> ... get rid of this 0xd (which I do not see as a huge improvement as now
>> you are limiting yourself to 0..9a..zA..Z while you could have billions
>> but whatever :) ). Also, there is a tiny chance that some may have test
>> scripts relying on SLOF waiting for \r :)
> 
> Honestly, every time I use the current boot menu after a while, I am
> confused by its current behavior (I never press RETURN in that case
> since I expect that one key press is enough to start an entry).

When would you use it at all? You can control the boot sequence via QEMU
command line, it is easier imho.

> I think
> I hardly have ever seen another boot menu in the past where you have to
> enter a multi-digit number and press RETURN afterwards ...
> And if you've got more than 30 boot devices, they likely do not fit on
> your screen anymore anyway, so using the boot menu does not make much
> sense anymore in that case, too, I think.

Well, then the next bugreport will be "I have 31 device and only 30 show up
in the list" ;) You could at least print a line saying "no more than 23 can
be shown" (80x25 minus one line for boot menu header minus one line for the
user input).

> 
> But if you really think that the current behavior of the boot menu is
> better, please say so, then I'll stop my efforts to improve that thing.

I have not heard of anyone actually using boot menu so since you are the
only user I know, and you are not happy and sent the patches, let it be
your way :)

> 
>> Would not it be easier to clear $bootdev and simply populate it with all
>> discovered aliases? Since we got this far, we do not care about $bootdev
>> anymore?
> 
> First, I'm not sure whether chaning $bootdev is really a good idea here
> now, since the user might exit the boot menu and type "boot" at the SLOF
> prompt again instead.
> 
> Then I wonder why you apparently prefer now to keep the Forth
> implementation of the boot menu?? IMHO the current code is quite hard to
> maintain/understand/extend with all this "evaluate" + "parse-word" magic
> in there. Last time you said "Either forth or c is ok" for an update
> (see https://lists.ozlabs.org/pipermail/slof/2017-April/001511.html),

I remember :) I just looked at the patches and wondered if it would be
easier to stick to Forth. I do not understand parse-word magic though so if
even you struggle here, then moving to C is a good thing.

> so
> I took the chance now to rewrite it in C, but if that's now suddenly not
> OK for you anymore, please let me now, then I'll try to disimprove the
> Forth code instead...

C is still ok. The patches splitting is confusing. I had to do little
debugging to find out where is that "\r" which you got rid of and where you
switched from qemu,boot-list/device to aliases, and commit logs did not
help with that. If the first patch moved forth bits to C (preserving the
old functionality) and the second one switched to aliases and removed "\r"
- that would make things easier to review imho, and also bisectable.


-- 
Alexey