[snowpatch] [PATCH 3/3] Rewrite settings parsing to avoid panic

[Andrew Donnellan]

On 13/12/18 1:43 pm, Michael Ellerman wrote:
> 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)

woo!

Reviewed-by: Andrew Donnellan <andrew.donnellan at au1.ibm.com>

> ---
>   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<Error>> {
>           .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<String, Project>,
>   }
>   
> -pub fn parse(path: &str) -> Config {
> +pub fn parse(path: &str) -> Result<Config, Box<Error>> {
>       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::<Config>(&toml_config)
> +        .map_err(|e| format!("Unable to parse config file: {}", e))?;
>   
> -    let toml_config = toml::de::from_str::<Config>(&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<Error>> {
> +        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(())
> +        }
>       }
>   }
> 

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan at au1.ibm.com  IBM Australia Limited