From 321be8555fe194d2e39a9f07342186fb23fb53a1 Mon Sep 17 00:00:00 2001 From: Manos Pitsidianakis Date: Sat, 16 Nov 2019 00:33:22 +0200 Subject: [PATCH] Cleanup startup error exit paths Make startup methods return Results so that the main binary can exit cleanly instead of using std::process::exit from arbitrary positions, which exits the process immediately and doesn't run destructors. --- build.rs | 77 --------------------------- melib/src/backends.rs | 16 +++--- melib/src/backends/imap.rs | 41 +++++++------- melib/src/backends/maildir/backend.rs | 61 ++++++++++----------- melib/src/backends/mbox.rs | 11 ++-- melib/src/backends/notmuch.rs | 16 +++--- src/bin.rs | 40 +++++++++----- text_processing/build.rs | 4 +- ui/src/conf.rs | 23 ++++---- ui/src/conf/accounts.rs | 8 +-- ui/src/state.rs | 29 +++++----- 11 files changed, 131 insertions(+), 195 deletions(-) delete mode 100644 build.rs diff --git a/build.rs b/build.rs deleted file mode 100644 index 6907b0d3..00000000 --- a/build.rs +++ /dev/null @@ -1,77 +0,0 @@ -/* - * meli - bin.rs - * - * Copyright 2019 Manos Pitsidianakis - * - * This file is part of meli. - * - * meli is free software: you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * meli is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with meli. If not, see . - */ -use std::fs::File; -use std::io::prelude::*; -use std::io::BufWriter; -use std::path::PathBuf; -use std::process::Command; - -fn main() -> Result<(), std::io::Error> { - if let Err(e) = std::fs::create_dir("src/manuals") { - if e.kind() != std::io::ErrorKind::AlreadyExists { - Err(e)?; - } - } - let mut build_flag = false; - let meli_1_metadata = std::fs::metadata("meli.1")?; - if let Ok(metadata) = std::fs::metadata("src/manuals/meli.txt") { - if metadata.modified()? < meli_1_metadata.modified()? { - build_flag = true; - } - } else { - /* Doesn't exist */ - build_flag = true; - } - - if build_flag { - let output = if let Ok(output) = Command::new("mandoc").args(&["meli.1"]).output() { - output.stdout - } else { - b"mandoc was not found on your system. It's required in order to compile the manual pages into plain text for use with the --*manual command line flags.".to_vec() - }; - let man_path = PathBuf::from("src/manuals/meli.txt"); - let file = File::create(&man_path)?; - BufWriter::new(file).write_all(&output)?; - } - - let mut build_flag = false; - let meli_conf_5_metadata = std::fs::metadata("meli.conf.5")?; - if let Ok(metadata) = std::fs::metadata("src/manuals/meli_conf.txt") { - if metadata.modified()? < meli_conf_5_metadata.modified()? { - build_flag = true; - } - } else { - /* Doesn't exist */ - build_flag = true; - } - - if build_flag { - let output = if let Ok(output) = Command::new("mandoc").args(&["meli.conf.5"]).output() { - output.stdout - } else { - b"mandoc was not found on your system. It's required in order to compile the manual pages into plain text for use with the --*manual command line flags.".to_vec() - }; - let man_path = PathBuf::from("src/manuals/meli_conf.txt"); - let file = File::create(&man_path)?; - BufWriter::new(file).write_all(&output)?; - } - Ok(()) -} diff --git a/melib/src/backends.rs b/melib/src/backends.rs index b7532eee..d81e056c 100644 --- a/melib/src/backends.rs +++ b/melib/src/backends.rs @@ -48,8 +48,12 @@ use std::ops::Deref; use fnv::FnvHashMap; use std; -pub type BackendCreator = - Box bool + Send + Sync>) -> Box>; +pub type BackendCreator = Box< + dyn Fn( + &AccountSettings, + Box bool + Send + Sync>, + ) -> Result>, +>; /// A hashmap containing all available mail backends. /// An abstraction over any available backends. @@ -72,28 +76,28 @@ impl Backends { { b.register( "maildir".to_string(), - Box::new(|| Box::new(|f, i| Box::new(MaildirType::new(f, i)))), + Box::new(|| Box::new(|f, i| MaildirType::new(f, i))), ); } #[cfg(feature = "mbox_backend")] { b.register( "mbox".to_string(), - Box::new(|| Box::new(|f, i| Box::new(MboxType::new(f, i)))), + Box::new(|| Box::new(|f, i| MboxType::new(f, i))), ); } #[cfg(feature = "imap_backend")] { b.register( "imap".to_string(), - Box::new(|| Box::new(|f, i| Box::new(ImapType::new(f, i)))), + Box::new(|| Box::new(|f, i| ImapType::new(f, i))), ); } #[cfg(feature = "notmuch_backend")] { b.register( "notmuch".to_string(), - Box::new(|| Box::new(|f, i| Box::new(NotmuchDb::new(f, i)))), + Box::new(|| Box::new(|f, i| NotmuchDb::new(f, i))), ); } b diff --git a/melib/src/backends/imap.rs b/melib/src/backends/imap.rs index d67764ca..ac1a50f2 100644 --- a/melib/src/backends/imap.rs +++ b/melib/src/backends/imap.rs @@ -112,7 +112,8 @@ impl MailBackend for ImapType { ($tx:expr,$($result:expr)+) => { $(if let Err(e) = $result { $tx.send(AsyncStatus::Payload(Err(e.into()))).unwrap(); - std::process::exit(1); + $tx.send(AsyncStatus::Finished).unwrap(); + return; })+ }; }; @@ -411,30 +412,29 @@ impl MailBackend for ImapType { macro_rules! get_conf_val { ($s:ident[$var:literal]) => { - $s.extra.get($var).unwrap_or_else(|| { - eprintln!( + $s.extra.get($var).ok_or_else(|| { + MeliError::new(format!( "Configuration error ({}): IMAP connection requires the field `{}` set", $s.name.as_str(), $var - ); - std::process::exit(1); + )) }) }; ($s:ident[$var:literal], $default:expr) => { $s.extra .get($var) .map(|v| { - <_>::from_str(v).unwrap_or_else(|_| { - eprintln!( - "Configuration error ({}): Invalid value for field `{}`: {}", + <_>::from_str(v).map_err(|e| { + MeliError::new(format!( + "Configuration error ({}): Invalid value for field `{}`: {}\n{}", $s.name.as_str(), $var, v, - ); - std::process::exit(1); + e + )) }) }) - .unwrap_or_else(|| $default) + .unwrap_or_else(|| Ok($default)) }; } @@ -442,21 +442,20 @@ impl ImapType { pub fn new( s: &AccountSettings, is_subscribed: Box bool + Send + Sync>, - ) -> Self { - debug!(s); - let server_hostname = get_conf_val!(s["server_hostname"]); - let server_username = get_conf_val!(s["server_username"]); - let server_password = get_conf_val!(s["server_password"]); - let server_port = get_conf_val!(s["server_port"], 143); + ) -> Result> { + let server_hostname = get_conf_val!(s["server_hostname"])?; + let server_username = get_conf_val!(s["server_username"])?; + let server_password = get_conf_val!(s["server_password"])?; + let server_port = get_conf_val!(s["server_port"], 143)?; let use_starttls = get_conf_val!(s["use_starttls"], { if server_port == 993 { false } else { true } - }); + })?; let danger_accept_invalid_certs: bool = - get_conf_val!(s["danger_accept_invalid_certs"], false); + get_conf_val!(s["danger_accept_invalid_certs"], false)?; let server_conf = ImapServerConf { server_hostname: server_hostname.to_string(), server_username: server_username.to_string(), @@ -467,7 +466,7 @@ impl ImapType { }; let connection = ImapConnection::new_connection(&server_conf); - ImapType { + Ok(Box::new(ImapType { account_name: s.name().to_string(), online: Arc::new(Mutex::new(false)), server_conf, @@ -481,7 +480,7 @@ impl ImapType { uid_index: Default::default(), byte_cache: Default::default(), }), - } + })) } pub fn shell(&mut self) { diff --git a/melib/src/backends/maildir/backend.rs b/melib/src/backends/maildir/backend.rs index 1a02e3a6..2464c4be 100644 --- a/melib/src/backends/maildir/backend.rs +++ b/melib/src/backends/maildir/backend.rs @@ -509,15 +509,18 @@ impl MailBackend for MaildirType { } impl MaildirType { - pub fn new(settings: &AccountSettings, is_subscribed: Box bool>) -> Self { + pub fn new( + settings: &AccountSettings, + is_subscribed: Box bool>, + ) -> Result> { let mut folders: FnvHashMap = Default::default(); fn recurse_folders>( folders: &mut FnvHashMap, settings: &AccountSettings, p: P, - ) -> Vec { + ) -> Result> { if !p.as_ref().exists() || !p.as_ref().is_dir() { - eprintln!( + return Err(MeliError::new(format!( "Configuration error: Path \"{}\" {}", p.as_ref().display(), if !p.as_ref().exists() { @@ -525,8 +528,7 @@ impl MaildirType { } else { "is not a directory." } - ); - std::process::exit(1); + ))); } let mut children = Vec::new(); for mut f in fs::read_dir(p).unwrap() { @@ -544,7 +546,7 @@ impl MaildirType { Vec::new(), &settings, ) { - f.children = recurse_folders(folders, settings, &path); + f.children = recurse_folders(folders, settings, &path)?; f.children .iter() .map(|c| folders.get_mut(c).map(|f| f.parent = Some(f.hash))) @@ -556,23 +558,21 @@ impl MaildirType { } } } - children + Ok(children) }; let root_path = PathBuf::from(settings.root_folder()).expand(); if !root_path.exists() { - eprintln!( + return Err(MeliError::new(format!( "Configuration error ({}): root_path `{}` is not a valid directory.", settings.name(), settings.root_folder.as_str() - ); - std::process::exit(1); + ))); } else if !root_path.is_dir() { - eprintln!( + return Err(MeliError::new(format!( "Configuration error ({}): root_path `{}` is not a directory.", settings.name(), settings.root_folder.as_str() - ); - std::process::exit(1); + ))); } if let Ok(f) = MaildirFolder::new( @@ -586,14 +586,14 @@ impl MaildirType { } if folders.is_empty() { - let children = recurse_folders(&mut folders, settings, &root_path); + let children = recurse_folders(&mut folders, settings, &root_path)?; children .iter() .map(|c| folders.get_mut(c).map(|f| f.parent = None)) .count(); } else { let root_hash = *folders.keys().nth(0).unwrap(); - let children = recurse_folders(&mut folders, settings, &root_path); + let children = recurse_folders(&mut folders, settings, &root_path)?; children .iter() .map(|c| folders.get_mut(c).map(|f| f.parent = Some(root_hash))) @@ -606,29 +606,24 @@ impl MaildirType { f.children.retain(|c| keys.contains(c)); } - let hash_indexes = Arc::new(Mutex::new(FnvHashMap::with_capacity_and_hasher( - folders.len(), - Default::default(), - ))); - { - let mut hash_indexes = hash_indexes.lock().unwrap(); - for &fh in folders.keys() { - hash_indexes.insert( - fh, - HashIndex { - index: FnvHashMap::with_capacity_and_hasher(0, Default::default()), - hash: fh, - }, - ); - } + let mut hash_indexes = + FnvHashMap::with_capacity_and_hasher(folders.len(), Default::default()); + for &fh in folders.keys() { + hash_indexes.insert( + fh, + HashIndex { + index: FnvHashMap::with_capacity_and_hasher(0, Default::default()), + hash: fh, + }, + ); } - MaildirType { + Ok(Box::new(MaildirType { name: settings.name().to_string(), folders, - hash_indexes, + hash_indexes: Arc::new(Mutex::new(hash_indexes)), folder_index: Default::default(), path: root_path, - } + })) } fn owned_folder_idx(&self, folder: &Folder) -> FolderHash { *self diff --git a/melib/src/backends/mbox.rs b/melib/src/backends/mbox.rs index 208e9ed2..5b99f255 100644 --- a/melib/src/backends/mbox.rs +++ b/melib/src/backends/mbox.rs @@ -562,14 +562,17 @@ impl MailBackend for MboxType { } impl MboxType { - pub fn new(s: &AccountSettings, _is_subscribed: Box bool>) -> Self { + pub fn new( + s: &AccountSettings, + _is_subscribed: Box bool>, + ) -> Result> { let path = Path::new(s.root_folder.as_str()).expand(); if !path.exists() { - panic!( + return Err(MeliError::new(format!( "\"root_folder\" {} for account {} is not a valid path.", s.root_folder.as_str(), s.name() - ); + ))); } let ret = MboxType { path, @@ -640,6 +643,6 @@ impl MboxType { } } */ - ret + Ok(Box::new(ret)) } } diff --git a/melib/src/backends/notmuch.rs b/melib/src/backends/notmuch.rs index 0daa05bc..530ec0d8 100644 --- a/melib/src/backends/notmuch.rs +++ b/melib/src/backends/notmuch.rs @@ -104,7 +104,10 @@ unsafe impl Send for NotmuchFolder {} unsafe impl Sync for NotmuchFolder {} impl NotmuchDb { - pub fn new(s: &AccountSettings, _is_subscribed: Box bool>) -> Self { + pub fn new( + s: &AccountSettings, + _is_subscribed: Box bool>, + ) -> Result> { let mut database: *mut notmuch_database_t = std::ptr::null_mut(); let path = Path::new(s.root_folder.as_str()).expand().to_path_buf(); if !path.exists() { @@ -125,7 +128,7 @@ impl NotmuchDb { ) }; if status != 0 { - panic!("notmuch_database_open returned {}.", status); + return Err(MeliError::new(format!("Could not open notmuch database at path {}. notmuch_database_open returned {}.", s.root_folder.as_str(), status); } assert!(!database.is_null()); let mut folders = FnvHashMap::default(); @@ -150,14 +153,13 @@ impl NotmuchDb { }, ); } else { - eprintln!( + return Err(MeliError::new(format!( "notmuch folder configuration entry \"{}\" should have a \"query\" value set.", k - ); - std::process::exit(1); + ))); } } - NotmuchDb { + Ok(Box::new(NotmuchDb { database: DbWrapper { inner: Arc::new(RwLock::new(database)), database_ph: std::marker::PhantomData, @@ -166,7 +168,7 @@ impl NotmuchDb { index: Arc::new(RwLock::new(Default::default())), folders: Arc::new(RwLock::new(folders)), save_messages_to: None, - } + })) } } diff --git a/src/bin.rs b/src/bin.rs index 158e79b9..e3e58af6 100644 --- a/src/bin.rs +++ b/src/bin.rs @@ -70,8 +70,7 @@ fn notify( macro_rules! error_and_exit { ($($err:expr),*) => {{ - eprintln!($($err),*); - std::process::exit(1); + return Err(MeliError::new(format!($($err),*))); }} } @@ -83,7 +82,17 @@ struct CommandLineArguments { version: bool, } -fn main() -> std::result::Result<(), std::io::Error> { +fn main() { + ::std::process::exit(match run_app() { + Ok(()) => 0, + Err(err) => { + eprintln!("{}", err); + 1 + } + }); +} + +fn run_app() -> Result<()> { enum CommandLineFlags { CreateConfig, Config, @@ -144,12 +153,12 @@ fn main() -> std::result::Result<(), std::io::Error> { println!("\t--version, -v\t\tprint version and exit"); println!("\t--create-config[ PATH]\tCreate a sample configuration file with available configuration options. If PATH is not specified, meli will try to create it in $XDG_CONFIG_HOME/meli/config"); println!("\t--config PATH, -c PATH\tUse specified configuration file"); - std::process::exit(0); + return Ok(()); } if args.version { println!("meli {}", option_env!("CARGO_PKG_VERSION").unwrap_or("0.0")); - std::process::exit(0); + return Ok(()); } match prev { @@ -162,25 +171,28 @@ fn main() -> std::result::Result<(), std::io::Error> { if let Some(config_path) = args.create_config.as_mut() { let config_path: PathBuf = if config_path.is_empty() { let xdg_dirs = xdg::BaseDirectories::with_prefix("meli").unwrap(); - xdg_dirs.place_config_file("config").unwrap_or_else(|e| { - error_and_exit!("Cannot create configuration directory:\n{}", e) - }) + xdg_dirs.place_config_file("config").map_err(|e| { + MeliError::new(format!( + "Cannot create configuration directory in {}:\n{}", + xdg_dirs.get_config_home().display(), + e + )) + })? } else { Path::new(config_path).to_path_buf() }; if config_path.exists() { - println!("File `{}` already exists.\nMaybe you meant to specify another path with --create-config=PATH", config_path.display()); - std::process::exit(1); + return Err(MeliError::new(format!("File `{}` already exists.\nMaybe you meant to specify another path with --create-config=PATH", config_path.display()))); } let mut file = std::fs::OpenOptions::new() .write(true) .create_new(true) .open(config_path.as_path()) - .unwrap_or_else(|e| error_and_exit!("Could not create config file:\n{}", e)); + .map_err(|e| MeliError::new(format!("Could not create config file:\n{}", e)))?; file.write_all(include_bytes!("../sample-config")) - .unwrap_or_else(|e| error_and_exit!("Could not write to config file:\n{}", e)); + .map_err(|e| MeliError::new(format!("Could not write to config file:\n{}", e)))?; println!("Written example configuration to {}", config_path.display()); - std::process::exit(0); + return Ok(()); } if let Some(config_location) = args.config.as_ref() { @@ -188,7 +200,7 @@ fn main() -> std::result::Result<(), std::io::Error> { } /* Create the application State. */ - let mut state = State::new(); + let mut state = State::new()?; let receiver = state.receiver(); let sender = state.sender(); diff --git a/text_processing/build.rs b/text_processing/build.rs index d740676b..70edcd70 100644 --- a/text_processing/build.rs +++ b/text_processing/build.rs @@ -2,13 +2,13 @@ const LINE_BREAK_TABLE_URL: &str = "http://www.unicode.org/Public/UCD/latest/ucd use std::fs::File; use std::io::prelude::*; use std::io::BufReader; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::process::Command; include!("src/types.rs"); fn main() -> Result<(), std::io::Error> { - let mod_path = PathBuf::from("src/tables.rs"); + let mod_path = Path::new("src/tables.rs"); if mod_path.exists() { eprintln!( "{} already exists, delete it if you want to replace it.", diff --git a/ui/src/conf.rs b/ui/src/conf.rs index 79c9b113..184c706e 100644 --- a/ui/src/conf.rs +++ b/ui/src/conf.rs @@ -312,10 +312,12 @@ impl FileSettings { file.write_all(include_bytes!("../../sample-config")) .expect("Could not write to config file."); println!("Written config to {}", config_path.display()); - std::process::exit(1); + return Err(MeliError::new( + "Edit the sample configuration and relaunch meli.", + )); } "n" | "N" | "no" | "No" | "NO" => { - std::process::exit(1); + return Err(MeliError::new("No configuration file found.")); } _ => { println!( @@ -332,8 +334,10 @@ impl FileSettings { file.read_to_string(&mut contents)?; let s = toml::from_str(&contents); if let Err(e) = s { - eprintln!("Config file contains errors: {}", e.to_string()); - std::process::exit(1); + return Err(MeliError::new(format!( + "Config file contains errors: {}", + e.to_string() + ))); } Ok(s.unwrap()) @@ -341,11 +345,8 @@ impl FileSettings { } impl Settings { - pub fn new() -> Settings { - let fs = FileSettings::new().unwrap_or_else(|e| { - eprintln!("Configuration error: {}", e); - std::process::exit(1); - }); + pub fn new() -> Result { + let fs = FileSettings::new()?; let mut s: HashMap = HashMap::new(); for (id, x) in fs.accounts { @@ -355,7 +356,7 @@ impl Settings { s.insert(id, ac); } - Settings { + Ok(Settings { accounts: s, pager: fs.pager, notifications: fs.notifications, @@ -363,7 +364,7 @@ impl Settings { composing: fs.composing, pgp: fs.pgp, terminal: fs.terminal, - } + }) } } diff --git a/ui/src/conf/accounts.rs b/ui/src/conf/accounts.rs index 4709b2f8..35869b59 100644 --- a/ui/src/conf/accounts.rs +++ b/ui/src/conf/accounts.rs @@ -227,7 +227,7 @@ impl Account { map: &Backends, work_context: WorkContext, notify_fn: NotifyFn, - ) -> Self { + ) -> Result { let s = settings.clone(); let backend = map.get(settings.account().format())( settings.account(), @@ -235,7 +235,7 @@ impl Account { s.folder_confs.contains_key(path) && s.folder_confs[path].folder_conf().subscribe.is_true() }), - ); + )?; let notify_fn = Arc::new(notify_fn); let data_dir = xdg::BaseDirectories::with_profile("meli", &name).unwrap(); @@ -258,7 +258,7 @@ impl Account { settings.conf.cache_type = crate::conf::CacheType::None; } - Account { + Ok(Account { index, name, is_online: false, @@ -278,7 +278,7 @@ impl Account { notify_fn, event_queue: VecDeque::with_capacity(8), - } + }) } fn init(&mut self) { diff --git a/ui/src/state.rs b/ui/src/state.rs index ea0e63a9..f8e4d567 100644 --- a/ui/src/state.rs +++ b/ui/src/state.rs @@ -199,14 +199,8 @@ impl Drop for State { } } -impl Default for State { - fn default() -> Self { - Self::new() - } -} - impl State { - pub fn new() -> Self { + pub fn new() -> Result { /* Create a channel to communicate with other threads. The main process is the sole receiver. * */ let (sender, receiver) = bounded(32 * ::std::mem::size_of::()); @@ -217,13 +211,11 @@ impl State { * */ let input_thread = unbounded(); let backends = Backends::new(); - let settings = Settings::new(); + let settings = Settings::new()?; - let termsize = termion::terminal_size().ok(); - let termcols = termsize.map(|(w, _)| w); - let termrows = termsize.map(|(_, h)| h); - let cols = termcols.unwrap_or(0) as usize; - let rows = termrows.unwrap_or(0) as usize; + let termsize = termion::terminal_size()?; + let cols = termsize.0 as usize; + let rows = termsize.1 as usize; let work_controller = WorkController::new(sender.clone()); let mut accounts: Vec = settings @@ -245,7 +237,7 @@ impl State { })), ) }) - .collect(); + .collect::>>()?; accounts.sort_by(|a, b| a.name().cmp(&b.name())); let mut s = State { @@ -284,10 +276,15 @@ impl State { s.switch_to_alternate_screen(); debug!("inserting mailbox hashes:"); for i in 0..s.context.accounts.len() { - s.context.is_online(i); + if s.context.is_online(i) && s.context.accounts[i].is_empty() { + return Err(MeliError::new(format!( + "Account {} has no folders configured.", + s.context.accounts[i].name() + ))); + } } s.context.restore_input(); - s + Ok(s) } /*