From 8cbd34e5e90a6adc5815e2990bee6cea622aa1b2 Mon Sep 17 00:00:00 2001 From: Matthew Ahrens Date: Tue, 17 May 2022 10:55:55 -0700 Subject: [PATCH] use `PathBuf`/`&Path` to represent file/directory paths (#431) In general, `PathBuf`/`&Path` should be used instead of `String`/`&str` because unix paths can be any nul-terminated byte array, which is more general than Rust's String (which must be valid UTF-8). By using String, we are unable to operate on some files (those whose names are not valid UTF-8). However, for the agent, this isn't that big of a deal in practice, since this isn't a general file-processing utility. However, we can still benefit from using Path: * The more specific type makes it more clear what variables are used for * We can take advantage of the directory-path-specific helper methods This commit changes our codebase to use PathBuf/Path. --- cmd/zfs_object_agent/object_perf/src/main.rs | 5 +-- cmd/zfs_object_agent/server/src/main.rs | 17 ++++----- cmd/zfs_object_agent/util/src/logging.rs | 7 ++-- cmd/zfs_object_agent/util/src/message.rs | 3 +- cmd/zfs_object_agent/util/src/tunable.rs | 9 +++-- .../util/src/zcache_devices.rs | 4 ++- cmd/zfs_object_agent/zcache/src/add.rs | 6 ++-- cmd/zfs_object_agent/zcache/src/list.rs | 19 +++++----- cmd/zfs_object_agent/zcache/src/main.rs | 4 ++- cmd/zfs_object_agent/zcdb/src/main.rs | 12 ++++--- .../zettacache/src/block_access.rs | 35 +++++++++--------- .../zettacache/src/block_allocator/zcdb.rs | 5 ++- cmd/zfs_object_agent/zettacache/src/open.rs | 36 +++++++------------ .../zettacache/src/superblock.rs | 4 +-- .../zettacache/src/zcachedb.rs | 4 ++- .../zettacache/src/zettacache/mod.rs | 7 ++-- .../zettacache/src/zettacache/zcdb.rs | 5 +-- cmd/zfs_object_agent/zettaobject/src/init.rs | 8 ++--- .../zettaobject/src/pool_destroy.rs | 18 ++++++---- .../zettaobject/src/public_connection.rs | 5 +-- .../zettaobject/src/root_connection.rs | 12 +++---- .../zettaobject/src/server.rs | 15 ++++---- cmd/zfs_object_agent/zoa/src/lib.rs | 21 ++++++----- 23 files changed, 140 insertions(+), 121 deletions(-) diff --git a/cmd/zfs_object_agent/object_perf/src/main.rs b/cmd/zfs_object_agent/object_perf/src/main.rs index 7b209cba2906..d4ea1cd4ccae 100644 --- a/cmd/zfs_object_agent/object_perf/src/main.rs +++ b/cmd/zfs_object_agent/object_perf/src/main.rs @@ -2,6 +2,7 @@ #![allow(clippy::print_stderr)] #![allow(clippy::print_stdout)] +use std::path::PathBuf; use std::time::Duration; use clap::Parser; @@ -69,11 +70,11 @@ struct Cli { value_name = "FILE", default_value = "/var/log/perflog" )] - output_file: String, + output_file: PathBuf, /// Configuration file to set tunables (toml/json/yaml) #[clap(short = 't', long, value_name = "FILE")] - config_file: Option, + config_file: Option, #[clap(subcommand)] command: Commands, diff --git a/cmd/zfs_object_agent/server/src/main.rs b/cmd/zfs_object_agent/server/src/main.rs index a080dd903985..56882b59ce61 100644 --- a/cmd/zfs_object_agent/server/src/main.rs +++ b/cmd/zfs_object_agent/server/src/main.rs @@ -1,3 +1,4 @@ +use std::path::PathBuf; use std::time::Duration; use clap::Args; @@ -38,7 +39,7 @@ struct LoggingArgs { /// File to log output to #[clap(short = 'o', long, value_name = "FILE")] - output_file: Option, + output_file: Option, /// Logging configuration yaml file #[clap( @@ -48,7 +49,7 @@ struct LoggingArgs { conflicts_with = "output-file", conflicts_with = "verbosity" )] - log_config: Option, + log_config: Option, } #[derive(Parser)] @@ -62,11 +63,11 @@ struct Cli { /// Configuration file to set tunables (toml/json/yaml) #[clap(short = 't', long, value_name = "FILE")] - config_file: Option, + config_file: Option, /// Directory for unix-domain sockets #[clap(short = 'k', long, value_name = "DIR", default_value = "/etc/zfs")] - socket_dir: String, + socket_dir: PathBuf, /// File/device to use for ZettaCache #[clap( @@ -75,12 +76,12 @@ struct Cli { value_name = "PATH", conflicts_with = "cache-device-dir" )] - cache_device: Option>, + cache_device: Option>, /// Directory path to use for importing devices that are part of the /// ZettaCache #[clap(short = 'd', long, value_name = "DIR")] - cache_device_dir: Option, + cache_device_dir: Option, /// Clear the cache when it has incompatible features #[clap(long)] @@ -269,8 +270,8 @@ fn main() { }); let cache_mode = match cli.cache_device { - Some(paths) => Some(CacheOpenMode::new_device_list(paths)), - None => cli.cache_device_dir.map(CacheOpenMode::new_device_dir), + Some(paths) => Some(CacheOpenMode::DeviceList(paths)), + None => cli.cache_device_dir.map(CacheOpenMode::DiscoveryDirectory), }; match zettaobject::init::start( diff --git a/cmd/zfs_object_agent/util/src/logging.rs b/cmd/zfs_object_agent/util/src/logging.rs index 0a6ad06fe3e6..8c6b93265b7c 100644 --- a/cmd/zfs_object_agent/util/src/logging.rs +++ b/cmd/zfs_object_agent/util/src/logging.rs @@ -5,6 +5,7 @@ use std::io::BufWriter; use std::io::Write; use std::panic; use std::panic::PanicInfo; +use std::path::Path; use std::process; use std::sync::atomic::AtomicBool; use std::sync::atomic::Ordering; @@ -241,7 +242,7 @@ fn setup_console_logging(verbosity: u64) { log4rs::init_config(config).unwrap(); } -fn setup_logfile(verbosity: u64, logfile: &str) { +fn setup_logfile(verbosity: u64, logfile: &Path) { let config = Config::builder() .appender( Appender::builder() @@ -277,8 +278,8 @@ fn setup_logfile(verbosity: u64, logfile: &str) { pub fn setup_logging( verbosity: u64, - file_name: Option<&str>, - log_config: Option<&str>, + file_name: Option<&Path>, + log_config: Option<&Path>, quiet_start: bool, ) { /* diff --git a/cmd/zfs_object_agent/util/src/message.rs b/cmd/zfs_object_agent/util/src/message.rs index 158e44577935..5b1df39cb242 100644 --- a/cmd/zfs_object_agent/util/src/message.rs +++ b/cmd/zfs_object_agent/util/src/message.rs @@ -1,5 +1,6 @@ use std::fmt::Debug; use std::mem::size_of; +use std::path::PathBuf; use std::ptr; use std::slice; @@ -141,5 +142,5 @@ pub const TYPE_INITIATE_MERGE: &str = "initiate merge"; #[derive(Serialize, Deserialize, Debug)] pub struct AddDiskRequest { - pub path: String, + pub path: PathBuf, } diff --git a/cmd/zfs_object_agent/util/src/tunable.rs b/cmd/zfs_object_agent/util/src/tunable.rs index f60c7257a844..8c086fc2c4c6 100644 --- a/cmd/zfs_object_agent/util/src/tunable.rs +++ b/cmd/zfs_object_agent/util/src/tunable.rs @@ -1,5 +1,6 @@ use std::any::type_name; use std::fmt::Debug; +use std::path::Path; use std::sync::atomic::*; use std::sync::RwLock; use std::time::Duration; @@ -289,10 +290,14 @@ impl Percent { } } -pub fn read_config(file_name: &str) -> Result<()> { +pub fn read_config(file_name: &Path) -> Result<()> { let mut config = CONFIG.write().unwrap(); *config = Config::builder() - .add_source(config::File::with_name(file_name)) + .add_source(config::File::with_name( + file_name + .to_str() + .ok_or_else(|| anyhow!("file name not utf8"))?, + )) .build()?; Ok(()) } diff --git a/cmd/zfs_object_agent/util/src/zcache_devices.rs b/cmd/zfs_object_agent/util/src/zcache_devices.rs index 227b8d59cae8..af071608d1b2 100644 --- a/cmd/zfs_object_agent/util/src/zcache_devices.rs +++ b/cmd/zfs_object_agent/util/src/zcache_devices.rs @@ -3,12 +3,14 @@ //! These structures on the zettacache side are serialized and then deserialized //! by the zettacache subcommands. +use std::path::PathBuf; + use serde::Deserialize; use serde::Serialize; #[derive(Serialize, Deserialize)] pub struct DeviceEntry { - pub name: String, + pub name: PathBuf, pub size: u64, } diff --git a/cmd/zfs_object_agent/zcache/src/add.rs b/cmd/zfs_object_agent/zcache/src/add.rs index 47e20da79eb2..f8f2426e7d29 100644 --- a/cmd/zfs_object_agent/zcache/src/add.rs +++ b/cmd/zfs_object_agent/zcache/src/add.rs @@ -1,5 +1,7 @@ //! `zcache add` subcommand +use std::path::PathBuf; + use anyhow::anyhow; use anyhow::Result; use async_trait::async_trait; @@ -15,7 +17,7 @@ use crate::subcommand::ZcacheSubCommand; #[derive(Parser)] #[clap(about = "Add a disk to the ZettaCache.")] pub struct Add { - path: String, + path: PathBuf, } #[async_trait] @@ -32,7 +34,7 @@ impl ZcacheSubCommand for Add { .await { Ok(_) => { - writeln_stdout!("Disk {} added", self.path); + writeln_stdout!("Disk {:?} added", self.path); } Err(RemoteError::ResultError(_)) => return Err(anyhow!("No cache found")), Err(RemoteError::Other(e)) => return Err(e), diff --git a/cmd/zfs_object_agent/zcache/src/list.rs b/cmd/zfs_object_agent/zcache/src/list.rs index cbe6edfbe3a0..d2a77a560978 100644 --- a/cmd/zfs_object_agent/zcache/src/list.rs +++ b/cmd/zfs_object_agent/zcache/src/list.rs @@ -2,7 +2,6 @@ use std::fs; use std::path::Path; -use std::path::PathBuf; use anyhow::anyhow; use anyhow::Context; @@ -50,24 +49,24 @@ pub struct List { impl List { /// Derive the device name to display based on command input flags. - fn derive_name(&self, path: &str) -> String { - let path_buf: PathBuf; - + fn derive_name(&self, path: &Path) -> String { let device_path = if self.real_paths { // Follow any symlinks to get the underlying device // e.g. "/dev/xvdz1" -> "/dev/nvme1n1p1" - path_buf = fs::canonicalize(path).unwrap(); - path_buf.as_path() + fs::canonicalize(path).unwrap_or_else(|_| path.to_owned()) } else { - Path::new(path) + path.to_owned() }; if self.full_paths { - device_path.to_str().unwrap() + device_path.to_string_lossy().into() } else { - device_path.file_name().unwrap().to_str().unwrap() + device_path + .file_name() + .unwrap_or(device_path.as_os_str()) + .to_string_lossy() + .into() } - .to_string() } fn max_name_length(&self, devices: &[DeviceEntry]) -> usize { diff --git a/cmd/zfs_object_agent/zcache/src/main.rs b/cmd/zfs_object_agent/zcache/src/main.rs index 928930aa32ff..750e412e418c 100644 --- a/cmd/zfs_object_agent/zcache/src/main.rs +++ b/cmd/zfs_object_agent/zcache/src/main.rs @@ -14,6 +14,8 @@ mod stats; mod subcommand; mod sync; +use std::path::PathBuf; + use anyhow::Result; use clap::Parser; use clap::Subcommand; @@ -38,7 +40,7 @@ struct Cli { /// File to log debugging output to #[clap(long, requires = "verbose", value_name = "FILE", global = true)] - log_file: Option, + log_file: Option, #[clap(subcommand)] command: Commands, diff --git a/cmd/zfs_object_agent/zcdb/src/main.rs b/cmd/zfs_object_agent/zcdb/src/main.rs index ceb000e8902a..6656031ef71d 100644 --- a/cmd/zfs_object_agent/zcdb/src/main.rs +++ b/cmd/zfs_object_agent/zcdb/src/main.rs @@ -1,3 +1,5 @@ +use std::path::PathBuf; + use clap::Parser; use clap::Subcommand; use git_version::git_version; @@ -21,7 +23,7 @@ static GIT_VERSION: &str = git_version!( struct Cli { /// File/device to use for ZettaCache #[clap(short = 'c', long, value_name = "PATH")] - cache_device: Option>, + cache_device: Option>, /// Directory path to use for importing devices that are part of the /// ZettaCache @@ -32,7 +34,7 @@ struct Cli { conflicts_with = "cache-device", default_value = "/dev/disk/by-id/" )] - cache_device_dir: String, + cache_device_dir: PathBuf, /// Sets the verbosity level for logging and debugging #[clap(short = 'v', long, parse(from_occurrences), global = true)] @@ -40,7 +42,7 @@ struct Cli { /// File to log debugging output to #[clap(long, requires = "verbose", value_name = "FILE", global = true)] - log_file: Option, + log_file: Option, #[clap(subcommand)] command: Commands, @@ -111,8 +113,8 @@ async fn main() -> Result<(), anyhow::Error> { // Set up cache paths let cache_mode = match cli.cache_device { - Some(paths) => CacheOpenMode::new_device_list(paths), - None => CacheOpenMode::new_device_dir(cli.cache_device_dir), + Some(paths) => CacheOpenMode::DeviceList(paths), + None => CacheOpenMode::DiscoveryDirectory(cli.cache_device_dir), }; match cli.command { diff --git a/cmd/zfs_object_agent/zettacache/src/block_access.rs b/cmd/zfs_object_agent/zettacache/src/block_access.rs index d34d7a4daa41..98bceb2c4a45 100644 --- a/cmd/zfs_object_agent/zettacache/src/block_access.rs +++ b/cmd/zfs_object_agent/zettacache/src/block_access.rs @@ -7,6 +7,7 @@ use std::io::Write; use std::os::unix::prelude::AsRawFd; use std::os::unix::prelude::OpenOptionsExt; use std::path::Path; +use std::path::PathBuf; use std::sync::atomic::Ordering; use std::sync::RwLock; use std::thread::sleep; @@ -146,7 +147,8 @@ pub struct Disk { #[allow(dead_code)] file: &'static File, - device_path: String, + path: PathBuf, + canonical_path: PathBuf, size: u64, sector_size: usize, #[derivative(Debug = "ignore")] @@ -201,7 +203,7 @@ struct WriteMessage { } impl Disk { - pub fn new(disk_path: &str, readonly: bool) -> Result { + pub fn new(path: &Path, readonly: bool) -> Result { // Note: using std file open so that this func can be non-async. // Although this is blocking from a tokio thread, it's used // infrequently, and we're already blocking from the ioctls to get the @@ -210,8 +212,8 @@ impl Disk { .read(true) .write(!readonly) .custom_flags(CUSTOM_OFLAGS) - .open(disk_path) - .with_context(|| format!("opening disk '{}'", disk_path))?; + .open(path) + .with_context(|| format!("opening disk {path:?}"))?; // see comment in `struct Disk` let file = &*Box::leak(Box::new(file)); let stat = nix::sys::stat::fstat(file.as_raw_fd())?; @@ -226,15 +228,11 @@ impl Disk { size = u64::try_from(stat.st_size)?; sector_size = *MIN_SECTOR_SIZE; } else { - panic!("{}: invalid file type {:?}", disk_path, mode); + panic!("{path:?}: invalid file type {mode:?}"); } - let device = Path::new(disk_path) - .file_name() - .unwrap() - .to_str() - .unwrap() - .to_owned(); + let short_name = path.file_name().unwrap().to_string_lossy().to_string(); + let canonical_path = Path::new(path).canonicalize()?; let (reader_tx, reader_rx) = flume::unbounded(); @@ -254,11 +252,12 @@ impl Disk { metadata_writer_rxs.push(rx); } - let io_stats = &*Box::leak(Box::new(DiskIoStats::new(device))); + let io_stats = &*Box::leak(Box::new(DiskIoStats::new(short_name))); let this = Disk { file, - device_path: disk_path.to_string(), + path: path.to_owned(), + canonical_path, size, sector_size, io_stats, @@ -298,7 +297,7 @@ impl Disk { }); } } - info!("opening cache file {}: {:?}", disk_path, this); + info!("opening cache file {path:?}: {this:?}"); Ok(this) } @@ -567,7 +566,7 @@ impl BlockAccess { .unwrap() .iter() .map(|d| DeviceEntry { - name: d.device_path.to_string(), + name: d.path.clone(), size: d.size, }) .collect(); @@ -585,10 +584,8 @@ impl BlockAccess { } } - pub fn disk_path(&self, disk: DiskId) -> String { - self.disks.read().unwrap()[disk.index()] - .device_path - .to_string() + pub fn disk_path(&self, disk: DiskId) -> PathBuf { + self.disks.read().unwrap()[disk.index()].path.clone() } pub fn total_capacity(&self) -> u64 { diff --git a/cmd/zfs_object_agent/zettacache/src/block_allocator/zcdb.rs b/cmd/zfs_object_agent/zettacache/src/block_allocator/zcdb.rs index cb65bf351e39..5c6fd2c24457 100644 --- a/cmd/zfs_object_agent/zettacache/src/block_allocator/zcdb.rs +++ b/cmd/zfs_object_agent/zettacache/src/block_allocator/zcdb.rs @@ -334,7 +334,10 @@ pub async fn zcachedb_dump_slabs( zcachedb_dump_slabs_print_legend(); for (disk, device_slabs) in slabs_per_device { writeln_stdout!("============================================================"); - writeln_stdout!("= {}", block_access.disk_path(disk)); + writeln_stdout!( + "= {:?}", + block_access.disk_path(disk) + ); writeln_stdout!("============================================================"); zcachedb_dump_slabs_report(&device_slabs, slab_size, &buckets, &opts) } diff --git a/cmd/zfs_object_agent/zettacache/src/open.rs b/cmd/zfs_object_agent/zettacache/src/open.rs index 23b73d963e66..684761cd615d 100644 --- a/cmd/zfs_object_agent/zettacache/src/open.rs +++ b/cmd/zfs_object_agent/zettacache/src/open.rs @@ -1,6 +1,7 @@ use std::collections::BTreeMap; use std::collections::HashMap; use std::collections::HashSet; +use std::path::Path; use std::path::PathBuf; use anyhow::anyhow; @@ -20,35 +21,27 @@ use crate::superblock::SuperblockPhys; use crate::superblock::SUPERBLOCK_SIZE; pub enum CacheOpenMode { - DeviceList { device_paths: Vec }, - Discovery { device_dir: String }, + DeviceList(Vec), + DiscoveryDirectory(PathBuf), } impl CacheOpenMode { - pub fn new_device_list(device_paths: Vec) -> Self { - CacheOpenMode::DeviceList { device_paths } - } - - pub fn new_device_dir(device_dir: String) -> Self { - CacheOpenMode::Discovery { device_dir } - } - - pub async fn device_paths(self) -> Result> { + pub async fn device_paths(self) -> Result> { Ok(match self { - CacheOpenMode::DeviceList { device_paths } => device_paths, - CacheOpenMode::Discovery { device_dir } => discover_devices(&device_dir).await?, + CacheOpenMode::DeviceList(paths) => paths, + CacheOpenMode::DiscoveryDirectory(dir) => discover_devices(&dir).await?, }) } } #[derive(Debug)] struct DiscoveredDevice { - device_path: String, + device_path: PathBuf, superblock: SuperblockPhys, } impl DiscoveredDevice { - fn new(device_path: String, superblock: SuperblockPhys) -> Self { + fn new(device_path: PathBuf, superblock: SuperblockPhys) -> Self { DiscoveredDevice { device_path, superblock, @@ -65,14 +58,11 @@ impl DiscoveredDevice { .with_context(|| format!("discovery: read_exact {path:?}"))?; let (superblock, _) = BlockAccess::chunk_from_raw_impl::(&buf) .with_context(|| format!("discovery: parse label {path:?}"))?; - Ok(DiscoveredDevice::new( - path.into_os_string().into_string().unwrap(), - superblock, - )) + Ok(DiscoveredDevice::new(path, superblock)) } } -async fn discover_devices(dir_path: &str) -> Result> { +async fn discover_devices(dir_path: &Path) -> Result> { let mut caches = HashMap::>::new(); let mut discovery = FuturesUnordered::new(); @@ -96,7 +86,7 @@ async fn discover_devices(dir_path: &str) -> Result> { // device import more deterministic (see DLPX-81000) error // out. return Err(anyhow!( - "found two disks with {:?} for cache '{cache_guid}'", + "found two disks with {:?} for cache {cache_guid}", old_device.superblock.disk )); } @@ -111,7 +101,7 @@ async fn discover_devices(dir_path: &str) -> Result> { // XXX - In the future we probably want to be able to specify a // cache by GUID so we can get past this error. Err(anyhow!( - "multiple valid caches found in '{dir_path}': {:?}", + "multiple valid caches found in {dir_path:?}: {:?}", caches.keys().collect::>(), )) } else { @@ -121,7 +111,7 @@ async fn discover_devices(dir_path: &str) -> Result> { .collect()) } } - None => Err(anyhow!("no valid caches found in '{dir_path}'")), + None => Err(anyhow!("no valid caches found in {dir_path:?}")), } } diff --git a/cmd/zfs_object_agent/zettacache/src/superblock.rs b/cmd/zfs_object_agent/zettacache/src/superblock.rs index 5ade34b7a8a6..2f995dd367ba 100644 --- a/cmd/zfs_object_agent/zettacache/src/superblock.rs +++ b/cmd/zfs_object_agent/zettacache/src/superblock.rs @@ -214,7 +214,7 @@ impl SuperblockPhys { for block_access_disk_id in block_access.disks() { match Self::read_impl(block_access, block_access_disk_id).await { Ok(superblock) => writeln_stdout!( - "{:?} - Path: {} Size: {} GUID: {} Primary?: {}", + "{:?} - Path: {:?} Size: {} GUID: {} Primary?: {}", superblock.disk, block_access.disk_path(block_access_disk_id), nice_p2size(block_access.disk_size(block_access_disk_id)), @@ -225,7 +225,7 @@ impl SuperblockPhys { } ), Err(_) => writeln_stderr!( - "error: {}: not a valid zettacache disk", + "error: {:?}: not a valid zettacache disk", block_access.disk_path(block_access_disk_id) ), } diff --git a/cmd/zfs_object_agent/zettacache/src/zcachedb.rs b/cmd/zfs_object_agent/zettacache/src/zcachedb.rs index 7cd257cc07e0..4f734a2a34f4 100644 --- a/cmd/zfs_object_agent/zettacache/src/zcachedb.rs +++ b/cmd/zfs_object_agent/zettacache/src/zcachedb.rs @@ -1,3 +1,5 @@ +use std::path::PathBuf; + use crate::zettacache::zcdb::ZCacheDBHandle; use crate::CacheOpenMode; @@ -136,7 +138,7 @@ impl ZettaCacheDBCommand { async fn issue_pool_state_command( command: ZettaCacheDBCommand, - paths: Vec, + paths: Vec, ) -> Result<(), anyhow::Error> { let mut handle = ZCacheDBHandle::open(paths).await?; match command { diff --git a/cmd/zfs_object_agent/zettacache/src/zettacache/mod.rs b/cmd/zfs_object_agent/zettacache/src/zettacache/mod.rs index d6547a976190..1cf155331083 100644 --- a/cmd/zfs_object_agent/zettacache/src/zettacache/mod.rs +++ b/cmd/zfs_object_agent/zettacache/src/zettacache/mod.rs @@ -10,6 +10,7 @@ use std::ops::Bound::Excluded; use std::ops::Bound::Included; use std::ops::Bound::Unbounded; use std::ops::Deref; +use std::path::Path; use std::sync::Arc; use std::time::Duration; use std::time::Instant; @@ -1838,7 +1839,7 @@ impl ZettaCache { } } - pub async fn add_disk(&self, path: &str) -> Result<()> { + pub async fn add_disk(&self, path: &Path) -> Result<()> { self.state.lock().await.add_disk(path)?; self.sync_checkpoint().await; Ok(()) @@ -2583,7 +2584,7 @@ impl ZettaCacheState { ); } - fn add_disk(&mut self, path: &str) -> Result<()> { + fn add_disk(&mut self, path: &Path) -> Result<()> { // We hold the state lock across all these operations to ensure that we're always // adding the last DiskId to the SlabAllocator and Primary, in the case of concurrent // calls to add_disk(). @@ -2605,7 +2606,7 @@ impl ZettaCacheState { // new cache size. self.clear_hit_data(); - info!("added {path} as {disk_id:?}"); + info!("added {path:?} as {disk_id:?}"); Ok(()) } diff --git a/cmd/zfs_object_agent/zettacache/src/zettacache/zcdb.rs b/cmd/zfs_object_agent/zettacache/src/zettacache/zcdb.rs index 70ee33c383cc..90ee08b03f49 100644 --- a/cmd/zfs_object_agent/zettacache/src/zettacache/zcdb.rs +++ b/cmd/zfs_object_agent/zettacache/src/zettacache/zcdb.rs @@ -1,3 +1,4 @@ +use std::path::PathBuf; use std::sync::Arc; use anyhow::Result; @@ -29,7 +30,7 @@ pub struct ZCacheDBHandle { } impl ZCacheDBHandle { - pub async fn dump_superblocks(paths: Vec) -> Result<()> { + pub async fn dump_superblocks(paths: Vec) -> Result<()> { let mut disks: Vec = Vec::with_capacity(paths.len()); for path in paths { match Disk::new(&path, true) { @@ -45,7 +46,7 @@ impl ZCacheDBHandle { Ok(()) } - pub async fn open(paths: Vec) -> Result { + pub async fn open(paths: Vec) -> Result { let mut disks: Vec = Vec::with_capacity(paths.len()); for path in paths { disks.push(Disk::new(&path, true)?); diff --git a/cmd/zfs_object_agent/zettaobject/src/init.rs b/cmd/zfs_object_agent/zettaobject/src/init.rs index 8fb0be5efcaa..551c988623d8 100644 --- a/cmd/zfs_object_agent/zettaobject/src/init.rs +++ b/cmd/zfs_object_agent/zettaobject/src/init.rs @@ -18,8 +18,8 @@ use crate::pool_destroy; use crate::public_connection::PublicServerState; use crate::root_connection::RootServerState; -fn lock_socket_dir(socket_dir: &str) { - let lock_file = format!("{}/zoa.lock", socket_dir); +fn lock_socket_dir(socket_dir: &Path) { + let lock_file = socket_dir.join("zoa.lock"); match OpenOptions::new() .read(true) .write(true) @@ -53,7 +53,7 @@ fn lock_socket_dir(socket_dir: &str) { } } Err(_) => { - error!("Failed to create lock file {}", &lock_file); + error!("Failed to create lock file {lock_file:?}"); std::process::exit(1); } } @@ -68,7 +68,7 @@ fn parse_id_from_file(id_path: &Path) -> Result { } pub fn start( - socket_dir: &str, + socket_dir: &Path, cache_mode: Option, clear_incompatible_cache: bool, runtime: Runtime, diff --git a/cmd/zfs_object_agent/zettaobject/src/pool_destroy.rs b/cmd/zfs_object_agent/zettaobject/src/pool_destroy.rs index 51785ae82b42..7cbbb0649ba1 100644 --- a/cmd/zfs_object_agent/zettaobject/src/pool_destroy.rs +++ b/cmd/zfs_object_agent/zettaobject/src/pool_destroy.rs @@ -1,5 +1,7 @@ use std::collections::HashMap; use std::io::ErrorKind; +use std::path::Path; +use std::path::PathBuf; use std::process; use std::sync::Arc; use std::time::SystemTime; @@ -154,7 +156,7 @@ struct PoolDestroyer { destroying_pools_map: DestroyingPoolsMap, // Full path to the zpool-destroy cache file, initialized at startup. - destroy_cache_filename: String, + destroy_cache_filename: PathBuf, } impl PoolDestroyer { @@ -205,11 +207,11 @@ impl PoolDestroyer { } Err(ref error) if error.kind() == ErrorKind::NotFound => { // zpool_destroy.cache file does not exist. No initialization is needed. - info!("{} does not exist", &self.destroy_cache_filename); + info!("{:?} does not exist", &self.destroy_cache_filename); Ok(()) } Err(error) => Err(anyhow!( - "Error opening {}; {:?}", + "Error opening {:?}; {:?}", &self.destroy_cache_filename, error )), @@ -223,7 +225,11 @@ impl PoolDestroyer { async fn write(&self) -> Result<()> { trace!("Writing out destroy cache file."); - let temp_filename = format!("{}.{}", &self.destroy_cache_filename, process::id()); + let temp_filename = format!( + "{}.{}", + self.destroy_cache_filename.to_string_lossy(), + process::id() + ); fs::write( &temp_filename, @@ -457,7 +463,7 @@ pub async fn remove_not_in_progress() { pool_destroyer.write().await.unwrap(); } -pub async fn init_pool_destroyer(socket_dir: &str) { +pub async fn init_pool_destroyer(socket_dir: &Path) { // The PoolDestroyer should be initialized only once. let mut maybe_pool_destroyer = POOL_DESTROYER.lock().await; assert!(maybe_pool_destroyer.is_none()); @@ -465,7 +471,7 @@ pub async fn init_pool_destroyer(socket_dir: &str) { // Filename for zpool-destroy cache file. let mut destroyer = PoolDestroyer { destroying_pools_map: Default::default(), - destroy_cache_filename: format!("{}/zpool_destroy.cache", socket_dir), + destroy_cache_filename: socket_dir.join("zpool_destroy.cache"), }; destroyer.init().await.unwrap(); diff --git a/cmd/zfs_object_agent/zettaobject/src/public_connection.rs b/cmd/zfs_object_agent/zettaobject/src/public_connection.rs index bf003678aecc..867934b3b4f5 100644 --- a/cmd/zfs_object_agent/zettaobject/src/public_connection.rs +++ b/cmd/zfs_object_agent/zettaobject/src/public_connection.rs @@ -1,3 +1,4 @@ +use std::path::Path; use std::sync::Arc; use std::sync::Mutex; @@ -54,8 +55,8 @@ impl PublicServerState { } } - pub fn start(socket_dir: &str, cache: Option) { - let socket_path = format!("{}/zfs_public_socket", socket_dir); + pub fn start(socket_dir: &Path, cache: Option) { + let socket_path = socket_dir.join("zfs_public_socket"); let mut server = Server::new( &socket_path, diff --git a/cmd/zfs_object_agent/zettaobject/src/root_connection.rs b/cmd/zfs_object_agent/zettaobject/src/root_connection.rs index 00a6581dd3da..3d2db28b73c9 100644 --- a/cmd/zfs_object_agent/zettaobject/src/root_connection.rs +++ b/cmd/zfs_object_agent/zettaobject/src/root_connection.rs @@ -1,5 +1,6 @@ use std::collections::HashMap; use std::fmt::Debug; +use std::path::Path; use std::sync::Arc; use anyhow::anyhow; @@ -71,8 +72,8 @@ impl RootServerState { } } - pub fn start(socket_dir: &str, cache: Option, id: Uuid) { - let socket_path = format!("{}/zfs_root_socket", socket_dir); + pub fn start(socket_dir: &Path, cache: Option, id: Uuid) { + let socket_path = socket_dir.join("zfs_root_socket"); let mut server = Server::new( &socket_path, 0o600, @@ -143,10 +144,9 @@ impl RootConnectionState { let request: CreatePoolRequest = nvpair::from_nvlist(&nvl)?; info!("got {:?}", request); let object_access = request.object_access.object_access().await?; - let result = match Pool::create(&object_access, &request.name, request.id.guid).await { - Ok(_) => Ok(()), - Err(e) => Err(FailureMessage::new(e)), - }; + let result = Pool::create(&object_access, &request.name, request.id.guid) + .await + .map_err(FailureMessage::new); return_result(TYPE_CREATE_POOL, request.id, result, true) }) diff --git a/cmd/zfs_object_agent/zettaobject/src/server.rs b/cmd/zfs_object_agent/zettaobject/src/server.rs index cb6741761572..e12358ff8e99 100644 --- a/cmd/zfs_object_agent/zettaobject/src/server.rs +++ b/cmd/zfs_object_agent/zettaobject/src/server.rs @@ -11,6 +11,8 @@ use std::collections::HashMap; use std::fmt::Debug; use std::fs; use std::os::unix::prelude::PermissionsExt; +use std::path::Path; +use std::path::PathBuf; use std::pin::Pin; use std::sync::Arc; @@ -56,7 +58,7 @@ tunable! { // Ss: ServerState (consumer's state associated with the server) // Cs: ConnectionState (consumer's state associated with the connection) pub struct Server { - socket_path: String, + socket_path: PathBuf, socket_permission: u32, state: Ss, connection_handler: Box>, @@ -94,7 +96,7 @@ where /// established. It is passed the server_state (Ss) and returns a /// connection_state (Cs), which is passed to each of the Handlers. pub fn new( - socket_path: &str, + socket_path: &Path, socket_permission: u32, server_state: Ss, connection_handler: Box>, @@ -158,7 +160,8 @@ where let server = Arc::new(self); // Create a temp socket file, set permissions and move it to the correct location. - let socket_path_tmp = format!("{}.tmp", server.socket_path); + let mut socket_path_tmp = server.socket_path.clone(); + socket_path_tmp.set_extension("tmp"); let _ = std::fs::remove_file(&socket_path_tmp); let _ = std::fs::remove_file(&server.socket_path); @@ -170,13 +173,13 @@ where fs::set_permissions(&socket_path_tmp, perms).unwrap(); fs::rename(socket_path_tmp, &server.socket_path).unwrap(); - info!("Listening on: {}", server.socket_path); + info!("Listening on: {:?}", server.socket_path); tokio::spawn(async move { loop { match listener.accept().await { Ok((stream, _)) => { - info!("accepted connection on {}", server.socket_path); + info!("accepted connection on {:?}", server.socket_path); let connection_state = (server.connection_handler)(&server.state); let server = server.clone(); tokio::spawn(async move { @@ -187,7 +190,7 @@ where }); } Err(e) => { - warn!("accept() on {} failed: {}", server.socket_path, e); + warn!("accept() on {:?} failed: {e}", server.socket_path); } } } diff --git a/cmd/zfs_object_agent/zoa/src/lib.rs b/cmd/zfs_object_agent/zoa/src/lib.rs index 7d9fc89142ec..a8d54fd00a3d 100644 --- a/cmd/zfs_object_agent/zoa/src/lib.rs +++ b/cmd/zfs_object_agent/zoa/src/lib.rs @@ -1,5 +1,8 @@ use std::ffi::CStr; +use std::ffi::OsStr; use std::os::raw::c_char; +use std::os::unix::prelude::OsStrExt; +use std::path::Path; use foreign_types::ForeignType; use libc::c_void; @@ -24,13 +27,11 @@ pub unsafe extern "C" fn libzoa_init( cache_path_ptr: *const c_char, // XXX change to take a list of paths handle: *mut *mut zoa_handle_t, ) -> i32 { - let socket_dir = CStr::from_ptr(socket_dir_ptr) - .to_string_lossy() - .into_owned(); - let log_file = CStr::from_ptr(log_file_ptr).to_string_lossy().into_owned(); + let socket_dir = Path::new(OsStr::from_bytes(CStr::from_ptr(socket_dir_ptr).to_bytes())); + let log_file = Path::new(OsStr::from_bytes(CStr::from_ptr(log_file_ptr).to_bytes())); let verbosity = 2; - util::setup_logging(verbosity, Some(log_file.as_str()), None, false); + util::setup_logging(verbosity, Some(log_file), None, false); let runtime = tokio::runtime::Builder::new_multi_thread() .enable_all() @@ -43,16 +44,14 @@ pub unsafe extern "C" fn libzoa_init( } if cache_path_ptr.is_null() { - if zettaobject::init::start(&socket_dir, None, false, runtime).is_err() { + if zettaobject::init::start(socket_dir, None, false, runtime).is_err() { return -1; } } else { - let cache = CStr::from_ptr(cache_path_ptr) - .to_string_lossy() - .into_owned(); + let cache_path = Path::new(OsStr::from_bytes(CStr::from_ptr(cache_path_ptr).to_bytes())); if zettaobject::init::start( - &socket_dir, - Some(CacheOpenMode::new_device_list(vec![cache])), + socket_dir, + Some(CacheOpenMode::DeviceList(vec![cache_path.to_owned()])), false, runtime, )