Skip to content
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.

Sign up for GitHub

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

Jump to bottom

initoverlayfs: Add initial initoverlayfs support #4721

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

initoverlayfs: Add initial initoverlayfs support #4721

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions rust/src/cliwrap/kernel_install.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@ use cap_std::fs_utf8::Dir as Utf8Dir;
use cap_std_ext::cap_std;
use fn_error_context::context;
use std::os::fd::AsRawFd;
use std::path::Path;
use std::process::Command;

const INITOVERLAY_INSTALL_CMD: &str = "/usr/bin/initoverlayfs-install";

/// Primary entrypoint to running our wrapped `kernel-install` handling.
#[context("rpm-ostree kernel-install wrapper")]
pub(crate) fn main(argv: &[&str]) -> Result<()> {
Expand Down Expand Up @@ -42,7 +45,7 @@ pub(crate) fn main(argv: &[&str]) -> Result<()> {
}
if let Some(k) = new_kernel {
undo_systemctl_wrap()?;
run_dracut(&k)?;
generate_initfs(&k)?;
redo_systemctl_wrap()?;
}
Ok(())
Expand All @@ -64,13 +67,20 @@ fn redo_systemctl_wrap() -> Result<()> {
Ok(())
}

#[context("Running dracut")]
fn run_dracut(kernel_dir: &str) -> Result<()> {
#[context("Generate initfs")]
fn generate_initfs(kernel_dir: &str) -> Result<()> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I should have mentioned this earlier but you have found a pre-existing confusing mess because we have two places that we run dracut, one here and the other in rpmostree-kernel.cxx. I am pretty sure what you really want is to modify rpmostree-kernel.cxx because it's the one that runs when creating a "base image" aka new commit. This code is trying to wrap the dracut command when invoked on the client side (which can be convenient for testing I guess).

let root_fs = Utf8Dir::open_ambient_dir("/", cap_std::ambient_authority())?;
let tmp_dir = tempfile::tempdir()?;
let tmp_initramfs_path = tmp_dir.path().join("initramfs.img");

let cliwrap_dracut = Utf8Path::new(cliwrap::CLIWRAP_DESTDIR).join("dracut");
let mut initfs_tool = String::new();
let initoverlayfs_path = Path::new(INITOVERLAY_INSTALL_CMD);
if initoverlayfs_path.exists() {
initfs_tool = INITOVERLAY_INSTALL_CMD.to_string()
} else {
initfs_tool = "dracut".to_string()
}
Comment on lines +76 to +82
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be simplified to a one liner like this:

Suggested change
let mut initfs_tool = String::new();
let initoverlayfs_path = Path::new(INITOVERLAY_INSTALL_CMD);
if initoverlayfs_path.exists() {
initfs_tool = INITOVERLAY_INSTALL_CMD.to_string()
} else {
initfs_tool = "dracut".to_string()
}
let initfs_tool = Path::new(INITOVERLAY_INSTALL_CMD).exists().then_some(INITOVERLAY_INSTALL_COMMAND).unwrap_or("dracut");

It's definitely more idiomatic to assign the result of a condition; the use of then_some as a combinator is more a taste thing.

In fact an instance of this pattern is right below:

    let dracut_path = cliwrap_dracut
        .exists()
        .then(|| cliwrap_dracut)
        .unwrap_or_else(|| Utf8Path::new("dracut").to_owned());

let cliwrap_dracut = Utf8Path::new(cliwrap::CLIWRAP_DESTDIR).join(initfs_tool);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is right, because we aren't cliwrapping the initoverlay command. So combining with the above, we should just directly assign to dracut_path below.

let dracut_path = cliwrap_dracut
.exists()
.then(|| cliwrap_dracut)
Expand Down
Loading