Merge pull request #1440 from jmarrero/revert-fallout

Revert "blockdev: implement signal-safe loopback device cleanup helper"

Author: cgwalters

Cargo.lock

@@ -11,13 +11,9 @@ anyhow = { workspace = true }
 bootc-utils = { package = "bootc-internal-utils", path = "../utils", version = "0.0.0" }
 camino = { workspace = true, features = ["serde1"] }
 fn-error-context = { workspace = true }
-libc = { workspace = true }
 regex = "1.10.4"
-rustix = { workspace = true }
 serde = { workspace = true, features = ["derive"] }
 serde_json = { workspace = true }
-tempfile = { workspace = true }
-tokio = { workspace = true, features = ["signal"] }
 tracing = { workspace = true }
 
 [dev-dependencies]

crates/blockdev/Cargo.toml

@@ -181,14 +181,6 @@ pub fn partitions_of(dev: &Utf8Path) -> Result<PartitionTable> {
 
 pub struct LoopbackDevice {
     pub dev: Option<Utf8PathBuf>,
-    // Handle to the cleanup helper process
-    cleanup_handle: Option<LoopbackCleanupHandle>,
-}
-
-/// Handle to manage the cleanup helper process for loopback devices
-struct LoopbackCleanupHandle {
-    /// Child process handle
-    child: std::process::Child,
 }
 
 impl LoopbackDevice {
@@ -216,15 +208,7 @@ impl LoopbackDevice {
             .run_get_string()?;
         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")?;
-
-        Ok(Self {
-            dev: Some(dev),
-            cleanup_handle: Some(cleanup_handle),
-        })
+        Ok(Self { dev: Some(dev) })
     }
 
     // Access the path to the loopback block device.
@@ -233,49 +217,13 @@ impl LoopbackDevice {
         self.dev.as_deref().unwrap()
     }
 
-    /// 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<LoopbackCleanupHandle> {
-        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")?;
-
-        // Create the helper process
-        let mut cmd = Command::new(self_exe);
-        cmd.args(["loopback-cleanup-helper", "--device", device_path]);
-
-        // Set environment variable to indicate this is a cleanup helper
-        cmd.env("BOOTC_LOOPBACK_CLEANUP_HELPER", "1");
-
-        // Set up stdio to redirect to /dev/null
-        cmd.stdin(Stdio::null());
-        cmd.stdout(Stdio::null());
-        cmd.stderr(Stdio::null());
-
-        // Spawn the process
-        let child = cmd
-            .spawn()
-            .context("Failed to spawn loopback cleanup helper")?;
-
-        Ok(LoopbackCleanupHandle { child })
-    }
-
     // Shared backend for our `close` and `drop` implementations.
     fn impl_close(&mut self) -> Result<()> {
         // SAFETY: This is the only place we take the option
         let Some(dev) = self.dev.take() else {
             tracing::trace!("loopback device already deallocated");
             return Ok(());
         };
-
-        // 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
-            let _ = cleanup_handle.child.kill();
-        }
-
         Command::new("losetup").args(["-d", dev.as_str()]).run()
     }
 
@@ -292,46 +240,6 @@ impl Drop for LoopbackDevice {
     }
 }
 
-/// Main function for the loopback cleanup helper process
-/// This function does not return - it either exits normally or via signal
-pub async fn run_loopback_cleanup_helper(device_path: &str) -> Result<()> {
-    // Check if we're running as a cleanup helper
-    if std::env::var("BOOTC_LOOPBACK_CLEANUP_HELPER").is_err() {
-        anyhow::bail!("This function should only be called as a cleanup helper");
-    }
-
-    // Set up death signal notification - we want to be notified when parent dies
-    rustix::process::set_parent_process_death_signal(Some(rustix::process::Signal::TERM))
-        .context("Failed to set parent death signal")?;
-
-    // Wait for SIGTERM (either from parent death or normal cleanup)
-    tokio::signal::unix::signal(tokio::signal::unix::SignalKind::terminate())
-        .expect("Failed to create signal stream")
-        .recv()
-        .await;
-
-    // Clean up the loopback device
-    let status = std::process::Command::new("losetup")
-        .args(["-d", device_path])
-        .status();
-
-    match status {
-        Ok(exit_status) if exit_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);
-            std::process::exit(1);
-        }
-        Err(e) => {
-            tracing::error!("Error cleaning up loopback device {}: {}", device_path, e);
-            std::process::exit(1);
-        }
-    }
-}
-
 /// Parse key-value pairs from lsblk --pairs.
 /// Newer versions of lsblk support JSON but the one in CentOS 7 doesn't.
 fn split_lsblk_line(line: &str) -> HashMap<String, String> {

crates/blockdev/src/blockdev.rs

@@ -462,12 +462,6 @@ pub(crate) enum InternalsOpts {
         #[clap(allow_hyphen_values = true)]
         args: Vec<OsString>,
     },
-    /// Loopback device cleanup helper (internal use only)
-    LoopbackCleanupHelper {
-        /// Device path to clean up
-        #[clap(long)]
-        device: String,
-    },
     /// Invoked from ostree-ext to complete an installation.
     BootcInstallCompletion {
         /// Path to the sysroot
@@ -1269,9 +1263,6 @@ async fn run_from_opt(opt: Opt) -> Result<()> {
                 let rootfs = &Dir::open_ambient_dir("/", cap_std::ambient_authority())?;
                 crate::install::completion::run_from_ostree(rootfs, &sysroot, &stateroot).await
             }
-            InternalsOpts::LoopbackCleanupHelper { device } => {
-                crate::blockdev::run_loopback_cleanup_helper(&device).await
-            }
             #[cfg(feature = "rhsm")]
             InternalsOpts::PublishRhsmFacts => crate::rhsm::publish_facts(&root).await,
         },

crates/lib/src/cli.rs

@@ -38,6 +38,3 @@ mod kernel;
 
 #[cfg(feature = "rhsm")]
 mod rhsm;
-
-// Re-export blockdev crate for internal use
-pub(crate) use bootc_blockdev as blockdev;