From patchwork Thu Dec 13 02:43:53 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Michael Ellerman X-Patchwork-Id: 1012482 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 43FdQf14Xcz9s7T for ; Thu, 13 Dec 2018 13:48:10 +1100 (AEDT) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ellerman.id.au Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 43FdQd6wm9zDqJW for ; Thu, 13 Dec 2018 13:48:09 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; dmarc=none (p=none dis=none) header.from=ellerman.id.au X-Original-To: snowpatch@lists.ozlabs.org Delivered-To: snowpatch@lists.ozlabs.org Received: from ozlabs.org (bilbo.ozlabs.org [IPv6:2401:3900:2:1::2]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 43FdKr5PY5zDqPW for ; Thu, 13 Dec 2018 13:44:00 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; dmarc=none (p=none dis=none) header.from=ellerman.id.au Received: by ozlabs.org (Postfix, from userid 1034) id 43FdKr3nkmz9s8r; Thu, 13 Dec 2018 13:44:00 +1100 (AEDT) From: Michael Ellerman To: snowpatch@lists.ozlabs.org Date: Thu, 13 Dec 2018 13:43:53 +1100 Message-Id: <20181213024353.19725-3-mpe@ellerman.id.au> X-Mailer: git-send-email 2.17.2 In-Reply-To: <20181213024353.19725-1-mpe@ellerman.id.au> References: <20181213024353.19725-1-mpe@ellerman.id.au> X-Mailman-Approved-At: Thu, 13 Dec 2018 13:48:00 +1100 Subject: [snowpatch] [PATCH 3/3] Rewrite settings parsing to avoid panic X-BeenThere: snowpatch@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Continuous Integration for patch-based workflows List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: snowpatch-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org Sender: "snowpatch" Change parse() to return a Result, using the question mark operator to catch errors and return early. Use map_err() to turn the low-level errors into something human readable. We could define our own error type but that seems like overkill for this. Rework the tests to cope. For the two failing cases that's a bit ugly because we have to map the Ok() to Err() and vice versa. There are now no panic()s or unwrap()s in settings.rs! Error messages seen by the user look like, eg: Error: Unable to parse config file: missing field `git` Error: Unable to open config file: Permission denied (os error 13) Error: Unable to open config file: No such file or directory (os error 2) Reviewed-by: Andrew Donnellan --- src/main.rs | 2 +- src/settings.rs | 42 +++++++++++++++++++++--------------------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/main.rs b/src/main.rs index 2e022a3939e9..d2c24be3a59f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -303,7 +303,7 @@ fn run() -> Result<(), Box> { .and_then(|d| d.version(Some(version)).deserialize()) .unwrap_or_else(|e| e.exit()); - let settings = settings::parse(&args.arg_config_file); + let settings = settings::parse(&args.arg_config_file)?; // The HTTP client we'll use to access the APIs // TODO: Handle https_proxy diff --git a/src/settings.rs b/src/settings.rs index a5513728fbaa..8e86b213a6bc 100644 --- a/src/settings.rs +++ b/src/settings.rs @@ -22,6 +22,7 @@ use git2; use git2::Repository; use std::collections::BTreeMap; +use std::error::Error; use std::fmt; use std::fs::File; use std::io::Read; @@ -186,23 +187,18 @@ pub struct Config { pub projects: BTreeMap, } -pub fn parse(path: &str) -> Config { +pub fn parse(path: &str) -> Result> { let mut toml_config = String::new(); - let mut file = File::open(&path).expect("Couldn't open config file, exiting."); + File::open(&path) + .map_err(|e| format!("Unable to open config file: {}", e))? + .read_to_string(&mut toml_config) + .map_err(|e| format!("Unable to read config file: {}", e))?; - file.read_to_string(&mut toml_config) - .unwrap_or_else(|err| panic!("Couldn't read config: {}", err)); + let config = toml::de::from_str::(&toml_config) + .map_err(|e| format!("Unable to parse config file: {}", e))?; - let toml_config = toml::de::from_str::(&toml_config); - - match toml_config { - Ok(config_inside) => config_inside, - Err(err) => { - error!("TOML error: {}", err); - panic!("Could not parse configuration file, exiting"); - } - } + Ok(config) } #[cfg(test)] @@ -210,19 +206,23 @@ mod test { use settings::*; #[test] - #[should_panic(expected = "Couldn't open config file")] - fn bad_path() { - parse("/nonexistent/config.file"); + fn bad_path() -> Result<(), &'static str> { + match parse("/nonexistent/config.file") { + Ok(_) => Err("Didn't fail opening non-existent file?"), + Err(_) => Ok(()), + } } #[test] - fn parse_example_openpower() { - parse("examples/openpower.toml"); + fn parse_example_openpower() -> Result<(), Box> { + parse("examples/openpower.toml").map(|_| ()) } #[test] - #[should_panic(expected = "Could not parse configuration file, exiting")] - fn parse_example_invalid() { - parse("examples/tests/invalid.toml"); + fn parse_example_invalid() -> Result<(), &'static str> { + match parse("examples/tests/invalid.toml") { + Ok(_) => Err("Didn't fail parsing invalid TOML?"), + Err(_) => Ok(()) + } } }