From 22f8433d2289c36a6407b811472dae76080f5fa3 Mon Sep 17 00:00:00 2001 From: Christof Petig Date: Sun, 15 Oct 2023 23:34:05 +0200 Subject: [PATCH] address imbris suggestions in this new version --- common/assets/src/plugin_cache.rs | 72 +++++++++++++++---------------- common/assets/src/tar_source.rs | 22 +++++----- common/state/src/plugin/mod.rs | 4 +- 3 files changed, 49 insertions(+), 49 deletions(-) diff --git a/common/assets/src/plugin_cache.rs b/common/assets/src/plugin_cache.rs index 595eb9995b..261d17e981 100644 --- a/common/assets/src/plugin_cache.rs +++ b/common/assets/src/plugin_cache.rs @@ -36,30 +36,27 @@ impl CombinedSource { if let Ok(file_entry) = self.fs.raw_source().read(id, ext) { result.push((None, file_entry)); } - if let Ok(guard) = self.plugin_list.read() { - for (n, p) in guard.iter().enumerate() { - if let Ok(entry) = p.cache.raw_source().read(id, ext) { - // the data is behind an RwLockReadGuard, so own it for returning - result.push((Some(n), match entry { - FileContent::Slice(s) => FileContent::Buffer(Vec::from(s)), - FileContent::Buffer(b) => FileContent::Buffer(b), - FileContent::Owned(s) => { - FileContent::Buffer(Vec::from(s.as_ref().as_ref())) - }, - })); - } + for (n, p) in self.plugin_list.read().unwrap().iter().enumerate() { + if let Ok(entry) = p.cache.raw_source().read(id, ext) { + // the data is behind an RwLockReadGuard, so own it for returning + result.push((Some(n), match entry { + FileContent::Slice(s) => FileContent::Buffer(Vec::from(s)), + FileContent::Buffer(b) => FileContent::Buffer(b), + FileContent::Owned(s) => FileContent::Buffer(Vec::from(s.as_ref().as_ref())), + })); } } result } - // we don't want to keep the lock, so we clone + // We don't want to keep the lock, so we clone fn plugin_path(&self, index: Option) -> Option { if let Some(index) = index { self.plugin_list .read() - .ok() - .and_then(|p| p.get(index).map(|p| p.path.clone())) + .unwrap() + .get(index) + .map(|plugin| plugin.path.clone()) } else { None } @@ -68,7 +65,7 @@ impl CombinedSource { impl Source for CombinedSource { fn read(&self, id: &str, ext: &str) -> std::io::Result> { - // we could shortcut on fs if we dont want to check for conflicts + // We could shortcut on fs if we dont check for conflicts let mut entries = self.read_multiple(id, ext); if entries.is_empty() { Err(std::io::ErrorKind::NotFound.into()) @@ -89,7 +86,7 @@ impl Source for CombinedSource { id: &str, f: &mut dyn FnMut(assets_manager::source::DirEntry), ) -> std::io::Result<()> { - // TODO: we should combine the sources + // TODO: We should combine the sources, but this isn't used in veloren self.fs.raw_source().read_dir(id, f) } @@ -98,8 +95,9 @@ impl Source for CombinedSource { || self .plugin_list .read() - .map(|p| p.iter().any(|p| p.cache.raw_source().exists(entry))) - .unwrap_or_default() + .unwrap() + .iter() + .any(|plugin| plugin.cache.raw_source().exists(entry)) } // TODO: Enable hot reloading for plugins @@ -115,29 +113,29 @@ pub struct CombinedCache(AssetCache); impl CombinedCache { pub fn new() -> std::io::Result { - CombinedSource::new().map(|s| Self(AssetCache::with_source(s))) + CombinedSource::new().map(|combined_source| Self(AssetCache::with_source(combined_source))) } - /// combine objects from filesystem and plugins + /// Combine objects from filesystem and plugins pub fn combine( &self, load_from: impl Fn(AnyCache) -> Result, ) -> Result { let mut result = load_from(self.0.raw_source().fs.as_any_cache()); - // report a severe error from the filesystem asset even if later overwritten by + // Report a severe error from the filesystem asset even if later overwritten by // an Ok value from a plugin - if let Err(ref e) = result { - match e + if let Err(ref fs_error) = result { + match fs_error .source() - .and_then(|s| s.downcast_ref::()) - .map(|e| e.kind()) + .and_then(|error_source| error_source.downcast_ref::()) + .map(|io_error| io_error.kind()) { Some(std::io::ErrorKind::NotFound) => (), - _ => tracing::error!("Filesystem asset load {e:?}"), + _ => tracing::error!("Filesystem asset load {fs_error:?}"), } } - for i in self.0.raw_source().plugin_list.read().unwrap().iter() { - match load_from(i.cache.as_any_cache()) { + for plugin in self.0.raw_source().plugin_list.read().unwrap().iter() { + match load_from(plugin.cache.as_any_cache()) { Ok(b) => { result = if let Ok(a) = result { Ok(a.concatenate(b)) @@ -145,15 +143,18 @@ impl CombinedCache { Ok(b) }; }, - // report any error other than NotFound - Err(e) => { - match e + // Report any error other than NotFound + Err(plugin_error) => { + match plugin_error .source() - .and_then(|s| s.downcast_ref::()) - .map(|e| e.kind()) + .and_then(|error_source| error_source.downcast_ref::()) + .map(|io_error| io_error.kind()) { Some(std::io::ErrorKind::NotFound) => (), - _ => tracing::error!("Loading from {:?} failed {e:?}", i.path), + _ => tracing::error!( + "Loading from {:?} failed {plugin_error:?}", + plugin.path + ), } }, } @@ -163,7 +164,6 @@ impl CombinedCache { pub fn register_tar(&self, path: PathBuf) -> std::io::Result<()> { let tar_source = Tar::from_path(&path)?; - //println!("Tar {:?} {:?}", path, tar_source); let cache = AssetCache::with_source(tar_source); self.0 .raw_source() diff --git a/common/assets/src/tar_source.rs b/common/assets/src/tar_source.rs index 8e2c86e914..cf44e342eb 100644 --- a/common/assets/src/tar_source.rs +++ b/common/assets/src/tar_source.rs @@ -10,7 +10,7 @@ use std::{ path::{self, Path, PathBuf}, }; -// derived from the zip source in the assets_manager crate +// Derived from the zip source in the assets_manager crate #[derive(Clone, Hash, PartialEq, Eq)] struct FileDesc(String, String); @@ -82,7 +82,7 @@ fn register_file( match comp { path::Component::Normal(s) => { let segment = s.to_str()?; - // reject paths with extensions + // Reject paths with extensions if segment.contains('.') { return None; } @@ -91,7 +91,7 @@ fn register_file( } parent_id.push_str(segment); }, - // reject paths with non-name components + // Reject paths with non-name components _ => return None, } } @@ -107,11 +107,11 @@ fn register_file( })() .is_none(); if unsupported_path { - tracing::warn!("Unsupported path in tar archive: {path:?}"); + tracing::error!("Unsupported path in tar archive: {path:?}"); } } -// we avoid the extra dependency of sync_file introduced by Zip here by opening +// We avoid the extra dependency of sync_file introduced by Zip here by opening // the file for each read struct Backend(PathBuf); @@ -120,7 +120,7 @@ impl Backend { File::open(self.0.clone()).and_then(|file| { let mut result = vec![0; len]; file.read_at(result.as_mut_slice(), pos) - .map(move |_bytes| result) + .map(move |_num_bytes| result) }) } } @@ -141,12 +141,12 @@ impl Tar { .map_err(|e| io::Error::new(io::ErrorKind::InvalidData, e))?; let mut files = HashMap::with_capacity(contents.size_hint().0); let mut dirs = HashMap::new(); - for e in contents.flatten() { - if matches!(e.header().entry_type(), EntryType::Regular) { + for entry in contents.flatten() { + if matches!(entry.header().entry_type(), EntryType::Regular) { register_file( - e.path().map_err(io::Error::other)?.as_ref(), - e.raw_file_position(), - e.size() as usize, + entry.path().map_err(io::Error::other)?.as_ref(), + entry.raw_file_position(), + entry.size() as usize, &mut files, &mut dirs, ); diff --git a/common/state/src/plugin/mod.rs b/common/state/src/plugin/mod.rs index 0db8359221..ae25d31d60 100644 --- a/common/state/src/plugin/mod.rs +++ b/common/state/src/plugin/mod.rs @@ -197,11 +197,11 @@ impl PluginMgr { { info!("Loading plugin at {:?}", entry.path()); Plugin::from_reader(fs::File::open(entry.path()).map_err(PluginError::Io)?).map( - |o| { + |plugin| { if let Err(e) = common::assets::register_tar(entry.path()) { error!("Plugin {:?} tar error {e:?}", entry.path()); } - Some(o) + Some(plugin) }, ) } else {