| Message ID | 1496330742-18181-5-git-send-email-thuth@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
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@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 :) 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? > - > - boot-dev-no my-boot-dev < IF > - s" boot-selected " s" $bootdev" evaluate $cat strdup evaluate > - ELSE > - ." Invalid choice!" cr > - THEN > - hex > -; > > : boot-menu-start > - ." Select boot device:" cr cr > - s" boot-start " s" $bootdev" evaluate $cat strdup evaluate > + boot-menu ?dup IF > + s" boot " 2swap $cat > + ['] evaluate catch ?dup IF > + ." boot attempt returned: " > + abort"-str @ count type cr > + throw > + THEN > + 0 0 load-list 2! > + THEN > ; > > : boot-menu-enabled? ( -- true|false ) >
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@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). 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. 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. > 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), 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... Thomas
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@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.
On 07.06.2017 05:45, Alexey Kardashevskiy wrote: > On 07/06/17 02:21, Thomas Huth wrote: [...] >> >> 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. Sure, but apparently there are at least some people who try to use the boot menu ... otherwise I would not get those Bugzilla tickets where they complain about the crappy behavior ;-) >> 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). I promise that I'll close that bug report as WONTFIX ... if you try to use a boot menu for handling more than 30 devices, you're doing something wrong anyway. >> 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. It's a complete rewrite, so first trying to implement the old behavior in C and then rewrite most of the code again to implement the new behavior does not make much sense either, I think. So I'll squash the current patches together, let's see how that looks like... Thomas
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 - 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 - - boot-dev-no my-boot-dev < IF - s" boot-selected " s" $bootdev" evaluate $cat strdup evaluate - ELSE - ." Invalid choice!" cr - THEN - hex -; : boot-menu-start - ." Select boot device:" cr cr - s" boot-start " s" $bootdev" evaluate $cat strdup evaluate + boot-menu ?dup IF + s" boot " 2swap $cat + ['] evaluate catch ?dup IF + ." boot attempt returned: " + abort"-str @ count type cr + throw + THEN + 0 0 load-list 2! + THEN ; : boot-menu-enabled? ( -- true|false )
Remove the old Forth-based boot menu code and use the new libbootmenu module instead. Signed-off-by: Thomas Huth <thuth@redhat.com> --- slof/fs/start-up.fs | 71 +++++++---------------------------------------------- 1 file changed, 9 insertions(+), 62 deletions(-)