Page MenuHomeFreeBSD

"help" loader command broken
Needs ReviewPublic

Authored by sigsys_gmail.com on Dec 29 2019, 12:37 AM.

Details

Reviewers
kevans
imp
tsoome
Summary

The "help" command from the loader doesn't work anymore. It says "Verbose help not available".

I found two problems behind that but I'm not sure how to fix them. Below is a tentative diff.

One is that the help file just doesn't seem to be installed anymore. On the newer installs that I have, the file is just not there. While on older installs, the file is old (2018). Problem seems to be with how HELP_FILES is handled in stand/loader.mk.

The other problem is that command_help() in stand/common/commands.c doesn't build a valid path for ZFS. It uses loaddev, but it's set to "disk0p2:" when I checked it from the loader prompt (while currdev is set to "zfs:zroot/ROOT/current:" (IIRC) which does produce a valid path (but maybe not always the right one).

Test Plan

Type "help" in the loader interactive prompt.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

kevans added inline comments.Dec 29 2019, 12:42 AM
stand/common/commands.c
138–140

I think loaddev is probably the correct incantation here -- if /boot is on ZFS, I would expect loaddev to be of the zfs format rather than the standard disk/part format. We have some kludgy stuff, as I recall, to set currdev differently if it's ZFS and we probably need to make sure that's DTRT for ZFS.

Just did a test booting in non-UEFI mode and then loaddev is set the same as currdev here. So the second problem seems to be specific to the EFI loader.

Also, I just did a make world to test this patch (even though it's probably not fully correct) and there were no apparent problems apart from loader.help getting installed multiple times (once for all of the loader variants).

Also, I just did a make world to test this patch (even though it's probably not fully correct) and there were no apparent problems apart from loader.help getting installed multiple times (once for all of the loader variants).

@imp/@tsoome: any reason we shouldn't just merge all of the help files and embed the contents in C instead of actually installing the file? i.e. how often do we think users might actually modify loader.help, and is that often enough to warrant building out one of various technical solutions to make sure that loader's loading the correct help contents.

I only ask because we often have platforms with different help files for different loaders within that platform (e.g. efiloader vs. bios loader, the former just has common commands (although we should probably provide help for some of its special commands) and the latter has help.i386), and I don't see any advantage to the current situation. We'd be duplicating compilation for each flavor of loader that we build (simple, 4th, lua), but I think the overhead's fairly minimal.

tsoome added inline comments.Dec 29 2019, 8:10 AM
stand/common/commands.c
138–140

There is something off. First of all, in case of uefi, the initial loaddev is disk with ESP, but set_currdev() is stand/efi/loader/main.c should set it to be the same as currdev. So the help command should have no problem reading help file. (still changing this to currdev should be correct thing because we have no loader scripts in ESP).

However, since in your case the loaddev is not changed, it can only mean you have older version of loader running - you can try to run: chain /boot/loader.efi if it does seem to work ok, copy it to ESP.

(there is also little philosophical question if we really should change loaddev along with currdev).

tsoome accepted this revision.Dec 29 2019, 9:39 AM
This revision is now accepted and ready to land.Dec 29 2019, 9:39 AM

wrt to the help files being installed multiple times, kevans already figured it out, but just to make it clear, the loader has some architecture specific help files but they also don't work.

$ cd /usr/src/stand/i386/loader
$ make -V HELP_FILES
/usr/src/stand/i386/loader/help.i386 /usr/src/stand/common/help.common
$ cd /usr/src/stand/i386/loader_lua
$ make -V HELP_FILES
/usr/src/stand/common/help.common

And it's stand/i386/loader_lua (and others) that get built, stand/i386/loader is shared code and not built directly. So stand/i386/loader/help.i386 never gets used.

So with the current patch, the exact same common help files get installed multiple times and the architecture specific help files don't ever get used. And if they were to get used, they would overwrite each others and it would be worse.

So as it is, the current patch maybe doesn't make anything worse. But it's still a mess.

stand/common/commands.c
138–140

I swear that my installed loader in ESP is up to date. It was being started by an UEFI shell from a STARTUP.NSH file so I tried without that by booting with the loader directly installed as BOOTX64.EFI, but it didn't change anything. loaddev isn't being set to currdev on this computer. It's using GELI too, maybe that's another factor.

imp added inline comments.Dec 30 2019, 3:48 AM
stand/common/commands.c
138–140

Geli? What are you installing into the ESP?

stand/common/commands.c
138–140

Just the loader and a UEFI shell (which I tried without). I didn't mean that the ESP was GELI-encrypted, just that it's loading the kernel from a GELI-encrypted root afterward.

imp added inline comments.Dec 30 2019, 4:18 AM
stand/common/commands.c
138–140

So copying loader.efi to the ESP? As opposed to boot1.efi?

stand/common/commands.c
138–140

Yeah.

I forgot about that but I had made that ESP partition a while after installation. I stole some space from the swap partition for it. This computer started with a 11.X install so that was a while ago.

So yeah I copied /boot/loader.efi (same file as loader_lua.efi) to the ESP. If you're supposed to use boot1.efi then I guess I did something weird. Is it surprising that it works?

Alright I couldn't let this go. Here's my attempt to fix it correctly with as little changes as possible. This installs a different help file for the EFI and non-EFI loaders.

This includes kevans' patch at D23390 because it's needed to make it work here.

This revision now requires review to proceed.Jan 30 2020, 3:40 AM

Forgot to add the new stand/efi/loader/help.efi file. There's no EFI-specific help yet but there could be.