There's a kernel build option for atkbd to have default keyboard layout specified with ATKBD_DFLT_KEYMAP. This option doesn't affect the bootloader when gptzfsboot/gptboot asks for passphrase. This patch allows users to specify this in /etc/make.conf with BOOT_DEFAULT_KEYMAP.
Details
Details
Handful of keymap layouts were tested.
Pending tests:
- what else does depend on stand/i386/common/cons.c that would cause a problem with the layout?
- how does it affect (if all) BIOSes where users have custom language already specified ?
- does my changes in Makefiles cause problems I'm not aware of?
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
Some general comments...
The style of indented #if isn't what we do in FreeBSD. Code should be indented as if the #if isn't present, and the # directives should be in column 1.
We don't add extra spaces around the ('s, as I've highlighted
We don't do if (foo) bar; on one line.
logical operators are at the end of the line, not the start of the continuation.
It's an interesting notion... You'll likely want to do something similar to the EFI boot loader as well.
Other than the style issues, I think the code isn't bad... A bit worried about requiring an extra binary to build now and I'll need to think about that to make sure that we bootstrap it for the Linux and MacOS builds properly.
| stand/i386/common/cons.c | ||
|---|---|---|
| 117–119 | FreeBSD's style would render these as if (sf & SF_SHIFT)
idx |= 1;
if (sf & SF_CTRL)
idx |= 2;
if (sf & SF_ALT)
idx |= 4;but that likely requires a comment of some sort since it is this weird thing that kbdcontrol does to generate its maps that the reader of this code is likely unfamiliar with. So this removes the extra spaces, and puts things on multiple lines. | |
| 122 | There's an extra space after the first and third ( here. | |
| 124 | ditto extra spaces around ( and ). | |
| 126 | extra space before sc. | |
| 128 | See my general comment about indenting. | |
| stand/i386/common/cons.h | ||
| 43 | The #defines here shouldn't be indented. | |
| stand/i386/gptboot/Makefile | ||
| 41 | Bare paths like this are problematic.... this creates a race between stand and kbdcontrol potentially... | |
| 66 | We generally don't include dependencies like this. Instead, consmap.h should be in the SRCS and cons.c should depend on it, not cons.o. | |
| 77 | This should be done by the build system already. If not, this is likely the wrong place to put it. Also I suspect that you'd need this in more than just gptboot, and so it should go some place common. | |
| 78 | ${.TARGET} here to avoid repeating the target and avoid baking in assumptions about where that file lives. | |
| 79 | likewise. | |
| stand/libsa/Makefile | ||
| 193 ↗ | (On Diff #103629) | This is too wide. CFLAGS += is verboten. You must only add it to the files that need it, |
Comment Actions
Basically, unless we can load keyboad map via HII interface, it will not be possible, we will not get scancodes passed via SimpleTextInput(Ex) protocols.
Other than the style issues, I think the code isn't bad... A bit worried about requiring an extra binary to build now and I'll need to think about that to make sure that we bootstrap it for the Linux and MacOS builds properly.
Comment Actions
As this was my first patch here I need to get familiar with the interface and the way how I can submit the changes. I also need to refresh my memory what I did as I did this in March.
Other than the style issues, I think the code isn't bad... A bit worried about requiring an extra binary to build now and I'll need to think about that to make sure that we bootstrap it for the Linux and MacOS builds properly.
There's no additional binary being produced. Modification is done in getc() to use map similarly to what atkbd does.
Comment Actions
- I've updated the code to follow the style as you mentioned.
- I've removed too wide CFLAGS from libsa Makefile.
I was not able to solve the other Makefile issues you pointed out. This is what I've trouble with right now (my assumptions):
- Currently three binaries require cons.o: gptboot, gptzfsboot and isoboot. There's no direct rule to create cons.o in any Makefile. I'm assuming it's because it gets built by the subdir rule. I thought it would be the best to create a target cons.o that does create the mapfile if needed. As more targets require this header I thought of putting it to stand/common. I didn't find out how I can do that.
- I've included mkdir in consmap.h target. I assumed I can build stand/ directly, i.e on a fresh source copy I can do cd stand && make. The directory doesn't exist, only if I do a proper make buildkernel beforehand from src top (is this my fault of thinking about it this way?)
kbdcontrol is used in e.g. ./sys/conf/files.amd64 where the map if created for kernel option ATKBD_DFLT_KEYMAP. But I'm assuming stand/ is standalone, so it doesn't make sense to touch that config at all (same reason why I used make.conf for BOOT_DEFAULT_KEYMAP).
Side note: I know legacy boot will be used less and less. Most likely not many people require this feature at all.