From 14a004f84c01642dcb8b5efce2fd20ac8fd45bfa Mon Sep 17 00:00:00 2001 From: johannesd3 Date: Sat, 23 Jan 2021 22:37:41 +0100 Subject: [PATCH] Refactored Cache Proper error handling, and moving the conversion between { credentials, volume } and file into the cache module --- audio/src/fetch.rs | 4 +- core/src/authentication.rs | 29 +-------- core/src/cache.rs | 130 +++++++++++++++++++++++++------------ src/main.rs | 31 +++++---- 4 files changed, 112 insertions(+), 82 deletions(-) diff --git a/audio/src/fetch.rs b/audio/src/fetch.rs index c47cb4d3..c65482bd 100644 --- a/audio/src/fetch.rs +++ b/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); } diff --git a/core/src/authentication.rs b/core/src/authentication.rs index 36cbd439..9109c7fb 100644 --- a/core/src/authentication.rs +++ b/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(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>(path: P) -> Option { - File::open(path).ok().map(Credentials::from_reader) - } - - fn save_to_writer(&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>(&self, path: P) { - let mut file = File::create(path).unwrap(); - self.save_to_writer(&mut file) - } } fn serialize_protobuf_enum(v: &T, ser: S) -> Result diff --git a/core/src/cache.rs b/core/src/cache.rs index e711777c..4dcce968 100644 --- a/core/src/cache.rs +++ b/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 { + 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::open(self.system_root.join("credentials.json")) + } + + fn read_credentials(&self) -> io::Result { + 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 { - 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::open(self.system_root.join("volume")) } - pub fn volume(&self) -> Option { - let path = self.volume_path(); - Volume::from_file(path) + fn read_volume(&self) -> io::Result { + 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 { + 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::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(&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(()) } } diff --git a/src/main.rs b/src/main.rs index 4f80657e..8c029f5a 100644 --- a/src/main.rs +++ b/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")