Menu does not redraw cleanly after selecting a BE (related to the extra output from read-conf)
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 1702 Build 1708: arc lint + arc unit
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| sys/boot/forth/menu.rc | ||
|---|---|---|
| 177 | Change "0 0 2swap" to instead "0 s>d 2swap" Reason: The stack input for >number is a double-wide number (the "d" in "ud1" stands for double, indicating that the two leading "cells", aka items, on the stack are a single double-precision number). By changing from "0 0 2swap" to instead "0 s>d 2swap" it is more clear that we are pushing a zero on the stack in the form of a double (HINT: s>d converts from single to double-precision; taking the stack from "0" to "0 0"). This is important for various architectures. | |
| 178 | Whoops, stack comments here are wrong. I misspoke last night when I said >number takes 4 items on the stack as input. The initial stack element takes two cells (looks like two items) but is a double-precision value. Used as the "accumulator". I would change the stack comments from: to instead the following [accurate] description: Please note that it is standard parlance in my existing code to use var and var' (pronounced "var-prime") to indicate when the result on the stack after execution is NOT a new value, but a possibly modified form of the stack input. In other words, the return caddr is guaranteed to be within u count of the input caddr, making it more appropriate to call it "caddr-prime" as the >number process merely increments caddr in a loop until it hits the first bad character. If all characters in the string were valid, then the return caddr will be caddr+u and u' (pronounced "u-prime") will be zero. Note however, that u' may also be zero for a NULL string input. The same thing happens with the initial "u" input, it is decremented with each step of the internal >number loop until it reaches zero, and thus is a modified "u" or "u-prime" (standard mathematics terminology). | |
Comment Actions
Already approve, but would like to see the summary text changed to the desired commit message prior to commit. A quick blurb in the other fields would be nice too (e.g., test).
| sys/boot/forth/menu.rc | ||
|---|---|---|
| 161 | you have small issue there; reading in default and local loader.conf will also create new module list and therefore leak current list. I did solve this by creating simple "free-module-options" word, walking through the list and freeing all existing modules. | |
| sys/boot/forth/menu.rc | ||
|---|---|---|
| 188 | goto_menu emits true to stack, so line 185 will result in ( -- N TRUE TRUE ) and thats bug. | |
| sys/boot/forth/menu.rc | ||
|---|---|---|
| 187 | here you have ascii value for number N in stack (51 for '3'), so instead of hardwired 3, you can use instead: dup 48 - goto_menu this would remove the need for hardwired numeric constant. | |
| sys/boot/forth/menu.rc | ||
|---|---|---|
| 161 | Can you please submit to either allanjude or myself the patch to add free-module-options word? One of us will then update the review patch. | |
| 187 | The N on the stack is ASCII decimal for selected menu item. The "3" that you see being passed to goto_menu must be the "3" in "menuset_name3=bootenv". If I changed this to use "dup 48 -" instead of "3", then it would always load the menu associated with "menuset_nameX" where X is the menu item chosen. Imagine the implications if the set_be_page word were used as the action for menu item 4. The goto_menu word would try to load the menu associated with menuset_name4. There is no such named-menuset. The existing hard-wired 3 is correct because we want to specifically (and always) load menuset_name3 which is the bootenv menu. | |
| 188 | Good catch. I'll fix this when I update to include your free-module-options word | |
Comment Actions
If we can get ahold of the free-module-options by tsoome then I think that actually makes this approach very nice (no edge-cases)
| sys/boot/forth/menu.rc | ||
|---|---|---|
| 187 | oh right, I misunderstood the background there. thanks for explaining:) | |
Comment Actions
I have sent by separate mail.
However, There is still one edge case you probably want to think about - if or how much it will apply; the case is about non-zfs boot where you have no BE's. To solve this, I used guardian construct around menuset creation like this:
\ Boot Environments are (supported) only on ZFS
s" currdev" getenv drop 4 s" zfs:" compare 0= [if]
menuset setup code
[then]
The issue is that menu code is generic and therefore will affect scenarios like ufs based installs and cd/usb boot media, where you do not have BE, and loader does not have support commands, so the BE menuset is unusable and will confuse the operator.
Note such guardian is working well in case of illumos where whole BE support is entirely implemented in forth, as we have BE index file in zfs:poolname:/boot/menu.lst maintained by OS beadm command, and to maintain consistent view of BE list, I actually don't even want to read dataset list directly from zfs.
The freebsd case should be covered by same/similar guardian as well, but perhaps I'm missing some context there (currdev versus vfs for example).
| sys/boot/i386/loader/main.c | ||
|---|---|---|
| 390 | Guard with #ifdef LOADER_ZFS_SUPPORT? | |
| sys/boot/zfs/zfs.c | ||
| 708–769 | Avoid assignment and just: return (zfs_list_dataset(spa, objid)); | |
| 723 | check the return codes? more below too. | |
| 783 | style(9) braces around return. | |
| 802 | move up to return early. | |
| 818 | style(9) more below too. if (rv != 0)
return (rv); | |
| 819 | Could the early returns here leave it in an inconsistent state that may course an issue? | |
| 822 | use sizeof(envname), more below. | |
| 835 | use sizeof(enval) | |
| 842 | As zfs_env_index is always incremented could be extracted above so we have the following instead? zfs_env_index++;
if (zfs_env_index > ZFS_BE_LAST)
break; | |
| 850 | use sizeof(envname) more below | |
| 860 | rv here is likely to be useless due to use by loop above, should that use a seperate var or just (void)unsetenv(envname) ? | |
| sys/boot/zfs/zfsimpl.c | ||
| 1600 | Not new but due to the fact the if does a return, there's no need for the else here. | |
| 1903 | No need for else here. | |
| sys/boot/zfs/zfsimpl.c | ||
|---|---|---|
| 1878 | Assign to err value and return what you go, so we don't loose info about the error + style(9) bool test fix e.g. err = objset_get_dnode(spa, &spa->spa_mos, objnum, &dataset);
if (err != 0) {
printf("ZFS: can't find dataset %ju\n", (uintmax_t)objnum);
return (err);
}more below too and the same with dnode_read | |
| sys/boot/forth/menu.rc | ||
|---|---|---|
| 134 | There is actually another small issue, which has simple solution and a bit deeper implications... The problem is, zfs dataset names can be up to 256 symbols long (more or less), and with long names, the menu box itself will get corrupted; now returning to main menu, only box for simple workaround, in illumos code I did create the following word:
: be_draw_screen clear \ Clear the screen (in screen.4th) print_version \ print version string (bottom-right; see version.4th) draw-beastie \ Draw FreeBSD logo at right (in beastie.4th) draw-brand \ Draw brand.4th logo at top (in brand.4th) menu-init \ Initialize menu and draw bounding box (in menu.4th) menu-redraw \ Redraw menu (in menu.4th) ; and to replace this simple "1 goto_menu" with: : be_back_to_main 1 goto_menu be_draw_screen \ return to main menu and then redraw the screen. ; so at least the main screen will be cleaned up. In addition, Jeff did propose additional idea, unfortunately it would require more development and is out of scope of this change, but is something to think about: as for different menus there are menusets, the menuset concept could be extended not to involve just menu entries, but "box" itself as well - to define wider and perhaps with different offset box for submenu. for example, for content like BE names, we know we wont fit in width, so we could define also wider box for this set, overlay it on main view, and if needed, apply additional method to "cut" excessive information. overlay covering (partially) logos and such is okay, as going back to main menu would redraw the screen. | |
| sys/boot/forth/menu-commands.4th | ||
|---|---|---|
| 404 ↗ | (On Diff #11366) | Previously pointed out by tsoome, and I agree, we need to kill this line (TRUE) because it leaks an extra TRUE onto the stack (goto_menu already throws a TRUE on the stack) |
| sys/boot/forth/menu.rc | ||
|---|---|---|
| 134 | I've been holding back a redesign that would address this. I was having issues finding sympathetic parties, such as yourself, that want to solve this issue like myself. Most commonly, say at conferences, I was getting push-back from other developers that were loath to see yet another redesign (not appreciating the technical merit behind such a redesign; specifically to address the stated issue of menu overflow leading to screen garbage). I got support from another interested party just moments ago (Shawn Webb; aka lattera) and am going to move forward with the redesign that I have envisaged over a year ago to address this issue. I'll post mock-ups in the next 48 hours. | |
Comment Actions
Updated with fixes from tsoome
Includes simplified menu layout a bit more like IllumOS
forth stuff:
free-module-options
be_draw_screen
be_back_to_main
remove excess 'true' in set_be_page
| sys/boot/forth/support.4th | ||
|---|---|---|
| 1049 ↗ | (On Diff #11813) | actually the comment on this line was correct one - this strdup does introduce memory leak, and it actually is unneeded, file name produced by this word will be consumed by fopen and therefore strget is just enough to produce addr len pair into the stack. |
| sys/boot/forth/support.4th | ||
|---|---|---|
| 1049 ↗ | (On Diff #11813) | Sorry for the confusion. This is an unrelated change that Phabricator is mis-identifying. I should have done a separate update to Phabricator when I rebased my tree on the latest -current This change is part of rS292899 and unrelated to the ZFS BE menu work. |
Comment Actions
Guard against currdev not being set (rarely happens, but lets not make the menu puke if it does)
| sys/boot/forth/menu-commands.4th | ||
|---|---|---|
| 393 ↗ | (On Diff #11825) | The comment at the end of this line appears out of place and I would remove it to make this menu item action function more like the others in this file. |
| sys/boot/forth/menu.rc | ||
| 134 | I'm pushing the redesign out to the future (pre-11-R) where we can phab it up in an unrelated change. With respect to the menu redraw, I worked with Allan to fine-tune the be_draw_menu function. We removed menu-redraw from be_draw_menu because using be_draw_menu in conjunction with goto_menu would cause a double-redraw of the menu, wherein first it would erase the current contents, redraw the current contents, then erase the entire screen, redraw the screen, and finally draw the desired menu. By moving menu-redraw out of be_draw_menu (and subsequently adding menu-redraw to set_bootenv) we avoid the double-redraw with stale menu data. It's worth noting that at 9600 baud on a serial line, this double-redraw is "in your face" since it takes ~4.95s to draw the entire screen at said baudrate. | |
| sys/boot/forth/support.4th | ||
| 1049 ↗ | (On Diff #11825) | You're right. I fixed the actual leak in SVN r292999 |