agent: Improve baremount

Change `baremount()` to accept `Path` values rather than string values
since:

- `Path` is more natural given the function deals with paths.
- This minimises the caller having to convert between string and `Path`
  types, which simplifies the surrounding code.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

src/agent/src/mount.rs

@@ -139,8 +139,8 @@ pub const STORAGE_HANDLER_LIST: &[&str] = &[
 
 #[instrument]
 pub fn baremount(
-    source: &str,
+    source: &Path,
-    destination: &str,
+    destination: &Path,
     fs_type: &str,
     flags: MsFlags,
     options: &str,
@@ -148,11 +148,11 @@ pub fn baremount(
 ) -> Result<()> {
     let logger = logger.new(o!("subsystem" => "baremount"));
 
-    if source.is_empty() {
+    if source.as_os_str().is_empty() {
         return Err(anyhow!("need mount source"));
     }
 
-    if destination.is_empty() {
+    if destination.as_os_str().is_empty() {
         return Err(anyhow!("need mount destination"));
     }
 
@@ -444,16 +444,19 @@ fn mount_storage(logger: &Logger, storage: &Storage) -> Result<()> {
     let options_vec = options_vec.iter().map(String::as_str).collect();
     let (flags, options) = parse_mount_flags_and_options(options_vec);
 
+    let source = Path::new(&storage.source);
+    let mount_point = Path::new(&storage.mount_point);
+
     info!(logger, "mounting storage";
-    "mount-source:" => storage.source.as_str(),
+    "mount-source" => source.display(),
-    "mount-destination" => storage.mount_point.as_str(),
+    "mount-destination" => mount_point.display(),
     "mount-fstype"  => storage.fstype.as_str(),
     "mount-options" => options.as_str(),
     );
 
     baremount(
-        storage.source.as_str(),
+        source,
-        storage.mount_point.as_str(),
+        mount_point,
         storage.fstype.as_str(),
         flags,
         options.as_str(),
@@ -579,7 +582,10 @@ fn mount_to_rootfs(logger: &Logger, m: &InitMount) -> Result<()> {
 
     fs::create_dir_all(Path::new(m.dest)).context("could not create directory")?;
 
-    baremount(m.src, m.dest, m.fstype, flags, &options, logger).or_else(|e| {
+    let source = Path::new(m.src);
+    let dest = Path::new(m.dest);
+
+    baremount(source, dest, m.fstype, flags, &options, logger).or_else(|e| {
         if m.src != "dev" {
             return Err(e);
         }
@@ -937,14 +943,10 @@ mod tests {
                 std::fs::create_dir_all(d).expect("failed to created directory");
             }
 
-            let result = baremount(
+            let src = Path::new(&src_filename);
-                &src_filename,
+            let dest = Path::new(&dest_filename);
-                &dest_filename,
+
-                d.fs_type,
+            let result = baremount(src, dest, d.fs_type, d.flags, d.options, &logger);
-                d.flags,
-                d.options,
-                &logger,
-            );
 
             let msg = format!("{}: result: {:?}", msg, result);
 
@@ -1021,15 +1023,11 @@ mod tests {
                 .unwrap_or_else(|_| panic!("failed to create directory {}", d));
         }
 
+        let src = Path::new(mnt_src_filename);
+        let dest = Path::new(mnt_dest_filename);
+
         // Create an actual mount
-        let result = baremount(
+        let result = baremount(src, dest, "bind", MsFlags::MS_BIND, "", &logger);
-            mnt_src_filename,
-            mnt_dest_filename,
-            "bind",
-            MsFlags::MS_BIND,
-            "",
-            &logger,
-        );
         assert!(result.is_ok(), "mount for test setup failed");
 
         let tests = &[

src/agent/src/namespace.rs

@@ -104,7 +104,10 @@ impl Namespace {
             if let Err(err) = || -> Result<()> {
                 let origin_ns_path = get_current_thread_ns_path(ns_type.get());
 
-                File::open(Path::new(&origin_ns_path))?;
+                let source = Path::new(&origin_ns_path);
+                let destination = new_ns_path.as_path();
+
+                File::open(&source)?;
 
                 // Create a new netns on the current thread.
                 let cf = ns_type.get_flags();
@@ -115,8 +118,6 @@ impl Namespace {
                     nix::unistd::sethostname(hostname.unwrap())?;
                 }
                 // Bind mount the new namespace from the current thread onto the mount point to persist it.
-                let source: &str = origin_ns_path.as_str();
-                let destination: &str = new_ns_path.as_path().to_str().unwrap_or("none");
 
                 let mut flags = MsFlags::empty();
 
@@ -131,7 +132,7 @@ impl Namespace {
 
                 baremount(source, destination, "none", flags, "", &logger).map_err(|e| {
                     anyhow!(
-                        "Failed to mount {} to {} with err:{:?}",
+                        "Failed to mount {:?} to {:?} with err:{:?}",
                         source,
                         destination,
                         e

src/agent/src/rpc.rs

@@ -1607,10 +1607,13 @@ async fn do_add_swap(sandbox: &Arc<Mutex<Sandbox>>, req: &AddSwapRequest) -> Res
 // - container rootfs bind mounted at /<CONTAINER_BASE>/<cid>/rootfs
 // - modify container spec root to point to /<CONTAINER_BASE>/<cid>/rootfs
 fn setup_bundle(cid: &str, spec: &mut Spec) -> Result<PathBuf> {
-    if spec.root.is_none() {
+    let spec_root = if let Some(sr) = &spec.root {
+        sr
+    } else {
         return Err(nix::Error::Sys(Errno::EINVAL).into());
-    }
+    };
-    let spec_root = spec.root.as_ref().unwrap();
+
+    let spec_root_path = Path::new(&spec_root.path);
 
     let bundle_path = Path::new(CONTAINER_BASE).join(cid);
     let config_path = bundle_path.join("config.json");
@@ -1618,15 +1621,21 @@ fn setup_bundle(cid: &str, spec: &mut Spec) -> Result<PathBuf> {
 
     fs::create_dir_all(&rootfs_path)?;
     baremount(
-        &spec_root.path,
+        spec_root_path,
-        rootfs_path.to_str().unwrap(),
+        &rootfs_path,
         "bind",
         MsFlags::MS_BIND,
         "",
         &sl!(),
     )?;
+
+    let rootfs_path_name = rootfs_path
+        .to_str()
+        .ok_or_else(|| anyhow!("failed to convert rootfs to unicode"))?
+        .to_string();
+
     spec.root = Some(Root {
-        path: rootfs_path.to_str().unwrap().to_owned(),
+        path: rootfs_path_name,
         readonly: spec_root.readonly,
     });
 

src/agent/src/sandbox.rs

@@ -458,10 +458,14 @@ mod tests {
     use slog::Logger;
     use std::fs::{self, File};
     use std::os::unix::fs::PermissionsExt;
+    use std::path::Path;
     use tempfile::Builder;
 
     fn bind_mount(src: &str, dst: &str, logger: &Logger) -> Result<(), Error> {
-        baremount(src, dst, "bind", MsFlags::MS_BIND, "", logger)
+        let src_path = Path::new(src);
+        let dst_path = Path::new(dst);
+
+        baremount(src_path, dst_path, "bind", MsFlags::MS_BIND, "", logger)
     }
 
     use serial_test::serial;

src/agent/src/watcher.rs

@@ -366,8 +366,8 @@ impl SandboxStorages {
                         }
 
                         match baremount(
-                            entry.source_mount_point.to_str().unwrap(),
+                            entry.source_mount_point.as_path(),
-                            entry.target_mount_point.to_str().unwrap(),
+                            entry.target_mount_point.as_path(),
                             "bind",
                             MsFlags::MS_BIND,
                             "bind",
@@ -477,8 +477,8 @@ impl BindWatcher {
         fs::create_dir_all(WATCH_MOUNT_POINT_PATH).await?;
 
         baremount(
-            "tmpfs",
+            Path::new("tmpfs"),
-            WATCH_MOUNT_POINT_PATH,
+            Path::new(WATCH_MOUNT_POINT_PATH),
             "tmpfs",
             MsFlags::empty(),
             "",