Refactored Cache

Proper error handling, and moving the conversion between
{ credentials, volume } and file into the cache module

audio/src/fetch.rs

@@ -429,8 +429,10 @@ impl AudioFile {
             complete_rx
                 .map(move |mut file| {
                     if let Some(cache) = session_.cache() {
-                        cache.save_file(file_id, &mut file);
                         debug!("File {} complete, saving to cache", file_id);
+                        if let Err(err) = cache.save_file(file_id, &mut file) {
+                            warn!("Cannot save file to cache: {}", err);
+                        }
                     } else {
                         debug!("File {} complete", file_id);
                     }

core/src/authentication.rs

@@ -1,16 +1,10 @@
 use aes::Aes192;
-use base64;
 use byteorder::{BigEndian, ByteOrder};
 use hmac::Hmac;
 use pbkdf2::pbkdf2;
 use protobuf::ProtobufEnum;
-use serde;
-use serde_json;
 use sha1::{Digest, Sha1};
-use std::fs::File;
-use std::io::{self, Read, Write};
-use std::ops::FnOnce;
-use std::path::Path;
+use std::io::{self, Read};
 
 use crate::protocol::authentication::AuthenticationType;
 
@@ -112,27 +106,6 @@ impl Credentials {
             auth_data: auth_data,
         }
     }
-
-    fn from_reader<R: Read>(mut reader: R) -> Credentials {
-        let mut contents = String::new();
-        reader.read_to_string(&mut contents).unwrap();
-
-        serde_json::from_str(&contents).unwrap()
-    }
-
-    pub(crate) fn from_file<P: AsRef<Path>>(path: P) -> Option<Credentials> {
-        File::open(path).ok().map(Credentials::from_reader)
-    }
-
-    fn save_to_writer<W: Write>(&self, writer: &mut W) {
-        let contents = serde_json::to_string(&self.clone()).unwrap();
-        writer.write_all(contents.as_bytes()).unwrap();
-    }
-
-    pub(crate) fn save_to_file<P: AsRef<Path>>(&self, path: P) {
-        let mut file = File::create(path).unwrap();
-        self.save_to_writer(&mut file)
-    }
 }
 
 fn serialize_protobuf_enum<T, S>(v: &T, ser: S) -> Result<S::Ok, S::Error>

core/src/cache.rs

@@ -1,9 +1,7 @@
 use std::fs;
 use std::fs::File;
-use std::io;
-use std::io::Read;
-use std::path::Path;
-use std::path::PathBuf;
+use std::io::{self, Error, ErrorKind, Read, Write};
+use std::path::{Path, PathBuf};
 
 use crate::authentication::Credentials;
 use crate::spotify_id::FileId;
@@ -27,51 +25,93 @@ fn mkdir_existing(path: &Path) -> io::Result<()> {
 }
 
 impl Cache {
-    pub fn new(audio_location: PathBuf, system_location: PathBuf, use_audio_cache: bool) -> Cache {
-        if use_audio_cache == true {
-            mkdir_existing(&audio_location).unwrap();
-            mkdir_existing(&audio_location.join("files")).unwrap();
+    pub fn new(
+        audio_location: PathBuf,
+        system_location: PathBuf,
+        use_audio_cache: bool,
+    ) -> io::Result<Cache> {
+        if use_audio_cache {
+            mkdir_existing(&audio_location)?;
+            mkdir_existing(&audio_location.join("files"))?;
         }
-        mkdir_existing(&system_location).unwrap();
+        mkdir_existing(&system_location)?;
 
-        Cache {
+        Ok(Cache {
             audio_root: audio_location,
             system_root: system_location,
-            use_audio_cache: use_audio_cache,
-        }
+            use_audio_cache,
+        })
     }
 }
 
 impl Cache {
-    fn credentials_path(&self) -> PathBuf {
-        self.system_root.join("credentials.json")
+    fn open_credentials_file(&self) -> io::Result<File> {
+        File::open(self.system_root.join("credentials.json"))
+    }
+
+    fn read_credentials(&self) -> io::Result<Credentials> {
+        let mut file = self.open_credentials_file()?;
+        let mut contents = String::new();
+        file.read_to_string(&mut contents)?;
+        serde_json::from_str(&contents).map_err(|e| Error::new(ErrorKind::InvalidData, e))
     }
 
     pub fn credentials(&self) -> Option<Credentials> {
-        let path = self.credentials_path();
-        Credentials::from_file(path)
+        match self.read_credentials() {
+            Ok(c) => Some(c),
+            Err(e) => {
+                if e.kind() != ErrorKind::NotFound {
+                    warn!("Error reading credentials from cache: {}", e);
+                }
+                None
+            }
+        }
     }
 
     pub fn save_credentials(&self, cred: &Credentials) {
-        let path = self.credentials_path();
-        cred.save_to_file(&path);
+        let result = self
+            .open_credentials_file()
+            .and_then(|mut file| write!(file, "{}", serde_json::to_string(cred)?));
+        if let Err(e) = result {
+            warn!("Cannot save credentials to cache: {}", e);
+        }
     }
 }
 
 // cache volume to system_root/volume
 impl Cache {
-    fn volume_path(&self) -> PathBuf {
-        self.system_root.join("volume")
+    fn open_volume_file(&self) -> io::Result<File> {
+        File::open(self.system_root.join("volume"))
     }
 
-    pub fn volume(&self) -> Option<u16> {
-        let path = self.volume_path();
-        Volume::from_file(path)
+    fn read_volume(&self) -> io::Result<Volume> {
+        let mut file = self.open_volume_file()?;
+        let mut contents = String::new();
+        file.read_to_string(&mut contents)?;
+        contents
+            .parse()
+            .map_err(|e| Error::new(ErrorKind::InvalidData, e))
+    }
+
+    pub fn volume(&self) -> Option<Volume> {
+        match self.read_volume() {
+            Ok(v) => Some(v),
+            Err(e) => {
+                if e.kind() != ErrorKind::NotFound {
+                    warn!("Error reading volume from cache: {}", e);
+                }
+                None
+            }
+        }
     }
 
     pub fn save_volume(&self, volume: Volume) {
-        let path = self.volume_path();
-        volume.save_to_file(&path);
+        let result = self
+            .open_volume_file()
+            .and_then(|mut file| write!(file, "{}", volume));
+        if let Err(e) = result {
+            warn!("Cannot save volume to cache: {}", e);
+        }
     }
 }
 
@@ -85,31 +125,39 @@ impl Cache {
     }
 
     pub fn file(&self, file: FileId) -> Option<File> {
-        File::open(self.file_path(file)).ok()
+        File::open(self.file_path(file))
+            .map_err(|e| {
+                if e.kind() != ErrorKind::NotFound {
+                    warn!("Error reading file from cache: {}", e)
+                }
+            })
+            .ok()
     }
 
-    pub fn save_file(&self, file: FileId, contents: &mut dyn Read) {
+    pub fn save_file<F: Read>(&self, file: FileId, contents: &mut F) -> io::Result<()> {
         if self.use_audio_cache {
             let path = self.file_path(file);
-            mkdir_existing(path.parent().unwrap()).unwrap();
+            mkdir_existing(path.parent().unwrap())?;
 
-            let mut cache_file = File::create(path).unwrap_or_else(|_e| {
-                ::std::fs::remove_dir_all(&self.audio_root.join("files")).unwrap();
-                mkdir_existing(&self.audio_root.join("files")).unwrap();
+            let mut cache_file = File::create(path).or_else(|_| {
+                fs::remove_dir_all(&self.audio_root.join("files"))?;
+                mkdir_existing(&self.audio_root.join("files"))?;
 
                 let path = self.file_path(file);
-                mkdir_existing(path.parent().unwrap()).unwrap();
-                File::create(path).unwrap()
-            });
-            ::std::io::copy(contents, &mut cache_file).unwrap_or_else(|_e| {
-                ::std::fs::remove_dir_all(&self.audio_root.join("files")).unwrap();
-                mkdir_existing(&self.audio_root.join("files")).unwrap();
+                mkdir_existing(path.parent().unwrap())?;
+                File::create(path)
+            })?;
+
+            io::copy(contents, &mut cache_file).or_else(|_| {
+                fs::remove_dir_all(&self.audio_root.join("files"))?;
+                mkdir_existing(&self.audio_root.join("files"))?;
 
                 let path = self.file_path(file);
-                mkdir_existing(path.parent().unwrap()).unwrap();
-                let mut file = File::create(path).unwrap();
-                ::std::io::copy(contents, &mut file).unwrap()
-            });
+                mkdir_existing(path.parent().unwrap())?;
+                let mut file = File::create(path)?;
+                io::copy(contents, &mut file)
+            })?;
         }
+        Ok(())
     }
 }

src/main.rs

@@ -254,18 +254,24 @@ fn setup(args: &[String]) -> Setup {
         mapped_volume: !matches.opt_present("mixer-linear-volume"),
     };
 
-    let cache = matches.opt_str("c").map(|cache_path| {
-        let use_audio_cache = !matches.opt_present("disable-audio-cache");
-        let system_cache_directory = matches
-            .opt_str("system-cache")
-            .unwrap_or(String::from(cache_path.clone()));
+    let use_audio_cache = !matches.opt_present("disable-audio-cache");
 
-        Cache::new(
-            PathBuf::from(cache_path),
-            PathBuf::from(system_cache_directory),
-            use_audio_cache,
-        )
-    });
+    let cache_directory = matches.opt_str("c").unwrap_or(String::from(""));
+    let system_cache_directory = matches
+        .opt_str("system-cache")
+        .unwrap_or(String::from(cache_directory.clone()));
+
+    let cache = match Cache::new(
+        PathBuf::from(cache_directory),
+        PathBuf::from(system_cache_directory),
+        use_audio_cache,
+    ) {
+        Ok(cache) => Some(cache),
+        Err(e) => {
+            warn!("Cannot create cache: {}", e);
+            None
+        }
+    };
 
     let initial_volume = matches
         .opt_str("initial-volume")
@@ -276,8 +282,9 @@ fn setup(args: &[String]) -> Setup {
             }
             (volume as i32 * 0xFFFF / 100) as u16
         })
+        .map(Volume)
         .or_else(|| cache.as_ref().and_then(Cache::volume))
-        .unwrap_or(0x8000);
+        .unwrap_or(Volume(0x8000));
 
     let zeroconf_port = matches
         .opt_str("zeroconf-port")