-
Notifications
You must be signed in to change notification settings - Fork 144
System reinstall composefs #1726
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
If `/boot` is a separate partition, the kernel + initrd paths need to be absolute to `/`, which was not the case until now as they were always absolute to the actual rootfs Signed-off-by: Pragyan Poudyal <[email protected]>
Signed-off-by: Pragyan Poudyal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds a composefs-backend option for system reinstallation and fixes a bug in GRUB bootloader path generation when /boot is a separate partition. The changes are well-implemented across the configuration and command-line layers. The bug fix correctly determines the absolute path prefix, and the refactoring to use Utf8PathBuf::join improves code clarity and safety. I've included one suggestion to improve variable naming for better readability.
| Bootloader::Grub => { | ||
| let root = Dir::open_ambient_dir(&root_path, ambient_authority()) | ||
| .context("Opening root path")?; | ||
|
|
||
| // Grub wants the paths to be absolute against the mounted drive that the kernel + | ||
| // initrd live in | ||
| // | ||
| // If "boot" is a partition, we want the paths to be absolute to "/" | ||
| let entries_path = match root.is_mountpoint("boot")? { | ||
| Some(true) => "/", | ||
| // We can be fairly sure that the kernels we target support `statx` | ||
| Some(false) | None => "/boot", | ||
| }; | ||
|
|
||
| ( | ||
| BLSEntryPath { | ||
| entries_path: root_path.join("boot"), | ||
| config_path: root_path.join("boot"), | ||
| abs_entries_path: entries_path.into(), | ||
| }, | ||
| None, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarity and to avoid confusion, it would be better to rename the local variable entries_path. It currently has the same name as the entries_path field in the BLSEntryPath struct, but they represent different concepts. The local variable is a path prefix for the bootloader configuration, while the struct field is the directory where kernel/initrd files are written. Renaming it to something like abs_path_prefix would make the code easier to understand.
Bootloader::Grub => {
let root = Dir::open_ambient_dir(&root_path, ambient_authority())
.context("Opening root path")?;
// Grub wants the paths to be absolute against the mounted drive that the kernel +
// initrd live in
//
// If "boot" is a partition, we want the paths to be absolute to "/"
let abs_path_prefix = match root.is_mountpoint("boot")? {
Some(true) => "/",
// We can be fairly sure that the kernels we target support `statx`
Some(false) | None => "/boot",
};
(
BLSEntryPath {
entries_path: root_path.join("boot"),
config_path: root_path.join("boot"),
abs_entries_path: abs_path_prefix.into(),
},
None,
)
}
Add a
composefs-backendoption to system-reinstall-bootcFix a bug with installing grub bootloader where we were using paths absolute to rootfs even if
/bootwas a separate partition