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.

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:
( ud1 caddr1 caddr2 u2 -- ud2 caddr1 caddr2 u2 )

to instead the following [accurate] description:
( ud caddr/u -- ud' caddr'/u' )

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).

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.

goto_menu emits true to stack, so line 185 will result in ( -- N TRUE TRUE ) and thats bug.

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.

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.

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.

Good catch. I'll fix this when I update to include your free-module-options word

Can we move this to menu-commands.4th please?

Can we move this to menu-commands.4th please?

Can we move this to menu-commands.4th please?

oh right, I misunderstood the background there. thanks for explaining:)

Guard with #ifdef LOADER_ZFS_SUPPORT?

Avoid assignment and just:

return (zfs_list_dataset(spa, objid));

check the return codes? more below too.

style(9) braces around return.

move up to return early.

style(9) more below too.

if (rv != 0)
        return (rv);

Could the early returns here leave it in an inconsistent state that may course an issue?

use sizeof(envname), more below.

use sizeof(enval)

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;

use sizeof(envname) more below

rv here is likely to be useless due to use by loop above, should that use a seperate var or just (void)unsetenv(envname) ?

Not new but due to the fact the if does a return, there's no need for the else here.

No need for else here.

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

It is already guarded, the ifdef starts above the previous function (lszfs) and ends just below 'command_reloadbe'

Possibly, switching them to break

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
contents will get redrawn and the garbage will remain.

for simple workaround, in illumos code I did create the following word:

1. moved *screen* redraw to separate 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.

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)

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.

I have pulled in free-module-options from tsoome (thanks again). Minor changes to it since IllumOS extended the modules structure

both of these are fixed

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.

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.

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.

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.

You're right. I fixed the actual leak in SVN r292999