Move composefs setup root to bootc initramfs#1550
Conversation
There was a problem hiding this comment.
Code Review
This pull request is a good refactoring that moves the composefs-setup-root logic into the bootc-initramfs crate, centralizing the initramfs-related code. The introduction of clap for command-line parsing is a definite improvement. My review focuses on correctness and maintainability. I've identified a critical memory safety issue, a high-severity bug where an error is silently ignored, and a few medium-severity issues to improve code clarity and fix an unused argument.
| #[arg(long, help = "Mountpoint (don't replace sysroot, for testing)")] | ||
| /// Mountpoint (don't replace sysroot, for testing) | ||
| pub target: Option<PathBuf>, |
There was a problem hiding this comment.
The command-line argument target is defined here but it appears to be unused in the setup_root function. The logic in setup_root always uses args.sysroot as the mount destination. If target is intended for testing purposes to provide an alternative mount point, it should be utilized in the mounting logic.
| fn ensure_dir(dirfd: impl AsFd, name: &str) -> rustix::io::Result<OwnedFd> { | ||
| match mkdirat(dirfd.as_fd(), name, 0o700.into()) { | ||
| Ok(()) | Err(Errno::EXIST) => {} | ||
| Err(err) => Err(err)?, |
There was a problem hiding this comment.
The use of Err(err)? in the match arm is a bit unconventional and can be confusing. It constructs a Result::Err variant only to immediately use the ? operator on it. A more direct and clearer way to propagate the error is to use a return statement.
| Err(err) => Err(err)?, | |
| Err(err) => return Err(err), |
| let mount_type = match config.mount { | ||
| Some(mt) => mt, | ||
| None => match config.transient { | ||
| true => MountType::Transient, | ||
| false => default, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
The nested match statement to determine mount_type can be simplified for better readability. Using Option::unwrap_or with an if expression can make the logic more concise and easier to follow.
let mount_type = config.mount.unwrap_or(if config.transient {
MountType::Transient
} else {
default
});|
|
||
| match mount_at(&sysroot_clone, &new_root, "sysroot") { | ||
| Ok(()) | Err(Errno::NOENT) => {} | ||
| Err(err) => Err(err)?, |
There was a problem hiding this comment.
The use of Err(err)? in this match arm is a bit unconventional. While correct, it can be made more explicit and readable. Using return Err(err.into()) would clearly show the error propagation and the conversion to an anyhow::Error.
Err(err) => return Err(err.into()),There was a problem hiding this comment.
BTW with stuff like this I personally am fine one marks it resolved without changing anything
|
|
||
| let cmdline = match &args.cmdline { | ||
| Some(cmdline) => cmdline, | ||
| None => &std::fs::read_to_string("/proc/cmdline")?, |
There was a problem hiding this comment.
Not a new problem per se but this raises the now-more-obvious issue that we added a kargs parser into bootc, and not into composefs-boot...
Except though of course the "detect composefs" karg parsing is only on the composefs branch...
Let's just add a // TODO: Deduplicate this with composefs branch karg parser?
There was a problem hiding this comment.
I thought about using the new parser, but it's in bootc-lib and including the entire create for just one function didn't make much sense. Maybe we should move the cmdline parser to bootc/utils?
There was a problem hiding this comment.
For now, I've just added the TODO comment
There was a problem hiding this comment.
Yeah +1 to moving the cmdline parser to bootc-utils, I'll go do that now
There was a problem hiding this comment.
Feels slightly large for utils? Some of the other crates we're adding are pretty small so a bootc-kargs crate seems nicer to me?
Or, slicing this differently...a general thing that's weird now is that lib contains most of the code.
But we could redo things so that there's e.g.:
- utils
- lib (general grab-bag of stuff like kargs, but not quite "utils")
- core (what is currently
lib) - cli
There was a problem hiding this comment.
👍 I'll ask claude nicely to redo it into its own crate. I'll probably call it bootc-kernel-cmdline though to disambiguate from the stuff in crates/lib/src/bootc_kargs.rs.
d802e03 to
2e4155c
Compare
Move the composefs-setup-root code from composefs-rs to bootc-initramfs crate Signed-off-by: Johan-Liebert1 <pragyanpoudyal41999@gmail.com>
2e4155c to
0ef1eca
Compare
…amfs Move composefs setup root to bootc initramfs
Move the composefs-setup-root code from composefs-rs to bootc-initramfs crate
Left
/etcmount as overlayfs as bind-mounting requires copying files over to state /etc dir which I think is best done in https://github.com/bootc-dev/bootc/pull/1444/files#diff-0b4a5898665a82dc611bd0dd06f4fd3ac8c188e2439e1e2418a02c2d68ae7fe9R2139