From a7d1c70c4bced794750a7d7f3778f873d0c1943c Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Mon, 15 Nov 2021 10:57:54 +0000 Subject: [PATCH] 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 --- src/agent/src/mount.rs | 48 ++++++++++++++++++-------------------- src/agent/src/namespace.rs | 9 +++---- src/agent/src/rpc.rs | 21 ++++++++++++----- src/agent/src/sandbox.rs | 6 ++++- src/agent/src/watcher.rs | 8 +++---- 5 files changed, 52 insertions(+), 40 deletions(-) diff --git a/src/agent/src/mount.rs b/src/agent/src/mount.rs index 3d55f874f0..fb92311bc3 100644 --- a/src/agent/src/mount.rs +++ b/src/agent/src/mount.rs @@ -139,8 +139,8 @@ pub const STORAGE_HANDLER_LIST: &[&str] = &[ #[instrument] pub fn baremount( - source: &str, - destination: &str, + source: &Path, + 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-destination" => storage.mount_point.as_str(), + "mount-source" => source.display(), + "mount-destination" => mount_point.display(), "mount-fstype" => storage.fstype.as_str(), "mount-options" => options.as_str(), ); baremount( - storage.source.as_str(), - storage.mount_point.as_str(), + source, + 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( - &src_filename, - &dest_filename, - d.fs_type, - d.flags, - d.options, - &logger, - ); + let src = Path::new(&src_filename); + let dest = Path::new(&dest_filename); + + let result = baremount(src, dest, d.fs_type, 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( - mnt_src_filename, - mnt_dest_filename, - "bind", - MsFlags::MS_BIND, - "", - &logger, - ); + let result = baremount(src, dest, "bind", MsFlags::MS_BIND, "", &logger); assert!(result.is_ok(), "mount for test setup failed"); let tests = &[ diff --git a/src/agent/src/namespace.rs b/src/agent/src/namespace.rs index 061370a46f..c821a0acb1 100644 --- a/src/agent/src/namespace.rs +++ b/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 diff --git a/src/agent/src/rpc.rs b/src/agent/src/rpc.rs index 778da07fe5..4cf58830ea 100644 --- a/src/agent/src/rpc.rs +++ b/src/agent/src/rpc.rs @@ -1607,10 +1607,13 @@ async fn do_add_swap(sandbox: &Arc>, req: &AddSwapRequest) -> Res // - container rootfs bind mounted at ///rootfs // - modify container spec root to point to ///rootfs fn setup_bundle(cid: &str, spec: &mut Spec) -> Result { - 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 { fs::create_dir_all(&rootfs_path)?; baremount( - &spec_root.path, - rootfs_path.to_str().unwrap(), + spec_root_path, + &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, }); diff --git a/src/agent/src/sandbox.rs b/src/agent/src/sandbox.rs index ddc18c5c9b..5264640147 100644 --- a/src/agent/src/sandbox.rs +++ b/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; diff --git a/src/agent/src/watcher.rs b/src/agent/src/watcher.rs index bb3fb16411..b3cd3f832b 100644 --- a/src/agent/src/watcher.rs +++ b/src/agent/src/watcher.rs @@ -366,8 +366,8 @@ impl SandboxStorages { } match baremount( - entry.source_mount_point.to_str().unwrap(), - entry.target_mount_point.to_str().unwrap(), + entry.source_mount_point.as_path(), + 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", - WATCH_MOUNT_POINT_PATH, + Path::new("tmpfs"), + Path::new(WATCH_MOUNT_POINT_PATH), "tmpfs", MsFlags::empty(), "",