Merge branch 'improve-linux-dns-errors'

1 files changed, 46 insertions, 66 deletions

diff --git a/talpid-core/src/firewall/linux/dns.rs b/talpid-core/src/firewall/linux/dns.rs
index 840ffd609e..a85e45cbfb 100644
--- a/talpid-core/src/firewall/linux/dns.rs
+++ b/talpid-core/src/firewall/linux/dns.rs
@@ -1,12 +1,10 @@
 extern crate notify;
 extern crate resolv_conf;
 
-use std::io::{self, Write};
 use std::net::IpAddr;
 use std::ops::DerefMut;
-use std::path::Path;
 use std::sync::{mpsc, Arc, Mutex, MutexGuard};
-use std::{fs, thread};
+use std::{fs, io, thread};
 
 use error_chain::ChainedError;
 
@@ -16,35 +14,23 @@ use self::resolv_conf::{Config, ScopedIp};
 const RESOLV_CONF_PATH: &str = "/etc/resolv.conf";
 const RESOLV_CONF_BACKUP_PATH: &str = "/etc/resolv.conf.mullvadbackup";
 
-error_chain!{
+error_chain! {
     errors {
-        BackupResolvConf {
-            description("Failed to create backup of /etc/resolv.conf")
-        }
-
-        ParseResolvConf {
-            description("Failed to parse contents of /etc/resolv.conf")
+        WatchResolvConf {
+            description("Failed to watch /etc/resolv.conf for changes")
         }
 
-        ReadResolvConf {
-            description("Failed to read /etc/resolv.conf")
+        WriteResolvConf {
+            description("Failed to write to /etc/resolv.conf")
         }
 
-        RemoveBackup {
-            description("Failed to remove stale backup of /etc/resolv.conf")
+        BackupResolvConf {
+            description("Failed to create backup of /etc/resolv.conf")
         }
 
         RestoreResolvConf {
             description("Failed to restore /etc/resolv.conf from backup")
         }
-
-        WatchResolvConf {
-            description("Failed to watch /etc/resolv.conf for changes")
-        }
-
-        WriteResolvConf {
-            description("Failed to write to /etc/resolv.conf")
-        }
     }
 }
 
@@ -55,7 +41,7 @@ pub struct DnsSettings {
 
 impl DnsSettings {
     pub fn new() -> Result<Self> {
-        Self::restore_persisted_state()?;
+        restore_from_backup().chain_err(|| ErrorKind::RestoreResolvConf)?;
 
         let state = Arc::new(Mutex::new(None));
         let watcher = DnsWatcher::start(state.clone())?;
@@ -69,10 +55,15 @@ impl DnsSettings {
     pub fn set_dns(&mut self, servers: Vec<IpAddr>) -> Result<()> {
         let mut state = self.lock_state();
         let new_state = match state.take() {
-            None => State {
-                backup: backup_config()?,
-                desired_dns: servers,
-            },
+            None => {
+                let backup = read_config().chain_err(|| ErrorKind::BackupResolvConf)?;
+                write_backup(&backup).chain_err(|| ErrorKind::BackupResolvConf)?;
+
+                State {
+                    backup,
+                    desired_dns: servers,
+                }
+            }
             Some(previous_state) => State {
                 backup: previous_state.backup,
                 desired_dns: servers,
@@ -100,31 +91,6 @@ impl DnsSettings {
             .lock()
             .expect("a thread panicked while using the DNS configuration state")
     }
-
-    fn restore_persisted_state() -> Result<()> {
-        let backup_file = Path::new(RESOLV_CONF_BACKUP_PATH);
-
-        match fs::read(&backup_file) {
-            Ok(backup) => {
-                info!("Restoring DNS state from backup");
-                let mut conf_file =
-                    fs::File::create(RESOLV_CONF_PATH).chain_err(|| ErrorKind::RestoreResolvConf)?;
-                conf_file
-                    .write_all(&backup)
-                    .chain_err(|| ErrorKind::RestoreResolvConf)?;
-                conf_file
-                    .sync_all()
-                    .chain_err(|| ErrorKind::RestoreResolvConf)?;
-                fs::remove_file(&backup_file).chain_err(|| ErrorKind::RemoveBackup)?;
-            }
-            Err(ref error) if error.kind() == io::ErrorKind::NotFound => {
-                trace!("No DNS state backup to restore")
-            }
-            Err(error) => return Err(Error::with_chain(error, ErrorKind::RestoreResolvConf)),
-        }
-
-        Ok(())
-    }
 }
 
 struct State {
@@ -197,7 +163,7 @@ impl DnsWatcher {
                 new_config.nameservers.append(&mut state.backup.nameservers);
                 state.backup = new_config;
 
-                update_backup(&state.backup)
+                write_backup(&state.backup).chain_err(|| "Failed to update /etc/resolv.conf backup")
             }
         } else {
             Ok(())
@@ -206,8 +172,10 @@ impl DnsWatcher {
 }
 
 fn read_config() -> Result<Config> {
-    let contents = fs::read_to_string(RESOLV_CONF_PATH).chain_err(|| ErrorKind::ReadResolvConf)?;
-    let config = Config::parse(&contents).chain_err(|| ErrorKind::ParseResolvConf)?;
+    let contents =
+        fs::read_to_string(RESOLV_CONF_PATH).chain_err(|| "Failed to read /etc/resolv.conf")?;
+    let config =
+        Config::parse(&contents).chain_err(|| "Failed to parse contents of /etc/resolv.conf")?;
 
     Ok(config)
 }
@@ -217,18 +185,30 @@ fn write_config(config: &Config) -> Result<()> {
         .chain_err(|| ErrorKind::WriteResolvConf)
 }
 
-fn backup_config() -> Result<Config> {
-    let contents = fs::read_to_string(RESOLV_CONF_PATH).chain_err(|| ErrorKind::ReadResolvConf)?;
-
-    fs::write(RESOLV_CONF_BACKUP_PATH, contents.as_bytes())
-        .chain_err(|| ErrorKind::BackupResolvConf)?;
+fn write_backup(backup: &Config) -> Result<()> {
+    fs::write(RESOLV_CONF_BACKUP_PATH, backup.to_string().as_bytes())
+        .chain_err(|| "Failed to write to /etc/resolv.conf backup file")
+}
 
-    let config = Config::parse(&contents).chain_err(|| ErrorKind::ParseResolvConf)?;
+fn restore_from_backup() -> Result<()> {
+    match fs::read_to_string(RESOLV_CONF_BACKUP_PATH) {
+        Ok(backup) => {
+            info!("Restoring DNS state from backup");
+            let config = Config::parse(&backup)
+                .chain_err(|| "Backup of /etc/resolv.conf could not be parsed")?;
 
-    Ok(config)
-}
+            write_config(&config)?;
 
-fn update_backup(backup: &Config) -> Result<()> {
-    fs::write(RESOLV_CONF_BACKUP_PATH, backup.to_string().as_bytes())
-        .chain_err(|| ErrorKind::BackupResolvConf)
+            fs::remove_file(RESOLV_CONF_BACKUP_PATH)
+                .chain_err(|| "Failed to remove stale backup of /etc/resolv.conf")
+        }
+        Err(ref error) if error.kind() == io::ErrorKind::NotFound => {
+            debug!("No DNS state backup to restore");
+            Ok(())
+        }
+        Err(error) => Err(Error::with_chain(
+            error,
+            "Failed to read /etc/resolv.conf backup",
+        )),
+    }
 }