install-to-filesystem: Allow /boot to be missing in target #1996
Conversation
alexlarsson
force-pushed
the
allow-no-boot-dir
branch
from
31dfc04 to
c56215f
Compare
![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 correctly modifies the install_to_filesystem function to handle cases where the /boot directory is missing, preventing an error and instead treating it as not being a separate mount. This is a good improvement for use cases like automotive where /boot may not be a real partition. I've added one suggestion to slightly refactor the code for better efficiency and clarity.
| let boot_is_mount = { | ||
| let root_dev = rootfs_fd.dir_metadata()?.dev(); | ||
| let boot_dev = target_rootfs_fd | ||
| .symlink_metadata_optional(BOOT)? | ||
| .ok_or_else(|| { | ||
| anyhow!("No /{BOOT} directory found in root; this is is currently required") | ||
| })? | ||
| .dev(); | ||
| tracing::debug!("root_dev={root_dev} boot_dev={boot_dev}"); | ||
| root_dev != boot_dev | ||
| if let Some(boot_metadata) = target_rootfs_fd.symlink_metadata_optional(BOOT)? { | ||
| let boot_dev = boot_metadata.dev(); | ||
| tracing::debug!("root_dev={root_dev} boot_dev={boot_dev}"); | ||
| root_dev != boot_dev | ||
| } else { | ||
| tracing::debug!("No /{BOOT} directory found"); | ||
| false | ||
| } | ||
| }; |
There was a problem hiding this comment.
This change is correct, but we can make it slightly more efficient and clear. By moving the root_dev calculation inside the if let block, we avoid computing it if /boot doesn't exist. This also allows removing the outer block, making the code a bit more direct.
let boot_is_mount = if let Some(boot_metadata) = target_rootfs_fd.symlink_metadata_optional(BOOT)? {
let root_dev = rootfs_fd.dir_metadata()?.dev();
let boot_dev = boot_metadata.dev();
tracing::debug!("root_dev={root_dev} boot_dev={boot_dev}");
root_dev != boot_dev
} else {
tracing::debug!("No /{BOOT} directory found");
false
};There was a problem hiding this comment.
Applied this change.
alexlarsson
force-pushed
the
allow-no-boot-dir
branch
from
c56215f to
7594243
Compare
|
@alexlarsson your change makes sense to me. |
When we're currently checking if /boot is a mountpoint we ware failing if /boot is missing. In the automotive usecase, with aboot, neither /boot or /boot/EFI are real partitions, so when bootc-image-builder runs there will be no mounts anywhere under /boot, which means bootc errors out with: ``` No /boot directory found in root; this is is currently required. ``` This isn't necessary, we can just continue on with boot_is_mount as false in this case. Note: Things later fail in bootupd, but that can be fixed separately. Signed-off-by: Alexander Larsson <alexl@redhat.com>
alexlarsson
force-pushed
the
allow-no-boot-dir
branch
from
7594243 to
d2e808d
Compare
3 participants
When we're currently checking if /boot is a mountpoint we ware failing if /boot is missing. In the automotive usecase, with aboot, neither /boot or /boot/EFI are real partitions, so when bootc-immage-builder runs there will be no mounts anywhere under /boot, which means bootc errors out with:
This isn't necessary, we can just continue on with boot_is_mount as false in this case.
Note: Things later fail in bootupd, but that can be fixed separately.