Page MenuHomeFreeBSD

loader: do not force default color environment settings
Changes PlannedPublic

Authored by emaste on Mar 9 2022, 4:48 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 20, 12:17 PM
Unknown Object (File)
Mon, Nov 18, 8:01 AM
Unknown Object (File)
Sat, Nov 2, 6:30 PM
Unknown Object (File)
Oct 4 2024, 1:23 PM
Unknown Object (File)
Oct 4 2024, 11:34 AM
Unknown Object (File)
Sep 20 2024, 5:20 AM
Unknown Object (File)
Sep 18 2024, 2:54 AM
Unknown Object (File)
Sep 17 2024, 5:06 PM
Subscribers

Details

Reviewers
tsoome
imp
manu
Summary

Do not force default values into the environment If the user does not specify teken.fg_color or teken.bg_color, allowing the kernel defaults to take effect.

PR: 261311

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

emaste requested review of this revision.Mar 9 2022, 4:48 PM

This seems like the correct approach to me: if the user does not set teken.fg_color/teken.bg_color then the loader and kernel each use their compiled defaults. If the user sets the environment variables then the loader and kernel both use the set colors.

Do we need similar things in UEFI?
Otherwise looks good.

This revision is now accepted and ready to land.Mar 9 2022, 6:30 PM

This has changes for both efi and vidconsole.

However, it seems this breaks the loader's own immediate color update. Without this change if you do set teken.fg_color=2 at the loader prompt the loader's fg color immediately changes. With this change it has no effect.

Setting teken.fg_color has no effect in the loader that is - it does have the expected effect once the kernel starts.

The whole point of color and font (and mode) setup is to provide consistent console from loader to kernel to user land.

To use kernel color scheme, the loader should start with the values provided for kernel. As simple as that.

Right now the documented method to set the kernel console colors just does not work at all, while teken.fg_color and teken.bg_color are completely undocumented. That needs to be fixed at the very minimum.

That said, I do not see why it is unreasonable to apply a user-provided setting to the loader and kernel, while leaving each at their own default if not set. If the kernel and loader are built with the same default the net effect is identical to the status quo.

Other issues:

  • it is currently not possible to use a different color attribute for kernel text
  • setting white fg on gray (i.e., fg_color=15 and bg_color=7) results in unreadable text as the background is white, not gray

Right now the documented method to set the kernel console colors just does not work at all, while teken.fg_color and teken.bg_color are completely undocumented. That needs to be fixed at the very minimum.

That said, I do not see why it is unreasonable to apply a user-provided setting to the loader and kernel, while leaving each at their own default if not set. If the kernel and loader are built with the same default the net effect is identical to the status quo.

I tend to agree here. Absent a mechanism for loader to inherit/infer/read the kernel's defaults, it's both surprising and unreasonable for loader to override those defaults without any other input (especially given that loader can't tell if the kernel's defaults are user-preferred or just the default). Ideally, we should document these things and offer a way to set them both independently or together to guarantee consistency or willful divergence.

stand/efi/libefi/efi_console.c
1052

You should be able to set these to the empty string, TUNABLE_INT_FETCH should fail as there's nothing for strtoq() to process. The magic for immediate change is in the hook, which you can't set without setting a value (maybe room for future improvement).

stand/efi/libefi/efi_console.c
1052

If there was already a teken.fg_color in loader.conf though we don't want to override it.

stand/efi/libefi/efi_console.c
1052

Oops, that comment was sitting around unsubmitted until just now.

If I understand correctly this would mean that no teken.*_color in loader.conf would translate to being set with an empty string in kenv, and both the loader and kernel would use their compiled-in defaults. Yes, I think this has the right user-facing effect, at the cost of having confusing kenv entries.