[pve-devel] [PATCH installer] proxmox-chroot: replace nix::(u)mount calls with external (u)mount

* [pve-devel] [PATCH installer] proxmox-chroot: replace nix::(u)mount calls with external (u)mount
@ 2025-03-11 13:27 Christoph Heiss
  2025-04-04  8:17 ` Thomas Lamprecht
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Christoph Heiss @ 2025-03-11 13:27 UTC (permalink / raw)
  To: pve-devel

Comes with a reduction of 52 -> 40 in terms of crate dependencies for
proxmox-chroot, 198 -> 192 for a full workspace build.

Currently, this is done inconsistently anyway, i.e. there are calls to
the external mount(8) as well as mount(2) and umount(2) via `nix`.
Just switch over to calling the external programs completely, which in
turn allows to drop the `nix` crate dependency from the tree.

No functional changes.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 debian/control             |  1 -
 proxmox-chroot/Cargo.toml  |  1 -
 proxmox-chroot/src/main.rs | 81 +++++++++++++++++++++++++-------------
 3 files changed, 53 insertions(+), 30 deletions(-)

diff --git a/debian/control b/debian/control
index 1ab4f19..dec07ed 100644
--- a/debian/control
+++ b/debian/control
@@ -15,7 +15,6 @@ Build-Depends: cargo:native,
                librust-glob-0.3-dev,
                librust-hex-0.4-dev,
                librust-native-tls-dev,
-               librust-nix-0.26+default-dev,
                librust-pretty-assertions-1.4-dev,
                librust-regex-1+default-dev (>= 1.7~~),
                librust-rustls-0.21+dangerous-configuration-dev,
diff --git a/proxmox-chroot/Cargo.toml b/proxmox-chroot/Cargo.toml
index a23f757..7a1390a 100644
--- a/proxmox-chroot/Cargo.toml
+++ b/proxmox-chroot/Cargo.toml
@@ -14,4 +14,3 @@ serde_json.workspace = true
 regex.workspace = true
 
 clap = { version = "4.0", features = ["derive"] }
-nix = "0.26.1"
diff --git a/proxmox-chroot/src/main.rs b/proxmox-chroot/src/main.rs
index f51f6d0..ef339ed 100644
--- a/proxmox-chroot/src/main.rs
+++ b/proxmox-chroot/src/main.rs
@@ -1,12 +1,11 @@
 use std::{
     fs, io,
-    path::{self, PathBuf},
+    path::{self, Path, PathBuf},
     process::Command,
 };
 
 use anyhow::{Result, bail};
 use clap::{Args, Parser, Subcommand, ValueEnum};
-use nix::mount::{MsFlags, mount, umount};
 use proxmox_installer_common::{
     RUNTIME_DIR,
     options::FsType,
@@ -118,9 +117,7 @@ fn cleanup(args: &CommandCleanup) -> Result<()> {
 
     match fs {
         Filesystems::Zfs => umount_zpool(),
-        Filesystems::Xfs => umount_fs()?,
-        Filesystems::Ext4 => umount_fs()?,
-        _ => (),
+        Filesystems::Btrfs | Filesystems::Xfs | Filesystems::Ext4 => umount(Path::new(TARGET_DIR))?,
     }
 
     println!("Chroot cleanup done. You can now reboot or leave the shell.");
@@ -213,7 +210,7 @@ fn mount_fs() -> Result<()> {
 
     match Command::new("mount")
         .arg(format!("/dev/mapper/{product}-root"))
-        .arg("/target")
+        .arg(TARGET_DIR)
         .output()
     {
         Err(e) => bail!("{e}"),
@@ -232,9 +229,25 @@ fn mount_fs() -> Result<()> {
     Ok(())
 }
 
-fn umount_fs() -> Result<()> {
-    umount(TARGET_DIR)?;
-    Ok(())
+fn umount(path: &Path) -> Result<()> {
+    match Command::new("umount")
+        .arg("--recursive")
+        .arg("--verbose")
+        .arg(path)
+        .output()
+    {
+        Ok(output) => {
+            if output.status.success() {
+                Ok(())
+            } else {
+                bail!(
+                    "unmounting of {path:?} failed: {}",
+                    String::from_utf8(output.stderr)?
+                )
+            }
+        }
+        Err(err) => bail!("unmounting of {path:?} failed: {:#}", err),
+    }
 }
 
 fn mount_btrfs(btrfs_uuid: Option<String>) -> Result<()> {
@@ -246,7 +259,7 @@ fn mount_btrfs(btrfs_uuid: Option<String>) -> Result<()> {
     match Command::new("mount")
         .arg("--uuid")
         .arg(uuid)
-        .arg("/target")
+        .arg(TARGET_DIR)
         .output()
     {
         Err(e) => bail!("{e}"),
@@ -303,36 +316,48 @@ fn get_btrfs_uuid() -> Result<String> {
     Ok(uuids[0].into())
 }
 
-fn bindmount() -> Result<()> {
-    println!("Bind mounting");
-    // https://github.com/nix-rust/nix/blob/7badbee1e388618457ed0d725c1091359f253012/test/test_mount.rs#L19
-    // https://github.com/nix-rust/nix/blob/7badbee1e388618457ed0d725c1091359f253012/test/test_mount.rs#L146
-    const NONE: Option<&'static [u8]> = None;
+fn do_bindmount(source: &Path, target: &Path) -> Result<()> {
+    match Command::new("mount")
+        .arg("--bind")
+        .arg(source)
+        .arg(target)
+        .output()
+    {
+        Ok(output) => {
+            if output.status.success() {
+                Ok(())
+            } else {
+                bail!(
+                    "bind-mount of {source:?} to {target:?} failed: {}",
+                    String::from_utf8(output.stderr)?
+                )
+            }
+        }
+        Err(err) => bail!("bind-mount of {source:?} to {target:?} failed: {:#}", err),
+    }
+}
+
+fn bindmount() -> Result<()> {
+    println!("Bind-mounting virtual filesystems");
 
-    let flags = MsFlags::MS_BIND;
     for item in BINDMOUNTS {
         let source = path::Path::new("/").join(item);
         let target = path::Path::new(TARGET_DIR).join(item);
 
-        println!("Bindmount {source:?} to {target:?}");
-        mount(Some(source.as_path()), target.as_path(), NONE, flags, NONE)?;
+        println!("Bind-mounting {source:?} to {target:?}");
+        do_bindmount(&source, &target)?;
     }
 
     let answer_path = path::Path::new("/mnt").join(ANSWER_MP);
+
     if answer_path.exists() {
         let target = path::Path::new(TARGET_DIR).join("mnt").join(ANSWER_MP);
 
-        println!("Create dir {target:?}");
+        println!("Creating target directory {target:?}");
         fs::create_dir_all(&target)?;
 
-        println!("Bindmount {answer_path:?} to {target:?}");
-        mount(
-            Some(answer_path.as_path()),
-            target.as_path(),
-            NONE,
-            flags,
-            NONE,
-        )?;
+        println!("Bind-mounting {answer_path:?} to {target:?}");
+        do_bindmount(&answer_path, &target)?;
     }
     Ok(())
 }
@@ -349,7 +374,7 @@ fn bind_umount() -> Result<()> {
     let answer_target = path::Path::new(TARGET_DIR).join("mnt").join(ANSWER_MP);
     if answer_target.exists() {
         println!("Unmounting and removing answer mountpoint");
-        if let Err(e) = umount(answer_target.as_os_str()) {
+        if let Err(e) = umount(&answer_target) {
             eprintln!("{e}");
         }
         if let Err(e) = fs::remove_dir(answer_target) {
-- 
2.48.1



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 5+ messages in thread