Add nullboot.conf support with a kernel-priority field - #166
julian-klode wants to merge 2 commits into
Conversation
|
Maybe we need to have
and
|
bd942ae to
1f8e8fb
Compare
julian-klode
changed the title
Implement nullboot.conf and nullboot.conf.d - yaml files. At this point parse only kernel-priority (similar to GRUB_FLAVOUR_ORDER in grub). Assisted-by: LLM
Wrap version.Version in a WeightedVersion that carries a priority computed from the kernel-priority configuration. Kernels with higher priority sort first; equal priorities fall back to version order. Assisted-by: LLM
1f8e8fb to
82ad415
Compare
|
We now have with unspecified suffixes defaulting to 0. I think this is a bit more cleaner, and the dict merges naturally. This is overkill, a single preference would also get the job done |
| for priority, res := range c.priorities { | ||
| for _, re := range res { | ||
| if re.MatchString(abi) { | ||
| return priority |
There was a problem hiding this comment.
With {fde: 100, azure-fde: 1000}, map order changes the weight for the same kernel (azure-fde matches both entries). This can prefer an older version.
| log.Println("using kernel priorities:", config.KernelPriority) | ||
| } | ||
|
|
||
| km, err := efibootmgr.NewKernelManager(esp, kernelSourceDir, vendor, maybeBm, config) |
There was a problem hiding this comment.
While running generic, prefer FIPS and run nullbootctl. Restore generic before reboot and run it again. BootNext will still select FIPS. Maybe that is a weird edge case but I think it's definitely unintended behavior.
This is not directly caused by your changes, but they make that possible.
There was a problem hiding this comment.
🟡 Changes recommended
Resolve the priority matching and exported API compatibility issues before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds YAML-based /etc/nullboot.conf support for prioritizing kernel flavours during boot selection.
Changes:
- Adds configuration and drop-in parsing.
- Introduces priority-aware kernel ordering.
- Updates CLI integration, dependencies, and tests.
File summaries
| File | Summary |
|---|---|
go.mod |
Promotes the YAML dependency for direct use. |
efibootmgr/reseal_test.go |
Updates kernel manager construction in tests. |
efibootmgr/kernel.go |
Adds priority-aware ordering; requires API compatibility fixes and updated ordering documentation. |
efibootmgr/kernel_test.go |
Updates and tests priority-based ordering. |
efibootmgr/config.go |
Parses priorities and drop-ins; matching must select the highest configured priority deterministically. |
efibootmgr/config_test.go |
Tests configuration parsing and matching. |
cmd/nullbootctl/main.go |
Loads and passes the nullboot configuration. |
Review details
Suppressed comments (3)
efibootmgr/kernel.go:139
- This changes the ordering used by
GetLatestKernelEntryfrom version-only to priority-then-version, but that exported method's existing doc still says it returns the entry with the "largest version" (kernel.go:329). Update the method contract to describe the priority-aware ordering so callers do not rely on the old behavior.
// Sort descending by flavour preference group, then by version
efibootmgr/kernel.go:91
- Adding a required
configparameter to these exported constructors is a source-incompatible API change: downstream users of theefibootmgrlibrary that do not need priorities will no longer compile. Preserve the existingNewKernelManager/NewKernelsignatures and add config-aware variants (or another optional-configuration API) so this feature does not break existing callers.
// NewKernelManager returns a new kernel manager managing kernels in the
// host system. config may be nil.
func NewKernelManager(esp, sourceDir, vendor string, bootManager *BootManager, config *Configuration) (*KernelManager, error) {
efibootmgr/kernel.go:43
Kernel.Versionis an exported field that previously had typeversion.Version; changing it toWeightedVersionis another source-incompatible API change, even for callers that do not use priorities. Keep the public field type stable and store the preference weight separately for internal ordering, or provide an explicit compatibility layer.
type Kernel struct {
Version WeightedVersion
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for priority, res := range c.priorities { | ||
| for _, re := range res { | ||
| if re.MatchString(abi) { | ||
| return priority | ||
| } | ||
| } | ||
| } | ||
| return 0 |
Field is using the same spelling as the grub one.
This allows preferring certain kernel flavours over others.
Not sure if the drop-ins appending flavour order is sensible, probably they should just override instead as it is a bit confusing coming from other configurations.