Merge branch 'cleanup-some-talpid-core'

3 files changed, 57 insertions, 76 deletions

diff --git a/talpid-core/src/process/openvpn.rs b/talpid-core/src/process/openvpn.rs
index a55d5d3453..50b9e59799 100644
--- a/talpid-core/src/process/openvpn.rs
+++ b/talpid-core/src/process/openvpn.rs
@@ -88,7 +88,7 @@ impl OpenVpnCommand {
     }
 
     /// Sets what configuration file will be given to OpenVPN
-    pub fn config<P: AsRef<Path>>(&mut self, path: P) -> &mut Self {
+    pub fn config(&mut self, path: impl AsRef<Path>) -> &mut Self {
         self.config = Some(path.as_ref().to_path_buf());
         self
     }
@@ -101,54 +101,48 @@ impl OpenVpnCommand {
 
     /// Sets the path to the file where the username and password for user-pass authentication
     /// is stored. See the `--auth-user-pass` OpenVPN documentation for details.
-    pub fn user_pass<P: AsRef<Path>>(&mut self, path: P) -> &mut Self {
+    pub fn user_pass(&mut self, path: impl AsRef<Path>) -> &mut Self {
         self.user_pass_path = Some(path.as_ref().to_path_buf());
         self
     }
 
     /// Sets the path to the file where the username and password for proxy authentication
     /// is stored.
-    pub fn proxy_auth<P: AsRef<Path>>(&mut self, path: P) -> &mut Self {
+    pub fn proxy_auth(&mut self, path: impl AsRef<Path>) -> &mut Self {
         self.proxy_auth_path = Some(path.as_ref().to_path_buf());
         self
     }
 
     /// Sets the path to the CA certificate file.
-    pub fn ca<P: AsRef<Path>>(&mut self, path: P) -> &mut Self {
+    pub fn ca(&mut self, path: impl AsRef<Path>) -> &mut Self {
         self.ca = Some(path.as_ref().to_path_buf());
         self
     }
 
     /// Sets the path to the CRL (Certificate revocation list) file.
-    pub fn crl<P: AsRef<Path>>(&mut self, path: P) -> &mut Self {
+    pub fn crl(&mut self, path: impl AsRef<Path>) -> &mut Self {
         self.crl = Some(path.as_ref().to_path_buf());
         self
     }
 
     /// Sets the path to the ip route command.
-    pub fn iproute_bin<S: Into<OsString>>(&mut self, iproute_bin: S) -> &mut Self {
+    pub fn iproute_bin(&mut self, iproute_bin: impl Into<OsString>) -> &mut Self {
         self.iproute_bin = Some(iproute_bin.into());
         self
     }
 
     /// Sets a plugin and its arguments that OpenVPN will be started with.
-    pub fn plugin<P: AsRef<Path>>(&mut self, path: P, args: Vec<String>) -> &mut Self {
+    pub fn plugin(&mut self, path: impl AsRef<Path>, args: Vec<String>) -> &mut Self {
         self.plugin = Some((path.as_ref().to_path_buf(), args));
         self
     }
 
     /// Sets a log file path.
-    pub fn log<P: AsRef<Path>>(&mut self, path: P) -> &mut Self {
+    pub fn log(&mut self, path: impl AsRef<Path>) -> &mut Self {
         self.log = Some(path.as_ref().to_path_buf());
         self
     }
 
-    /// Build a runnable expression from the current state of the command.
-    pub fn build(&self) -> duct::Expression {
-        log::debug!("Building expression: {}", &self);
-        duct::cmd(&self.openvpn_bin, self.get_arguments()).unchecked()
-    }
-
     /// Sets extra options
     pub fn tunnel_options(&mut self, tunnel_options: &net::openvpn::TunnelOptions) -> &mut Self {
         self.tunnel_options = tunnel_options.clone();
@@ -168,8 +162,14 @@ impl OpenVpnCommand {
         self
     }
 
+    /// Build a runnable expression from the current state of the command.
+    pub fn build(&self) -> duct::Expression {
+        log::debug!("Building expression: {}", &self);
+        duct::cmd(&self.openvpn_bin, self.get_arguments()).unchecked()
+    }
+
     /// Returns all arguments that the subprocess would be spawned with.
-    pub fn get_arguments(&self) -> Vec<OsString> {
+    fn get_arguments(&self) -> Vec<OsString> {
         let mut args: Vec<OsString> = Self::base_arguments().iter().map(OsString::from).collect();
 
         if let Some(ref config) = self.config {
diff --git a/talpid-core/src/tunnel/mod.rs b/talpid-core/src/tunnel/mod.rs
index c0f1abcde2..db73f2656d 100644
--- a/talpid-core/src/tunnel/mod.rs
+++ b/talpid-core/src/tunnel/mod.rs
@@ -5,7 +5,7 @@ use std::{
     net::IpAddr,
     path::{Path, PathBuf},
 };
-
+use crate::logging;
 #[cfg(unix)]
 use talpid_types::net::wireguard as wireguard_types;
 use talpid_types::net::{openvpn as openvpn_types, GenericTunnelOptions, TunnelParameters};
@@ -16,6 +16,9 @@ pub mod openvpn;
 #[cfg(unix)]
 mod wireguard;
 
+const OPENVPN_LOG_FILENAME: &str = "openvpn.log";
+const WIREGUARD_LOG_FILENAME: &str = "wireguard.log";
+
 
 error_chain! {
     errors {
@@ -35,6 +38,10 @@ error_chain! {
         UnsupportedPlatform {
             description("Tunnel type not supported on this operating system")
         }
+        /// Failed to rotate tunnel log file
+        RotateLogError {
+            description("Failed to rotate tunnel log file")
+        }
     }
 
     links {
@@ -117,7 +124,7 @@ impl TunnelMonitor {
     pub fn start<L>(
         tunnel_parameters: &TunnelParameters,
         tunnel_alias: Option<OsString>,
-        log: Option<PathBuf>,
+        log_dir: &Option<PathBuf>,
         resource_dir: &Path,
         on_event: L,
     ) -> Result<Self>
@@ -125,14 +132,15 @@ impl TunnelMonitor {
         L: Fn(TunnelEvent) + Send + Sync + 'static,
     {
         Self::ensure_ipv6_can_be_used_if_enabled(&tunnel_parameters.get_generic_options())?;
+        let log_file = Self::prepare_tunnel_log_file(&tunnel_parameters, log_dir)?;
 
         match tunnel_parameters {
             TunnelParameters::OpenVpn(config) => {
-                Self::start_openvpn_tunnel(&config, tunnel_alias, log, resource_dir, on_event)
+                Self::start_openvpn_tunnel(&config, tunnel_alias, log_file, resource_dir, on_event)
             }
             #[cfg(unix)]
             TunnelParameters::Wireguard(config) => {
-                Self::start_wireguard_tunnel(&config, log, on_event)
+                Self::start_wireguard_tunnel(&config, log_file, on_event)
             }
             #[cfg(windows)]
             TunnelParameters::Wireguard(_) => bail!(ErrorKind::UnsupportedPlatform),
@@ -187,6 +195,23 @@ impl TunnelMonitor {
         }
     }
 
+    fn prepare_tunnel_log_file(
+        parameters: &TunnelParameters,
+        log_dir: &Option<PathBuf>,
+    ) -> Result<Option<PathBuf>> {
+        if let Some(ref log_dir) = log_dir {
+            let filename = match parameters {
+                TunnelParameters::OpenVpn(_) => OPENVPN_LOG_FILENAME,
+                TunnelParameters::Wireguard(_) => WIREGUARD_LOG_FILENAME,
+            };
+            let tunnel_log = log_dir.join(filename);
+            logging::rotate_log(&tunnel_log).chain_err(|| ErrorKind::RotateLogError)?;
+            Ok(Some(tunnel_log))
+        } else {
+            Ok(None)
+        }
+    }
+
 
     /// Creates a handle to this monitor, allowing the tunnel to be closed while some other
     /// thread
diff --git a/talpid-core/src/tunnel_state_machine/connecting_state.rs b/talpid-core/src/tunnel_state_machine/connecting_state.rs
index 0896fd18cf..305fb24383 100644
--- a/talpid-core/src/tunnel_state_machine/connecting_state.rs
+++ b/talpid-core/src/tunnel_state_machine/connecting_state.rs
@@ -23,32 +23,18 @@ use super::{
 };
 use crate::{
     firewall::FirewallPolicy,
-    logging,
     tunnel::{self, CloseHandle, TunnelEvent, TunnelMetadata, TunnelMonitor},
 };
 
 
 const MIN_TUNNEL_ALIVE_TIME: Duration = Duration::from_millis(1000);
 
-const OPENVPN_LOG_FILENAME: &str = "openvpn.log";
-const WIREGUARD_LOG_FILENAME: &str = "wireguard.log";
-
 #[cfg(windows)]
 const TUNNEL_INTERFACE_ALIAS: Option<&str> = Some("Mullvad");
 #[cfg(not(windows))]
 const TUNNEL_INTERFACE_ALIAS: Option<&str> = None;
 
-error_chain! {
-    errors {
-        RotateLogError {
-            description("Failed to rotate tunnel log file")
-        }
-    }
-
-    links {
-        TunnelMonitorError(tunnel::Error, tunnel::ErrorKind);
-    }
-}
+error_chain! {}
 
 /// The tunnel has been started, but it is not established/functional.
 pub struct ConnectingState {
@@ -86,9 +72,18 @@ impl ConnectingState {
         log_dir: &Option<PathBuf>,
         resource_dir: &Path,
         retry_attempt: u32,
-    ) -> Result<Self> {
+    ) -> crate::tunnel::Result<Self> {
         let (event_tx, event_rx) = mpsc::unbounded();
-        let monitor = Self::spawn_tunnel_monitor(&parameters, log_dir, resource_dir, event_tx)?;
+        let on_tunnel_event = move |event| {
+            let _ = event_tx.unbounded_send(event);
+        };
+        let monitor = TunnelMonitor::start(
+            &parameters,
+            TUNNEL_INTERFACE_ALIAS.to_owned().map(OsString::from),
+            log_dir,
+            resource_dir,
+            on_tunnel_event,
+        )?;
         let close_handle = monitor.close_handle();
         let tunnel_close_event = Self::spawn_tunnel_monitor_wait_thread(monitor);
 
@@ -101,43 +96,6 @@ impl ConnectingState {
         })
     }
 
-    fn spawn_tunnel_monitor(
-        parameters: &TunnelParameters,
-        log_dir: &Option<PathBuf>,
-        resource_dir: &Path,
-        events: mpsc::UnboundedSender<TunnelEvent>,
-    ) -> Result<TunnelMonitor> {
-        let on_tunnel_event = move |event| {
-            let _ = events.unbounded_send(event);
-        };
-        let log_file = Self::prepare_tunnel_log_file(&parameters, log_dir)?;
-
-        Ok(TunnelMonitor::start(
-            &parameters,
-            TUNNEL_INTERFACE_ALIAS.to_owned().map(OsString::from),
-            log_file.clone(),
-            resource_dir,
-            on_tunnel_event,
-        )?)
-    }
-
-    fn prepare_tunnel_log_file(
-        parameters: &TunnelParameters,
-        log_dir: &Option<PathBuf>,
-    ) -> Result<Option<PathBuf>> {
-        if let Some(ref log_dir) = log_dir {
-            let filename = match parameters {
-                TunnelParameters::OpenVpn(_) => OPENVPN_LOG_FILENAME,
-                TunnelParameters::Wireguard(_) => WIREGUARD_LOG_FILENAME,
-            };
-            let tunnel_log = log_dir.join(filename);
-            logging::rotate_log(&tunnel_log).chain_err(|| ErrorKind::RotateLogError)?;
-            Ok(Some(tunnel_log))
-        } else {
-            Ok(None)
-        }
-    }
-
     fn spawn_tunnel_monitor_wait_thread(
         tunnel_monitor: TunnelMonitor,
     ) -> oneshot::Receiver<Option<BlockReason>> {
@@ -392,9 +350,7 @@ impl TunnelState for ConnectingState {
                         }
                         Err(error) => {
                             let block_reason = match *error.kind() {
-                                ErrorKind::TunnelMonitorError(
-                                    tunnel::ErrorKind::EnableIpv6Error,
-                                ) => BlockReason::Ipv6Unavailable,
+                                tunnel::ErrorKind::EnableIpv6Error => BlockReason::Ipv6Unavailable,
                                 _ => BlockReason::StartTunnelError,
                             };