diff --git a/crates/blockdev/src/blockdev.rs b/crates/blockdev/src/blockdev.rs index 54198799..f9ee28dc 100644 --- a/crates/blockdev/src/blockdev.rs +++ b/crates/blockdev/src/blockdev.rs @@ -1,12 +1,12 @@ use std::collections::HashMap; use std::env; use std::path::Path; -use std::process::Command; +use std::path::PathBuf; +use std::process::{Command, Stdio}; use std::sync::OnceLock; use anyhow::{anyhow, Context, Result}; -use camino::Utf8Path; -use camino::Utf8PathBuf; +use camino::{Utf8Path, Utf8PathBuf}; use fn_error_context::context; use regex::Regex; use serde::Deserialize; @@ -217,13 +217,23 @@ impl LoopbackDevice { let dev = Utf8PathBuf::from(dev.trim()); tracing::debug!("Allocated loopback {dev}"); - // Try to spawn cleanup helper process - if it fails, make it fatal - let cleanup_handle = Self::spawn_cleanup_helper(dev.as_str()) - .context("Failed to spawn loopback cleanup helper")?; + // Try to spawn cleanup helper, but don't fail if it doesn't work + let cleanup_handle = match Self::spawn_cleanup_helper(dev.as_str()) { + Ok(handle) => Some(handle), + Err(e) => { + tracing::warn!( + "Failed to spawn loopback cleanup helper for {}: {}. \ + Loopback device may not be cleaned up if process is interrupted.", + dev, + e + ); + None + } + }; Ok(Self { dev: Some(dev), - cleanup_handle: Some(cleanup_handle), + cleanup_handle, }) } @@ -236,14 +246,12 @@ impl LoopbackDevice { /// Spawn a cleanup helper process that will clean up the loopback device /// if the parent process dies unexpectedly fn spawn_cleanup_helper(device_path: &str) -> Result { - use std::process::{Command, Stdio}; - - // Get the path to our own executable - let self_exe = - std::fs::read_link("/proc/self/exe").context("Failed to read /proc/self/exe")?; + // Try multiple strategies to find the bootc binary + let bootc_path = Self::find_bootc_binary() + .context("Failed to locate bootc binary for cleanup helper")?; // Create the helper process - let mut cmd = Command::new(self_exe); + let mut cmd = Command::new(bootc_path); cmd.args(["loopback-cleanup-helper", "--device", device_path]); // Set environment variable to indicate this is a cleanup helper @@ -252,7 +260,7 @@ impl LoopbackDevice { // Set up stdio to redirect to /dev/null cmd.stdin(Stdio::null()); cmd.stdout(Stdio::null()); - cmd.stderr(Stdio::null()); + // Don't redirect stderr so we can see error messages // Spawn the process let child = cmd @@ -262,6 +270,44 @@ impl LoopbackDevice { Ok(LoopbackCleanupHandle { child }) } + /// Find the bootc binary using multiple strategies + fn find_bootc_binary() -> Result { + // Strategy 1: Try /proc/self/exe (works in most cases) + if let Ok(exe_path) = std::fs::read_link("/proc/self/exe") { + if exe_path.exists() { + return Ok(exe_path); + } else { + tracing::warn!("/proc/self/exe points to non-existent path: {:?}", exe_path); + } + } else { + tracing::warn!("Failed to read /proc/self/exe"); + } + + // Strategy 2: Try argv[0] from std::env + if let Some(argv0) = std::env::args().next() { + let argv0_path = PathBuf::from(argv0); + if argv0_path.is_absolute() && argv0_path.exists() { + return Ok(argv0_path); + } + // If it's relative, try to resolve it + if let Ok(canonical) = argv0_path.canonicalize() { + return Ok(canonical); + } + } + + // Strategy 3: Try common installation paths + let common_paths = ["/usr/bin/bootc", "/usr/local/bin/bootc"]; + + for path in &common_paths { + let path_buf = PathBuf::from(path); + if path_buf.exists() { + return Ok(path_buf); + } + } + + anyhow::bail!("Could not locate bootc binary using any available strategy") + } + // Shared backend for our `close` and `drop` implementations. fn impl_close(&mut self) -> Result<()> { // SAFETY: This is the only place we take the option @@ -272,7 +318,7 @@ impl LoopbackDevice { // Kill the cleanup helper since we're cleaning up normally if let Some(mut cleanup_handle) = self.cleanup_handle.take() { - // Send SIGTERM to the child process + // Send SIGTERM to the child process and let it do the cleanup let _ = cleanup_handle.child.kill(); } @@ -311,22 +357,32 @@ pub async fn run_loopback_cleanup_helper(device_path: &str) -> Result<()> { .await; // Clean up the loopback device - let status = std::process::Command::new("losetup") + let output = std::process::Command::new("losetup") .args(["-d", device_path]) - .status(); + .output(); - match status { - Ok(exit_status) if exit_status.success() => { + match output { + Ok(output) if output.status.success() => { // Log to systemd journal instead of stderr tracing::info!("Cleaned up leaked loopback device {}", device_path); std::process::exit(0); } - Ok(_) => { - tracing::error!("Failed to clean up loopback device {}", device_path); + Ok(output) => { + let stderr = String::from_utf8_lossy(&output.stderr); + tracing::error!( + "Failed to clean up loopback device {}: {}. Stderr: {}", + device_path, + output.status, + stderr.trim() + ); std::process::exit(1); } Err(e) => { - tracing::error!("Error cleaning up loopback device {}: {}", device_path, e); + tracing::error!( + "Error executing losetup to clean up loopback device {}: {}", + device_path, + e + ); std::process::exit(1); } } diff --git a/crates/lib/src/cli.rs b/crates/lib/src/cli.rs index 80543b7d..1ba55872 100644 --- a/crates/lib/src/cli.rs +++ b/crates/lib/src/cli.rs @@ -468,6 +468,12 @@ pub(crate) enum InternalsOpts { #[clap(long)] device: String, }, + /// Test loopback device allocation and cleanup (internal use only) + AllocateCleanupLoopback { + /// File path to create loopback device for + #[clap(long)] + file_path: Utf8PathBuf, + }, /// Invoked from ostree-ext to complete an installation. BootcInstallCompletion { /// Path to the sysroot @@ -1272,6 +1278,26 @@ async fn run_from_opt(opt: Opt) -> Result<()> { InternalsOpts::LoopbackCleanupHelper { device } => { crate::blockdev::run_loopback_cleanup_helper(&device).await } + InternalsOpts::AllocateCleanupLoopback { file_path: _ } => { + // Create a temporary file for testing + let temp_file = + tempfile::NamedTempFile::new().context("Failed to create temporary file")?; + let temp_path = temp_file.path(); + + // Create a loopback device + let loopback = crate::blockdev::LoopbackDevice::new(temp_path) + .context("Failed to create loopback device")?; + + println!("Created loopback device: {}", loopback.path()); + + // Close the device to test cleanup + loopback + .close() + .context("Failed to close loopback device")?; + + println!("Successfully closed loopback device"); + Ok(()) + } #[cfg(feature = "rhsm")] InternalsOpts::PublishRhsmFacts => crate::rhsm::publish_facts(&root).await, },