Merge pull request #850 from Luap99/osstring

use OsString/Path over String for file paths

src/commands/firewalld_reload.rs

@@ -1,3 +1,8 @@
+use std::{
+    ffi::{OsStr, OsString},
+    path::Path,
+};
+
 use zbus::{blocking::Connection, dbus_proxy, CacheProperties};
 
 use crate::{
@@ -15,11 +20,13 @@ trait FirewallDDbus {}
 
 const SIGNAL_NAME: &str = "Reloaded";
 
-pub fn listen(config_dir: Option<String>) -> NetavarkResult<()> {
-    let config_dir = config_dir
-        .as_deref()
-        .unwrap_or(constants::DEFAULT_CONFIG_DIR);
-    log::debug!("looking for firewall configs in {}", config_dir);
+pub fn listen(config_dir: Option<OsString>) -> NetavarkResult<()> {
+    let config_dir = Path::new(
+        config_dir
+            .as_deref()
+            .unwrap_or(OsStr::new(constants::DEFAULT_CONFIG_DIR)),
+    );
+    log::debug!("looking for firewall configs in {:?}", config_dir);
 
     let conn = Connection::system()?;
     let proxy = FirewallDDbusProxyBlocking::builder(&conn)
@@ -41,13 +48,13 @@ pub fn listen(config_dir: Option<String>) -> NetavarkResult<()> {
     Ok(())
 }
 
-fn reload_rules(config_dir: &str) {
+fn reload_rules(config_dir: &Path) {
     if let Err(e) = reload_rules_inner(config_dir) {
         log::error!("failed to reload firewall rules: {e}");
     }
 }
 
-fn reload_rules_inner(config_dir: &str) -> NetavarkResult<()> {
+fn reload_rules_inner(config_dir: &Path) -> NetavarkResult<()> {
     let conf = read_fw_config(config_dir).wrap("read firewall config")?;
     // If we got no conf there are no containers so nothing to do.
     if let Some(conf) = conf {

src/commands/mod.rs

@@ -1,3 +1,5 @@
+use std::ffi::OsString;
+
 use crate::error::{NetavarkError, NetavarkResult};
 
 pub mod dhcp_proxy;
@@ -7,7 +9,7 @@ pub mod teardown;
 pub mod update;
 pub mod version;
 
-fn get_config_dir(dir: Option<String>, cmd: &str) -> NetavarkResult<String> {
+fn get_config_dir(dir: Option<OsString>, cmd: &str) -> NetavarkResult<OsString> {
     dir.ok_or_else(|| {
         NetavarkError::msg(format!(
             "--config not specified but required for netavark {}",

src/commands/setup.rs

@@ -12,6 +12,7 @@ use clap::builder::NonEmptyStringValueParser;
 use clap::Parser;
 use log::{debug, error, info};
 use std::collections::HashMap;
+use std::ffi::OsString;
 use std::fs::{self};
 use std::os::fd::AsFd;
 use std::path::Path;
@@ -33,10 +34,10 @@ impl Setup {
 
     pub fn exec(
         &self,
-        input_file: Option<String>,
-        config_dir: Option<String>,
-        aardvark_bin: String,
-        plugin_directories: Option<Vec<String>>,
+        input_file: Option<OsString>,
+        config_dir: Option<OsString>,
+        aardvark_bin: OsString,
+        plugin_directories: Option<Vec<OsString>>,
         rootless: bool,
     ) -> NetavarkResult<()> {
         match network::validation::ns_checks(&self.network_namespace_path) {
@@ -85,7 +86,7 @@ impl Setup {
                     per_network_opts,
                     port_mappings: &network_options.port_mappings,
                     dns_port,
-                    config_dir: &config_dir,
+                    config_dir: Path::new(&config_dir),
                     rootless,
                 },
                 &plugin_directories,
@@ -145,19 +146,7 @@ impl Setup {
                     }
                 }
 
-                let path_string = match path.into_os_string().into_string() {
-                    Ok(path) => path,
-                    Err(_) => {
-                        return Err(std::io::Error::new(
-                            std::io::ErrorKind::Other,
-                            "failed to convert path to String",
-                        )
-                        .into());
-                    }
-                };
-
-                let aardvark_interface =
-                    Aardvark::new(path_string, rootless, aardvark_bin, dns_port);
+                let aardvark_interface = Aardvark::new(path, rootless, aardvark_bin, dns_port);
 
                 if let Err(er) = aardvark_interface.commit_netavark_entries(aardvark_entries) {
                     return Err(std::io::Error::new(

src/commands/teardown.rs

@@ -9,6 +9,7 @@ use crate::{firewall, network};
 use clap::builder::NonEmptyStringValueParser;
 use clap::Parser;
 use log::debug;
+use std::ffi::OsString;
 use std::os::fd::AsFd;
 use std::path::Path;
 
@@ -29,10 +30,10 @@ impl Teardown {
 
     pub fn exec(
         &self,
-        input_file: Option<String>,
-        config_dir: Option<String>,
-        aardvark_bin: String,
-        plugin_directories: Option<Vec<String>>,
+        input_file: Option<OsString>,
+        config_dir: Option<OsString>,
+        aardvark_bin: OsString,
+        plugin_directories: Option<Vec<OsString>>,
         rootless: bool,
     ) -> NetavarkResult<()> {
         debug!("{:?}", "Tearing down..");
@@ -62,17 +63,10 @@ impl Teardown {
         if !aardvark_entries.is_empty() {
             // stop dns server first before netavark clears the interface
             let path = Path::new(&config_dir).join("aardvark-dns");
-            if let Ok(path_string) = path.into_os_string().into_string() {
-                let aardvark_interface =
-                    Aardvark::new(path_string, rootless, aardvark_bin, dns_port);
-                if let Err(err) = aardvark_interface.delete_from_netavark_entries(aardvark_entries)
-                {
-                    error_list.push(NetavarkError::wrap("remove aardvark entries", err));
-                }
-            } else {
-                error_list.push(NetavarkError::msg(
-                    "Unable to parse aardvark config directory",
-                ));
+
+            let aardvark_interface = Aardvark::new(path, rootless, aardvark_bin, dns_port);
+            if let Err(err) = aardvark_interface.delete_from_netavark_entries(aardvark_entries) {
+                error_list.push(NetavarkError::wrap("remove aardvark entries", err));
             }
         }
 
@@ -108,7 +102,7 @@ impl Teardown {
                     per_network_opts,
                     port_mappings: &network_options.port_mappings,
                     dns_port,
-                    config_dir: &config_dir,
+                    config_dir: Path::new(&config_dir),
                     rootless,
                 },
                 &plugin_directories,

src/commands/update.rs

@@ -6,6 +6,7 @@ use crate::network::core_utils;
 use clap::builder::NonEmptyStringValueParser;
 use clap::Parser;
 use log::debug;
+use std::ffi::OsString;
 use std::path::Path;
 
 #[derive(Parser, Debug)]
@@ -29,32 +30,26 @@ impl Update {
 
     pub fn exec(
         &mut self,
-        config_dir: Option<String>,
-        aardvark_bin: String,
+        config_dir: Option<OsString>,
+        aardvark_bin: OsString,
         rootless: bool,
     ) -> NetavarkResult<()> {
         let dns_port = core_utils::get_netavark_dns_port()?;
         let config_dir = get_config_dir(config_dir, "update")?;
         if Path::new(&aardvark_bin).exists() {
             let path = Path::new(&config_dir).join("aardvark-dns");
-            if let Ok(path_string) = path.into_os_string().into_string() {
-                let aardvark_interface =
-                    Aardvark::new(path_string, rootless, aardvark_bin, dns_port);
-                // if empty network_dns_servers are passed, pass empty array instead of `[""]`
-                if self.network_dns_servers.len() == 1 && self.network_dns_servers[0].is_empty() {
-                    self.network_dns_servers = Vec::new();
-                }
-                if let Err(err) = aardvark_interface
-                    .modify_network_dns_servers(&self.network_name, &self.network_dns_servers)
-                {
-                    return Err(NetavarkError::wrap(
-                        "unable to modify network dns servers",
-                        err,
-                    ));
-                }
-            } else {
-                return Err(NetavarkError::msg(
-                    "Unable to parse aardvark config directory",
+
+            let aardvark_interface = Aardvark::new(path, rootless, aardvark_bin, dns_port);
+            // if empty network_dns_servers are passed, pass empty array instead of `[""]`
+            if self.network_dns_servers.len() == 1 && self.network_dns_servers[0].is_empty() {
+                self.network_dns_servers = Vec::new();
+            }
+            if let Err(err) = aardvark_interface
+                .modify_network_dns_servers(&self.network_name, &self.network_dns_servers)
+            {
+                return Err(NetavarkError::wrap(
+                    "unable to modify network dns servers",
+                    err,
                 ));
             }
         }

src/dns/aardvark.rs

@@ -4,14 +4,15 @@ use fs2::FileExt;
 use libc::pid_t;
 use nix::sys::signal::{self, Signal};
 use nix::unistd::Pid;
+use std::ffi::{OsStr, OsString};
 use std::fs;
 use std::fs::File;
 use std::fs::OpenOptions;
 use std::io::Result;
 use std::io::{prelude::*, ErrorKind};
 use std::net::Ipv4Addr;
 use std::net::{IpAddr, Ipv6Addr};
-use std::path::Path;
+use std::path::{Path, PathBuf};
 use std::process::{Command, Stdio};
 
 const SYSTEMD_CHECK_PATH: &str = "/run/systemd/system";
@@ -33,22 +34,22 @@ pub struct AardvarkEntry<'a> {
 #[derive(Debug, Clone)]
 pub struct Aardvark {
     /// aardvark's config directory
-    pub config: String,
+    pub config: PathBuf,
     /// tells if container is rootfull or rootless
     pub rootless: bool,
     /// path to the aardvark-dns binary
-    pub aardvark_bin: String,
+    pub aardvark_bin: OsString,
     /// port to bind to
-    pub port: String,
+    pub port: OsString,
 }
 
 impl Aardvark {
-    pub fn new(config: String, rootless: bool, aardvark_bin: String, port: u16) -> Self {
+    pub fn new(config: PathBuf, rootless: bool, aardvark_bin: OsString, port: u16) -> Self {
         Aardvark {
             config,
             rootless,
             aardvark_bin,
-            port: port.to_string(),
+            port: port.to_string().into(),
         }
     }
 
@@ -89,20 +90,24 @@ impl Aardvark {
         // only use systemd when it is booted, see sd_booted(3)
         if Path::new(SYSTEMD_CHECK_PATH).exists() && Aardvark::is_executable_in_path(SYSTEMD_RUN) {
             // TODO: This could be replaced by systemd-api.
-            aardvark_args = vec![SYSTEMD_RUN, "-q", "--scope"];
+            aardvark_args = vec![
+                OsStr::new(SYSTEMD_RUN),
+                OsStr::new("-q"),
+                OsStr::new("--scope"),
+            ];
 
             if self.rootless {
-                aardvark_args.push("--user");
+                aardvark_args.push(OsStr::new("--user"));
             }
         }
 
         aardvark_args.extend(vec![
-            self.aardvark_bin.as_str(),
-            "--config",
-            &self.config,
-            "-p",
-            &self.port,
-            "run",
+            self.aardvark_bin.as_os_str(),
+            OsStr::new("--config"),
+            self.config.as_os_str(),
+            OsStr::new("-p"),
+            self.port.as_os_str(),
+            OsStr::new("run"),
         ]);
 
         log::debug!("start aardvark-dns: {:?}", aardvark_args);

src/firewall/state.rs

@@ -68,7 +68,7 @@ fn remove_file_ignore_enoent<P: AsRef<Path>>(path: P) -> io::Result<()> {
     }
 }
 
-fn firewall_config_dir(config_dir: &str) -> PathBuf {
+fn firewall_config_dir(config_dir: &Path) -> PathBuf {
     Path::new(config_dir).join(FIREWALL_DIR)
 }
 
@@ -78,7 +78,7 @@ fn firewall_config_dir(config_dir: &str) -> PathBuf {
 /// As a special case when network_id and container_id is empty it will return
 /// the paths for the directories instead which are used to walk the dir for all configs.
 fn get_file_paths(
-    config_dir: &str,
+    config_dir: &Path,
     network_id: &str,
     container_id: &str,
     create_dirs: bool,
@@ -126,7 +126,7 @@ fn get_file_paths(
 /// This should be caller after firewall setup to allow the firewalld reload
 /// service to read the configs later and readd the rules.
 pub fn write_fw_config(
-    config_dir: &str,
+    config_dir: &Path,
     network_id: &str,
     container_id: &str,
     fw_driver: &str,
@@ -168,7 +168,7 @@ pub fn write_fw_config(
 /// On firewall teardown remove the specific config files again so the
 /// firewalld reload service does not keep using them.
 pub fn remove_fw_config(
-    config_dir: &str,
+    config_dir: &Path,
     network_id: &str,
     container_id: &str,
     complete_teardown: bool,
@@ -206,7 +206,7 @@ pub struct FirewallConfig {
 }
 
 /// Read all firewall configs files from the dir.
-pub fn read_fw_config(config_dir: &str) -> NetavarkResult<Option<FirewallConfig>> {
+pub fn read_fw_config(config_dir: &Path) -> NetavarkResult<Option<FirewallConfig>> {
     let paths = get_file_paths(config_dir, "", "", false)?;
 
     // now it is possible the firewall-reload is started before any containers were started so we just
@@ -262,7 +262,7 @@ mod tests {
         let driver = "iptables";
 
         let tmpdir = Builder::new().prefix("netavark-tests").tempdir().unwrap();
-        let config_dir = tmpdir.path().to_str().unwrap();
+        let config_dir = tmpdir.path();
 
         let net_conf = SetupNetwork {
             subnets: Some(vec!["10.0.0.0/24".parse().unwrap()]),
@@ -343,7 +343,7 @@ mod tests {
     #[test]
     fn test_read_fw_config_empty() {
         let tmpdir = Builder::new().prefix("netavark-tests").tempdir().unwrap();
-        let config_dir = tmpdir.path().to_str().unwrap();
+        let config_dir = tmpdir.path();
 
         let res = read_fw_config(config_dir).expect("no read_fw_config error");
         assert!(res.is_none(), "no firewall config should be given");