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 <pragyanpoudyal41999@gmail.com>
Signed-off-by: Pragyan Poudyal <pragyanpoudyal41999@gmail.com>
There was a problem hiding this comment.
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.
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