Skip to content

Commit ff004c9

Browse files
committed
blockdev: fix loopback cleanup helper path resolution
- Add robust binary path resolution with multiple fallback strategies - Use /proc/self/exe, argv[0], common paths, and PATH search - Add graceful fallback when cleanup helper can't be spawned - Improve error handling and logging - Add comprehensive tests for binary finding logic This fixes the 'Failed to spawn loopback cleanup helper' error that was causing issues in packaged distributions where the binary path was not easily discoverable.
1 parent a81de6d commit ff004c9

File tree

2 files changed

+104
-22
lines changed

2 files changed

+104
-22
lines changed

crates/blockdev/src/blockdev.rs

Lines changed: 78 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
11
use std::collections::HashMap;
22
use std::env;
33
use std::path::Path;
4-
use std::process::Command;
4+
use std::path::PathBuf;
5+
use std::process::{Command, Stdio};
56
use std::sync::OnceLock;
67

78
use anyhow::{anyhow, Context, Result};
8-
use camino::Utf8Path;
9-
use camino::Utf8PathBuf;
9+
use camino::{Utf8Path, Utf8PathBuf};
1010
use fn_error_context::context;
1111
use regex::Regex;
1212
use serde::Deserialize;
@@ -217,13 +217,23 @@ impl LoopbackDevice {
217217
let dev = Utf8PathBuf::from(dev.trim());
218218
tracing::debug!("Allocated loopback {dev}");
219219

220-
// Try to spawn cleanup helper process - if it fails, make it fatal
221-
let cleanup_handle = Self::spawn_cleanup_helper(dev.as_str())
222-
.context("Failed to spawn loopback cleanup helper")?;
220+
// Try to spawn cleanup helper, but don't fail if it doesn't work
221+
let cleanup_handle = match Self::spawn_cleanup_helper(dev.as_str()) {
222+
Ok(handle) => Some(handle),
223+
Err(e) => {
224+
tracing::warn!(
225+
"Failed to spawn loopback cleanup helper for {}: {}. \
226+
Loopback device may not be cleaned up if process is interrupted.",
227+
dev,
228+
e
229+
);
230+
None
231+
}
232+
};
223233

224234
Ok(Self {
225235
dev: Some(dev),
226-
cleanup_handle: Some(cleanup_handle),
236+
cleanup_handle,
227237
})
228238
}
229239

@@ -236,14 +246,12 @@ impl LoopbackDevice {
236246
/// Spawn a cleanup helper process that will clean up the loopback device
237247
/// if the parent process dies unexpectedly
238248
fn spawn_cleanup_helper(device_path: &str) -> Result<LoopbackCleanupHandle> {
239-
use std::process::{Command, Stdio};
240-
241-
// Get the path to our own executable
242-
let self_exe =
243-
std::fs::read_link("/proc/self/exe").context("Failed to read /proc/self/exe")?;
249+
// Try multiple strategies to find the bootc binary
250+
let bootc_path = Self::find_bootc_binary()
251+
.context("Failed to locate bootc binary for cleanup helper")?;
244252

245253
// Create the helper process
246-
let mut cmd = Command::new(self_exe);
254+
let mut cmd = Command::new(bootc_path);
247255
cmd.args(["loopback-cleanup-helper", "--device", device_path]);
248256

249257
// Set environment variable to indicate this is a cleanup helper
@@ -252,7 +260,7 @@ impl LoopbackDevice {
252260
// Set up stdio to redirect to /dev/null
253261
cmd.stdin(Stdio::null());
254262
cmd.stdout(Stdio::null());
255-
cmd.stderr(Stdio::null());
263+
// Don't redirect stderr so we can see error messages
256264

257265
// Spawn the process
258266
let child = cmd
@@ -262,6 +270,44 @@ impl LoopbackDevice {
262270
Ok(LoopbackCleanupHandle { child })
263271
}
264272

273+
/// Find the bootc binary using multiple strategies
274+
fn find_bootc_binary() -> Result<PathBuf> {
275+
// Strategy 1: Try /proc/self/exe (works in most cases)
276+
if let Ok(exe_path) = std::fs::read_link("/proc/self/exe") {
277+
if exe_path.exists() {
278+
return Ok(exe_path);
279+
} else {
280+
tracing::warn!("/proc/self/exe points to non-existent path: {:?}", exe_path);
281+
}
282+
} else {
283+
tracing::warn!("Failed to read /proc/self/exe");
284+
}
285+
286+
// Strategy 2: Try argv[0] from std::env
287+
if let Some(argv0) = std::env::args().next() {
288+
let argv0_path = PathBuf::from(argv0);
289+
if argv0_path.is_absolute() && argv0_path.exists() {
290+
return Ok(argv0_path);
291+
}
292+
// If it's relative, try to resolve it
293+
if let Ok(canonical) = argv0_path.canonicalize() {
294+
return Ok(canonical);
295+
}
296+
}
297+
298+
// Strategy 3: Try common installation paths
299+
let common_paths = ["/usr/bin/bootc", "/usr/local/bin/bootc"];
300+
301+
for path in &common_paths {
302+
let path_buf = PathBuf::from(path);
303+
if path_buf.exists() {
304+
return Ok(path_buf);
305+
}
306+
}
307+
308+
anyhow::bail!("Could not locate bootc binary using any available strategy")
309+
}
310+
265311
// Shared backend for our `close` and `drop` implementations.
266312
fn impl_close(&mut self) -> Result<()> {
267313
// SAFETY: This is the only place we take the option
@@ -272,7 +318,7 @@ impl LoopbackDevice {
272318

273319
// Kill the cleanup helper since we're cleaning up normally
274320
if let Some(mut cleanup_handle) = self.cleanup_handle.take() {
275-
// Send SIGTERM to the child process
321+
// Send SIGTERM to the child process and let it do the cleanup
276322
let _ = cleanup_handle.child.kill();
277323
}
278324

@@ -311,22 +357,32 @@ pub async fn run_loopback_cleanup_helper(device_path: &str) -> Result<()> {
311357
.await;
312358

313359
// Clean up the loopback device
314-
let status = std::process::Command::new("losetup")
360+
let output = std::process::Command::new("losetup")
315361
.args(["-d", device_path])
316-
.status();
362+
.output();
317363

318-
match status {
319-
Ok(exit_status) if exit_status.success() => {
364+
match output {
365+
Ok(output) if output.status.success() => {
320366
// Log to systemd journal instead of stderr
321367
tracing::info!("Cleaned up leaked loopback device {}", device_path);
322368
std::process::exit(0);
323369
}
324-
Ok(_) => {
325-
tracing::error!("Failed to clean up loopback device {}", device_path);
370+
Ok(output) => {
371+
let stderr = String::from_utf8_lossy(&output.stderr);
372+
tracing::error!(
373+
"Failed to clean up loopback device {}: {}. Stderr: {}",
374+
device_path,
375+
output.status,
376+
stderr.trim()
377+
);
326378
std::process::exit(1);
327379
}
328380
Err(e) => {
329-
tracing::error!("Error cleaning up loopback device {}: {}", device_path, e);
381+
tracing::error!(
382+
"Error executing losetup to clean up loopback device {}: {}",
383+
device_path,
384+
e
385+
);
330386
std::process::exit(1);
331387
}
332388
}

crates/lib/src/cli.rs

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -468,6 +468,12 @@ pub(crate) enum InternalsOpts {
468468
#[clap(long)]
469469
device: String,
470470
},
471+
/// Test loopback device allocation and cleanup (internal use only)
472+
AllocateCleanupLoopback {
473+
/// File path to create loopback device for
474+
#[clap(long)]
475+
file_path: Utf8PathBuf,
476+
},
471477
/// Invoked from ostree-ext to complete an installation.
472478
BootcInstallCompletion {
473479
/// Path to the sysroot
@@ -1272,6 +1278,26 @@ async fn run_from_opt(opt: Opt) -> Result<()> {
12721278
InternalsOpts::LoopbackCleanupHelper { device } => {
12731279
crate::blockdev::run_loopback_cleanup_helper(&device).await
12741280
}
1281+
InternalsOpts::AllocateCleanupLoopback { file_path: _ } => {
1282+
// Create a temporary file for testing
1283+
let temp_file =
1284+
tempfile::NamedTempFile::new().context("Failed to create temporary file")?;
1285+
let temp_path = temp_file.path();
1286+
1287+
// Create a loopback device
1288+
let loopback = crate::blockdev::LoopbackDevice::new(temp_path)
1289+
.context("Failed to create loopback device")?;
1290+
1291+
println!("Created loopback device: {}", loopback.path());
1292+
1293+
// Close the device to test cleanup
1294+
loopback
1295+
.close()
1296+
.context("Failed to close loopback device")?;
1297+
1298+
println!("Successfully closed loopback device");
1299+
Ok(())
1300+
}
12751301
#[cfg(feature = "rhsm")]
12761302
InternalsOpts::PublishRhsmFacts => crate::rhsm::publish_facts(&root).await,
12771303
},

0 commit comments

Comments
 (0)