Get kargs from usr/lib/bootc/kargs.d #1783
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces two main changes. First, it adds support for reading kernel arguments from .toml files in /usr/lib/bootc/kargs.d during installation and upgrades. This is a valuable feature for customizing kernel command line arguments. Second, it refactors the storage handling by introducing boot_dir and esp fields in the Storage struct. This significantly cleans up the code by centralizing the logic for accessing the boot directory and the ESP, removing redundant ESP mounting logic across multiple files. The changes are well-implemented and improve both functionality and code maintainability. I have one minor suggestion to improve an error message for better debuggability.
| Cmdline::from(options) | ||
| } | ||
|
|
||
| _ => anyhow::bail!("Found NonEFI config"), |
There was a problem hiding this comment.
The error message "Found NonEFI config" is confusing. This branch is executed when a config type other than NonEFI is found (e.g., EFI or Unknown). A more accurate message would be "Unsupported BLS config type, expected NonEFI", which would make debugging easier.
| _ => anyhow::bail!("Found NonEFI config"), | |
| _ => anyhow::bail!("Unsupported BLS config type, expected NonEFI"), |
There was a problem hiding this comment.
We change the key efi to uki here #1780
This would need to change anyway afterwards
|
The big assumption here is that all composefs systems will boot via UEFI. That being said, we were already making this assumption in a few other places |
Johan-Liebert1
force-pushed
the
bootc-kargs.d
branch
from
3d66308 to
1f75c8d
Compare
|
I think I overwrote some of my changes on force pushing. Marking this as draft for now |
| /// Directory holding the physical root | ||
| pub physical_root: Dir, | ||
|
|
||
| /// The 'boot' directory, useful and `Some` only for composefs systems |
There was a problem hiding this comment.
It's more that ostree::Sysroot handles this itself right now right?
And that kind of gets to the next thing. The way I'd been thinking about struct Storage is closer to an abstraction over ostree::Repo and composefs::Repository.
Maybe arguably it'd be slightly cleaner to actually have a struct SystemRoot that has these new members, and has an impl Deref<Target=Storage>?
But eh.
There was a problem hiding this comment.
The way I'd been thinking about struct Storage is closer to an abstraction over ostree::Repo and composefs::Repository
We do have physical_root and run in struct Storage so not quite an abstraction over Repository...
Maybe we could put these in BootedComposefs, but that doesn't sound right either
crates/lib/src/bootc_kargs.rs
Outdated
|
|
||
| let fname_str = fname | ||
| .to_str() | ||
| .ok_or_else(|| anyhow::anyhow!("Failed to get filename as string"))?; |
There was a problem hiding this comment.
Perhaps arguably we should skip in this case?
I think we want a helper fn for this that includes the path in the error message if we do propagate an error.
(Non-UTF8 filenames is just a giant mess)
There was a problem hiding this comment.
Perhaps arguably we should skip in this case?
I think we can skip it, and maybe have in the docs that we only support UTF8 filenames
There was a problem hiding this comment.
Skipping any non UTF-8 named files with a warning
Parse toml files in usr/lib/bootc/kargs.d and append them to kernel cmdline on install and upgrade/switch. Also, copy over current deployment's cmdline args on upgrade/switch to another deployment Signed-off-by: Pragyan Poudyal <[email protected]>
We have a lot of places where we mount the ESP temporarily and a lot of switch cases for Grub's vs SystemdBoot's 'boot' directory. We add a `boot_dir` field in Storage which points to `/sysroot/boot` for systems with Grub as the bootloader and points to the ESP for systems with SystemdBoot as the bootloader. Also we mount the ESP temporarily while creating the storage struct, which cleans up the code quite a bit. Signed-off-by: Pragyan Poudyal <[email protected]>
Signed-off-by: Pragyan Poudyal <[email protected]>
Johan-Liebert1
force-pushed
the
bootc-kargs.d
branch
from
1f75c8d to
20eb4e4
Compare
2 participants
Parse toml files in usr/lib/bootc/kargs.d and append them to kernel
cmdline on install and upgrade/switch. Also, copy over current
deployment's cmdline args on upgrade/switch to another deployment
storage: Add
boot_dirandespfieldsWe have a lot of places where we mount the ESP temporarily and a lot of
switch cases for Grub's vs SystemdBoot's 'boot' directory.
We add a
boot_dirfield in Storage which points to/sysroot/bootforsystems with Grub as the bootloader and points to the ESP for systems
with SystemdBoot as the bootloader.
Also we mount the ESP temporarily while creating the storage struct,
which cleans up the code quite a bit.