Turn boot.kernelParams into an attribute set #28593
Conversation
This fixes NixOS#28277 and also its subsequent pull request NixOS#28392. The problem with the latter was that it has simply sorted the list of kernelParams within its apply function by converting it into an attrset and back to a list. This had the disadvantage, that it's no longer possible to include store paths in parameter values and it also didn't allow items to be ordered, which breaks passing arguments to init by separating them with "--". In order to address NixOS#28277 properly, we turn boot.kernelParams into an attribute set in addition with a coercedTo type for backwards- compatibility with the list of strings that we had so far. Being an attrset this allows us to deduplicate and sort the kernel parameters in a more meaningful way and gives us an error whenever a value is defined twice as well as an option to use priorities to override existing values. For example if we want to set console=tty0 while already defined in another module, we can simply do so like this: { lib, ... }: { boot.kernelParams.console = lib.mkForce "tty0"; } To support passing arguments to init we now have another option called boot.initArgs, which will be unordered to support custom init systems and also added to the kernel parameters separated by "--". Signed-off-by: aszlig <aszlig@redmoonstudios.org> Cc: @volth, @edolstra Fixes: NixOS#28277
|
@aszlig, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @viric and @puffnfresh to be potential reviewers. |
And if you actually want to have two (or more) console= args? Will that be possible? Ref. kernel documentation: https://www.kernel.org/doc/html/latest/admin-guide/serial-console.html, which shows an example with two console= args where order matters. (The last console= will be used when you open /dev/console.) |
| ''; | ||
| }; | ||
|
|
||
| boot.kernelParamList = mkOption { |
There was a problem hiding this comment.
This option can be avoided by having an apply attribute on boot.kernelParams that converts it to a list. I'm not convinced apply is a good idea in general, though...
There was a problem hiding this comment.
I'm using this option in order to make sure the resulting config attribute has the same type as the input and also to avoid to introduce another function inlib.
| <manvolnum>1</manvolnum> | ||
| </citerefentry> via kernel parameters. | ||
|
|
||
| You could directly use <option>boot.kernelParams</option> to do the |
There was a problem hiding this comment.
This needs <para> if you want the paragraph breaks to show up in the docs.
| default = let | ||
| enabled = filterAttrs (const (v: v != false)) config.boot.kernelParams; | ||
| hasSpace = val: builtins.match ".* .*" val != null; | ||
| mkStrVal = val: if hasSpace val then "\"${val}\"" else val; |
There was a problem hiding this comment.
Why not just quote every value unconditionally?
There was a problem hiding this comment.
Simply because it looks ugly in dmesg.
| Furthermore, passing init arguments via | ||
| <option>boot.kernelParams</option> will cause the arguments to be | ||
| sorted, so it's unsuitable if you use an init that depends on the order | ||
| of arguments. |
There was a problem hiding this comment.
Are there any init arguments that depend on the order? systemd(1) suggests the order shouldn't matter.
There was a problem hiding this comment.
There aren't, but init could also be a shell script and/or basically anything, if you want to pass $1 to such a shell script it will be difficult if the value is sorted.
There was a problem hiding this comment.
Well, in NixOS init is always systemd.
There was a problem hiding this comment.
By default it is, but for example to build for embedded systems, it might make sense to change it. An example of this is in the VirtualBox tests which changes init to avoid running a full system during software virtualization (which already is slow as hell).
|
@bjornfor: You're absolutely right and this might be somewhat of a dead end here. One solution for this would be to also accept lists for the attribute values (and merge multiple definitions accordingly), to provide multiple values but I fear this would make it way too complicated. Example: { boot.kernelParams.console = [ "ttyS1,9600" "tty0" ]; } |
|
So in summary: If #28277 is a real issue that we want to have fixed, I'd pursue this further, but I wouldn't mind closing this PR altogether. |
Note that this pull request is only a suggestion, currently untested and requires discussion, please do not merge it!
This fixes #28277 and also its subsequent pull request #28392.
The problem with #28392 was that it has simply sorted the list of
boot.kernelParamswithin its apply function by converting it into an attrset and back to a list. This had the disadvantage, that it's no longer possible to include store paths in parameter values and it also didn't allow items to be ordered, which breaks passing arguments to init by separating them with--.In order to address #28277 properly, we turn
boot.kernelParamsinto an attribute set in addition with acoercedTotype for backwards-compatibility with the list of strings that we had so far.Being an attrset this allows us to deduplicate and sort the kernel parameters in a more meaningful way and gives us an error whenever a value is defined twice as well as an option to use priorities to override existing values.
For example if we want to set
console=tty0while already defined in another module, we can simply do so like this:To support passing arguments to init we now have another option called
boot.initArgs, which will be unordered to support custom init systems and also added to the kernel parameters separated by--.Cc: @volth, @edolstra
Fixes: #28277