From e635ad723bafdd646ebf16747c333dec5db5ede7 Mon Sep 17 00:00:00 2001 From: John Eckersberg Date: Thu, 2 Oct 2025 16:48:48 -0400 Subject: [PATCH 1/3] system-reinstall-bootc: Do not warn on unmounted LVM volumes If the system has a swap partition (or any other volume which is not currently mounted) the `findmnt` command will (expectedly) fail to find it. Don't early exit in this case, instead just ignore that volume. If it wasn't mounted in the first place, we don't need to warn about it being unmounted after the reinstall operation is complete. Signed-off-by: John Eckersberg Closes: #1659 --- crates/system-reinstall-bootc/src/lvm.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/system-reinstall-bootc/src/lvm.rs b/crates/system-reinstall-bootc/src/lvm.rs index facf9e289..78b6db545 100644 --- a/crates/system-reinstall-bootc/src/lvm.rs +++ b/crates/system-reinstall-bootc/src/lvm.rs @@ -2,7 +2,7 @@ use std::process::Command; use anyhow::Result; use bootc_mount::run_findmnt; -use bootc_utils::CommandRunExt; +use bootc_utils::{CommandRunExt, ResultExt}; use serde::Deserialize; #[derive(Debug, Deserialize)] @@ -54,7 +54,7 @@ pub(crate) fn check_root_siblings() -> Result> { let siblings: Vec = all_volumes .iter() .filter(|lv| { - let mount = run_findmnt(&["-S", &lv.lv_path], None).unwrap_or_default(); + let mount = run_findmnt(&["-S", &lv.lv_path], None).log_err_default(); if let Some(fs) = mount.filesystems.first() { &fs.target == "/" } else { @@ -63,14 +63,14 @@ pub(crate) fn check_root_siblings() -> Result> { }) .flat_map(|root_lv| parse_volumes(Some(root_lv.vg_name.as_str())).unwrap_or_default()) .try_fold(Vec::new(), |mut acc, r| -> anyhow::Result<_> { - let mount = run_findmnt(&["-S", &r.lv_path], None)?; + let mount = run_findmnt(&["-S", &r.lv_path], None).log_err_default(); let mount_path = if let Some(fs) = mount.filesystems.first() { &fs.target } else { "" }; - if mount_path != "/" { + if mount_path != "/" && !mount_path.is_empty() { acc.push(format!( "Type: LVM, Mount Point: {}, LV: {}, VG: {}, Size: {}", mount_path, r.lv_name, r.vg_name, r.lv_size From 36328ebe21c243f356fcfb672ed5e9d47c18a25f Mon Sep 17 00:00:00 2001 From: John Eckersberg Date: Mon, 6 Oct 2025 12:43:53 -0400 Subject: [PATCH 2/3] system-reinstall-bootc: Add context annotations to Result-returning functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add #[context()] attribute macro to all functions that return Result to improve error reporting. This includes adding the fn-error-context dependency and importing the context macro in all relevant modules. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Signed-off-by: John Eckersberg --- Cargo.lock | 1 + crates/system-reinstall-bootc/Cargo.toml | 1 + crates/system-reinstall-bootc/src/btrfs.rs | 2 ++ crates/system-reinstall-bootc/src/config/mod.rs | 2 ++ crates/system-reinstall-bootc/src/lvm.rs | 3 +++ crates/system-reinstall-bootc/src/main.rs | 2 ++ crates/system-reinstall-bootc/src/podman.rs | 5 +++++ crates/system-reinstall-bootc/src/prompt.rs | 8 ++++++++ crates/system-reinstall-bootc/src/users.rs | 9 +++++++++ 9 files changed, 33 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 776368c14..f9f395ea3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2574,6 +2574,7 @@ dependencies = [ "clap", "crossterm 0.29.0", "dialoguer", + "fn-error-context", "indoc", "log", "openssh-keys", diff --git a/crates/system-reinstall-bootc/Cargo.toml b/crates/system-reinstall-bootc/Cargo.toml index bb4ee7483..d25b24f45 100644 --- a/crates/system-reinstall-bootc/Cargo.toml +++ b/crates/system-reinstall-bootc/Cargo.toml @@ -22,6 +22,7 @@ bootc-utils = { package = "bootc-internal-utils", path = "../utils", version = " anstream = { workspace = true } anyhow = { workspace = true } clap = { workspace = true, features = ["derive"] } +fn-error-context = { workspace = true } indoc = { workspace = true } log = { workspace = true } rustix = { workspace = true } diff --git a/crates/system-reinstall-bootc/src/btrfs.rs b/crates/system-reinstall-bootc/src/btrfs.rs index 57e08bb18..31df04d61 100644 --- a/crates/system-reinstall-bootc/src/btrfs.rs +++ b/crates/system-reinstall-bootc/src/btrfs.rs @@ -1,6 +1,8 @@ use anyhow::Result; use bootc_mount::Filesystem; +use fn_error_context::context; +#[context("check_root_siblings")] pub(crate) fn check_root_siblings() -> Result> { let mounts = bootc_mount::run_findmnt(&[], None)?; let problem_filesystems: Vec = mounts diff --git a/crates/system-reinstall-bootc/src/config/mod.rs b/crates/system-reinstall-bootc/src/config/mod.rs index e6dedf65b..738fde231 100644 --- a/crates/system-reinstall-bootc/src/config/mod.rs +++ b/crates/system-reinstall-bootc/src/config/mod.rs @@ -2,6 +2,7 @@ use std::{fs::File, io::BufReader}; use anyhow::{Context, Result}; use bootc_utils::PathQuotedDisplay; +use fn_error_context::context; use serde::{Deserialize, Serialize}; mod cli; @@ -17,6 +18,7 @@ pub(crate) struct ReinstallConfig { } impl ReinstallConfig { + #[context("load")] pub fn load() -> Result> { let Some(config) = std::env::var_os(CONFIG_VAR) else { return Ok(None); diff --git a/crates/system-reinstall-bootc/src/lvm.rs b/crates/system-reinstall-bootc/src/lvm.rs index 78b6db545..92d691809 100644 --- a/crates/system-reinstall-bootc/src/lvm.rs +++ b/crates/system-reinstall-bootc/src/lvm.rs @@ -3,6 +3,7 @@ use std::process::Command; use anyhow::Result; use bootc_mount::run_findmnt; use bootc_utils::{CommandRunExt, ResultExt}; +use fn_error_context::context; use serde::Deserialize; #[derive(Debug, Deserialize)] @@ -23,6 +24,7 @@ pub(crate) struct LogicalVolume { vg_name: String, } +#[context("parse_volumes")] pub(crate) fn parse_volumes(group: Option<&str>) -> Result> { if which::which("podman").is_err() { tracing::debug!("lvs binary not found. Skipping logical volume check."); @@ -46,6 +48,7 @@ pub(crate) fn parse_volumes(group: Option<&str>) -> Result> { .collect()) } +#[context("check_root_siblings")] pub(crate) fn check_root_siblings() -> Result> { let all_volumes = parse_volumes(None)?; diff --git a/crates/system-reinstall-bootc/src/main.rs b/crates/system-reinstall-bootc/src/main.rs index 052de5ca3..5a08f0ec5 100644 --- a/crates/system-reinstall-bootc/src/main.rs +++ b/crates/system-reinstall-bootc/src/main.rs @@ -3,6 +3,7 @@ use anyhow::{ensure, Context, Result}; use bootc_utils::CommandRunExt; use clap::Parser; +use fn_error_context::context; use rustix::process::getuid; mod btrfs; @@ -29,6 +30,7 @@ struct Opts { // Note if we ever add any other options here, } +#[context("run")] fn run() -> Result<()> { // We historically supported an environment variable providing a config to override the image, so // keep supporting that. I'm considering deprecating that though. diff --git a/crates/system-reinstall-bootc/src/podman.rs b/crates/system-reinstall-bootc/src/podman.rs index af89a67d4..f88ac036d 100644 --- a/crates/system-reinstall-bootc/src/podman.rs +++ b/crates/system-reinstall-bootc/src/podman.rs @@ -3,9 +3,11 @@ use crate::prompt; use super::ROOT_KEY_MOUNT_POINT; use anyhow::{ensure, Context, Result}; use bootc_utils::CommandRunExt; +use fn_error_context::context; use std::process::Command; use which::which; +#[context("bootc_has_clean")] fn bootc_has_clean(image: &str) -> Result { let output = Command::new("podman") .args([ @@ -22,6 +24,7 @@ fn bootc_has_clean(image: &str) -> Result { Ok(stdout_str.contains("--cleanup")) } +#[context("reinstall_command")] pub(crate) fn reinstall_command(image: &str, ssh_key_file: &str) -> Result { let mut podman_command_and_args = [ // We use podman to run the bootc container. This might change in the future to remove the @@ -108,6 +111,7 @@ fn image_exists_command(image: &str) -> Command { command } +#[context("pull_if_not_present")] pub(crate) fn pull_if_not_present(image: &str) -> Result<()> { let result = image_exists_command(image).status()?; @@ -136,6 +140,7 @@ const fn podman_install_script_path() -> &'static str { } } +#[context("ensure_podman_installed")] pub(crate) fn ensure_podman_installed() -> Result<()> { if which("podman").is_ok() { return Ok(()); diff --git a/crates/system-reinstall-bootc/src/prompt.rs b/crates/system-reinstall-bootc/src/prompt.rs index f88de6b33..98f649e36 100644 --- a/crates/system-reinstall-bootc/src/prompt.rs +++ b/crates/system-reinstall-bootc/src/prompt.rs @@ -11,6 +11,7 @@ macro_rules! println_flush { use crate::{btrfs, lvm, prompt, users::get_all_users_keys}; use anyhow::{ensure, Context, Result}; +use fn_error_context::context; use crossterm::event::{self, Event}; use std::time::Duration; @@ -19,6 +20,7 @@ const NO_SSH_PROMPT: &str = "None of the users on this system found have authori if your image doesn't use cloud-init or other means to set up users, \ you may not be able to log in after reinstalling. Do you want to continue?"; +#[context("prompt_single_user")] fn prompt_single_user(user: &crate::users::UserKeys) -> Result> { let prompt = indoc::formatdoc! { "Found only one user ({user}) with {num_keys} SSH authorized keys. @@ -32,6 +34,7 @@ fn prompt_single_user(user: &crate::users::UserKeys) -> Result Result> { @@ -55,6 +58,7 @@ fn prompt_user_selection( .collect()) } +#[context("reboot")] pub(crate) fn reboot() -> Result<()> { let delay_seconds = 10; println_flush!( @@ -79,6 +83,7 @@ pub(crate) fn reboot() -> Result<()> { /// Temporary safety mechanism to stop devs from running it on their dev machine. TODO: Discuss /// final prompting UX in https://github.com/bootc-dev/bootc/discussions/1060 +#[context("temporary_developer_protection_prompt")] pub(crate) fn temporary_developer_protection_prompt() -> Result<()> { // Print an empty line so that the warning stands out from the rest of the output println_flush!(); @@ -94,6 +99,7 @@ pub(crate) fn temporary_developer_protection_prompt() -> Result<()> { Ok(()) } +#[context("ask_yes_no")] pub(crate) fn ask_yes_no(prompt: &str, default: bool) -> Result { dialoguer::Confirm::new() .with_prompt(prompt) @@ -114,6 +120,7 @@ pub(crate) fn press_enter() { } } +#[context("mount_warning")] pub(crate) fn mount_warning() -> Result<()> { let mut mounts = btrfs::check_root_siblings()?; mounts.extend(lvm::check_root_siblings()?); @@ -138,6 +145,7 @@ pub(crate) fn mount_warning() -> Result<()> { /// The keys are stored in a temporary file which is passed to /// the podman run invocation to be used by /// `bootc install to-existing-root --root-ssh-authorized-keys` +#[context("get_ssh_keys")] pub(crate) fn get_ssh_keys(temp_key_file_path: &str) -> Result<()> { let users = get_all_users_keys()?; if users.is_empty() { diff --git a/crates/system-reinstall-bootc/src/users.rs b/crates/system-reinstall-bootc/src/users.rs index f5823eba2..de08d3b1b 100644 --- a/crates/system-reinstall-bootc/src/users.rs +++ b/crates/system-reinstall-bootc/src/users.rs @@ -1,6 +1,7 @@ use anyhow::{Context, Result}; use bootc_utils::CommandRunExt; use bootc_utils::PathQuotedDisplay; +use fn_error_context::context; use openssh_keys::PublicKey; use rustix::fs::Uid; use rustix::process::geteuid; @@ -17,6 +18,7 @@ use std::os::unix::process::CommandExt; use std::process::Command; use uzers::os::unix::UserExt; +#[context("loginctl_users")] fn loginctl_users() -> Result> { let loginctl_raw_output = loginctl_run_compat()?; @@ -24,6 +26,7 @@ fn loginctl_users() -> Result> { } /// See [`test::test_parse_lsblk`] for example loginctl output +#[context("loginctl_parse")] fn loginctl_parse(users: Value) -> Result> { users .as_array() @@ -47,6 +50,7 @@ fn loginctl_parse(users: Value) -> Result> { } /// Run `loginctl` with some compatibility maneuvers to get JSON output +#[context("loginctl_run_compat")] fn loginctl_run_compat() -> Result { let mut command = Command::new("loginctl"); command.arg("list-sessions").arg("--output").arg("json"); @@ -71,6 +75,7 @@ struct UidChange { } impl UidChange { + #[context("new")] fn new(change_to_uid: Uid) -> Result { let (uid, euid) = (getuid(), geteuid()); set_thread_res_uid(uid, change_to_uid, euid).context("setting effective uid failed")?; @@ -115,6 +120,7 @@ struct SshdConfig<'a> { } impl<'a> SshdConfig<'a> { + #[context("parse")] pub fn parse(sshd_output: &'a str) -> Result> { let config = sshd_output .lines() @@ -138,6 +144,7 @@ impl<'a> SshdConfig<'a> { } } +#[context("get_keys_from_files")] fn get_keys_from_files(user: &uzers::User, keyfiles: &Vec<&str>) -> Result> { let home_dir = user.home_dir(); let mut user_authorized_keys: Vec = Vec::new(); @@ -170,6 +177,7 @@ fn get_keys_from_files(user: &uzers::User, keyfiles: &Vec<&str>) -> Result Result> { let user_config = uzers::get_user_by_name(command_user).context(format!( "authorized_keys_command_user {command_user} not found" @@ -184,6 +192,7 @@ fn get_keys_from_command(command: &str, command_user: &str) -> Result Result> { let loginctl_user_names = loginctl_users().context("enumerate users")?; From 480ac2a855b63e102f3bc726993d7f0d0fc889ce Mon Sep 17 00:00:00 2001 From: John Eckersberg Date: Tue, 7 Oct 2025 13:32:16 -0400 Subject: [PATCH 3/3] reinstall: Correctly check for `lvs` binary in `parse_volumes` Signed-off-by: John Eckersberg --- crates/system-reinstall-bootc/src/lvm.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/system-reinstall-bootc/src/lvm.rs b/crates/system-reinstall-bootc/src/lvm.rs index 92d691809..5247ae9ed 100644 --- a/crates/system-reinstall-bootc/src/lvm.rs +++ b/crates/system-reinstall-bootc/src/lvm.rs @@ -26,7 +26,7 @@ pub(crate) struct LogicalVolume { #[context("parse_volumes")] pub(crate) fn parse_volumes(group: Option<&str>) -> Result> { - if which::which("podman").is_err() { + if which::which("lvs").is_err() { tracing::debug!("lvs binary not found. Skipping logical volume check."); return Ok(Vec::::new()); }