From ec385c80f0dcb9074ebb484cde2511022f2a90f6 Mon Sep 17 00:00:00 2001 From: Seth Pellegrino Date: Wed, 12 Aug 2020 08:27:54 -0700 Subject: [PATCH 1/4] feat: errors with file and line info --- src/lib.rs | 154 ++++++++++++++++++++++++++++++++++++++++-------- tests/config.rs | 5 +- 2 files changed, 132 insertions(+), 27 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 13a3445..f497fdf 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -82,6 +82,48 @@ impl std::default::Default for SSHConfig { } } +/// FileError is an error tied to a particular place in a file +#[derive(Debug)] +pub struct FileError { + /// filename is the name of the file where the error occurred + pub filename: String, + /// lineno is the line number within the file, or 0 + pub lineno: usize, + /// err is the error + pub err: Error, +} + +impl std::error::Error for FileError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + Some(&self.err) + } +} + +impl fmt::Display for FileError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match (self.filename.as_str(), self.lineno) { + ("", 0) => fmt::Display::fmt(&self.err, f), + (_, 0) => write!(f, "{}: {}", self.filename, self.err), + _ => write!(f, "{}:{} {}", self.filename, self.lineno, self.err), + } + } +} + +impl From<((S, usize), E)> for FileError +where + S: AsRef, + Error: From, +{ + fn from(o: ((S, usize), E)) -> Self { + let ((filename, lineno), err) = o; + FileError { + filename: filename.as_ref().to_owned(), + lineno, + err: From::from(err), + } + } +} + #[allow(missing_docs)] #[derive(Debug)] pub enum Error { @@ -187,7 +229,7 @@ fn homedir() -> OsString { #[allow(missing_docs)] impl SSHConfig { - pub fn from_default_files() -> Result { + pub fn from_default_files() -> Result { Self::from_files( [&Self::user_config(), &Self::system_config()] .iter() @@ -195,14 +237,14 @@ impl SSHConfig { ) } - pub fn from_file

(path: P) -> Result + pub fn from_file

(path: P) -> Result where P: AsRef, { Self::from_files(std::iter::once(path)) } - pub fn from_files(paths: I) -> Result + pub fn from_files(paths: I) -> Result where P: AsRef, I: IntoIterator, @@ -270,14 +312,25 @@ struct ReadMeta { user_config: bool, } -fn readconf_depth>(path: P, meta: ReadMeta) -> Result, Error> { +fn readconf_depth>(path: P, meta: ReadMeta) -> Result, FileError> { + let filename = if let Some(s) = path.as_ref().to_str() { + s + } else { + return Err(Error::PathInvalidUnicode(path.as_ref().to_path_buf(), None)) + .zip_err(("", 0))?; + }; + if meta.depth > MAX_READCONF_DEPTH { - return Err(Error::MaxDepthExceeded); + return Err(FileError { + filename: filename.to_owned(), + lineno: 0, + err: Error::MaxDepthExceeded, + }); } // The only depth 0 file that gets checked for perms is the user config file if meta.depth > 0 || *path.as_ref() == SSHConfig::user_config() { - let meta = fs::metadata(&path)?; + let meta = fs::metadata(&path).zip_err((filename, 0))?; let perms = meta.permissions(); if cfg!(unix) { @@ -292,22 +345,22 @@ fn readconf_depth>(path: P, meta: ReadMeta) -> Result>(path: P, meta: ReadMeta) -> Result { - return Err(Error::BadInclude(p)); + return Err(FileError { + filename: filename.to_owned(), + lineno, + err: Error::BadInclude(p), + }); } - p if p.starts_with('~') => String::from_utf8(tilde_expand(p.as_bytes()))?, + p if p.starts_with('~') => String::from_utf8(tilde_expand(p.as_bytes())) + .zip_err((filename, lineno))?, p if !Path::new(&p).has_root() => { let anchor = if meta.user_config { SSHConfig::user_dir() @@ -340,7 +398,11 @@ fn readconf_depth>(path: P, meta: ReadMeta) -> Result p, @@ -359,10 +421,21 @@ fn readconf_depth>(path: P, meta: ReadMeta) -> Result { - return Err(e.into()) + Err(FileError { + err: Error::Read(e), + filename, + lineno, + }) if e.kind() == io::ErrorKind::InvalidData => { + return Err(FileError { + err: Error::Read(e), + filename, + lineno, + }) } - Err(Error::Read(_)) => continue, + Err(FileError { + err: Error::Read(_), + .. + }) => continue, err @ Err(_) => return err, Ok(o) => vec.extend(o), @@ -377,10 +450,24 @@ fn readconf_depth>(path: P, meta: ReadMeta) -> Result { + fn zip_err(self, u: U) -> Result; +} + +impl ZipErr for Result { + fn zip_err(self, u: U) -> Result { + match self { + Ok(v) => Ok(v), + Err(e) => Err((u, e)), + } + } +} + #[cfg(test)] mod test { - use super::{option, SSHConfig, SSHOption, MAX_READCONF_DEPTH}; + use super::{option, Error, FileError, SSHConfig, SSHOption, MAX_READCONF_DEPTH}; use std::fs; + use std::path::Path; use tempfile::tempdir; #[test] @@ -403,6 +490,22 @@ mod test { assert_eq!(cfg.0[..expect.len()], expect) } + #[test] + fn err_context() { + let dir = tempdir().unwrap(); + let path = dir.path().join("config"); + fs::write(&path, r"Badopt").expect("failed writing config"); + let err = SSHConfig::from_file(&path).expect_err("parse should have failed"); + + assert!( + Path::new(&err.filename).ends_with("config"), + "expected file path ending in `config`, got: {}", + err.filename + ); + assert_eq!(err.lineno, 1); + assert_matches!(err.err, Error::Parse(_)); + } + #[test] fn includes() { let dir = tempdir().unwrap(); @@ -448,12 +551,13 @@ mod test { } } - let err = SSHConfig::from_file(dir.path().join(format!("file_{}", MAX_READCONF_DEPTH + 1))) - .expect_err("parse should have failed"); + let FileError { err, .. } = + SSHConfig::from_file(dir.path().join(format!("file_{}", MAX_READCONF_DEPTH + 1))) + .expect_err("parse should have failed"); assert_eq!( std::mem::discriminant(&err), - std::mem::discriminant(&super::Error::MaxDepthExceeded), + std::mem::discriminant(&Error::MaxDepthExceeded), ) } } diff --git a/tests/config.rs b/tests/config.rs index 16fcc32..a27715a 100644 --- a/tests/config.rs +++ b/tests/config.rs @@ -1,6 +1,6 @@ use ssh2_config::{ option::{self, parse_opt, SSHOption}, - Error, SSHConfig, MAX_READCONF_DEPTH, + Error, FileError, SSHConfig, MAX_READCONF_DEPTH, }; use std::fs; use std::io; @@ -452,7 +452,8 @@ fn deep_includes() { assert!(output.status.success()); let bad_file = dir.path().join(format!("file_{}", MAX_READCONF_DEPTH + 1)); - let err = SSHConfig::from_file(&bad_file).expect_err("parse should have failed"); + let FileError { err, .. } = + SSHConfig::from_file(&bad_file).expect_err("parse should have failed"); assert_eq!( std::mem::discriminant(&err), From 01a1341f3b8aa763396fcb005833051393d9f889 Mon Sep 17 00:00:00 2001 From: Seth Pellegrino Date: Wed, 12 Aug 2020 09:13:34 -0700 Subject: [PATCH 2/4] test: remove overly specific test --- src/lib.rs | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index f497fdf..83a4362 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -470,26 +470,6 @@ mod test { use std::path::Path; use tempfile::tempdir; - #[test] - // TODO remove and/or figure out how to chroot in CI - #[cfg(not(CI))] - fn it_works() { - let cfg = SSHConfig::from_default_files().expect("read failed"); - let expect = [ - SSHOption::Host(String::from("github.com")), - SSHOption::User(String::from("git")), - SSHOption::Host(String::from("bitbucket.org")), - SSHOption::User(String::from("git")), - SSHOption::Include(option::Include::Opts(vec![])), - SSHOption::Host(String::from("*")), - SSHOption::SendEnv(vec![ - option::Env::Send(String::from("LANG")), - option::Env::Send(String::from("LC_*")), - ]), - ]; - assert_eq!(cfg.0[..expect.len()], expect) - } - #[test] fn err_context() { let dir = tempdir().unwrap(); From b51a4722924a60ecd4698f79a4b108846e78b4f1 Mon Sep 17 00:00:00 2001 From: sethp Date: Fri, 14 Aug 2020 09:49:19 -0700 Subject: [PATCH 3/4] Update src/lib.rs --- src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index 83a4362..f732486 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -85,7 +85,7 @@ impl std::default::Default for SSHConfig { /// FileError is an error tied to a particular place in a file #[derive(Debug)] pub struct FileError { - /// filename is the name of the file where the error occurred + /// filename is the path of the file where the error occurred, or "" (if the path is not unicode) pub filename: String, /// lineno is the line number within the file, or 0 pub lineno: usize, From 57f545474297019b14d63006f639f419f17d4086 Mon Sep 17 00:00:00 2001 From: Seth Pellegrino Date: Sun, 16 Aug 2020 07:27:29 -0700 Subject: [PATCH 4/4] lint: clean up extra Self types --- src/option.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/option.rs b/src/option.rs index d6fd619..5ff097b 100644 --- a/src/option.rs +++ b/src/option.rs @@ -476,7 +476,7 @@ impl<'a> Args<'a> { } /// has_next returns true if there is at least one more argument. - fn has_next(self: &mut Self) -> bool { + fn has_next(&mut self) -> bool { if let Some(next) = self.0.next() { self.0.put_back(next); true @@ -489,7 +489,7 @@ impl<'a> Args<'a> { impl<'a> Iterator for Args<'a> { type Item = Result; - fn next(self: &mut Self) -> Option { + fn next(&mut self) -> Option { match self.map_owned(std::convert::identity) { Err(Error::MissingArgument) => None, res => Some(res), @@ -499,7 +499,7 @@ impl<'a> Iterator for Args<'a> { impl<'a> Arguments for Args<'a> { // comparable to strdelim - fn map_next(self: &mut Self, f: F) -> Result + fn map_next(&mut self, f: F) -> Result where F: FnOnce(&str, &mut Self) -> Result, { @@ -548,12 +548,12 @@ impl<'a> Arguments for Args<'a> { /// - `Args` pub trait Arguments: Iterator> { /// map_next provides access to the next argument and subsequent arguments - fn map_next(self: &mut Self, f: F) -> Result + fn map_next(&mut self, f: F) -> Result where F: FnOnce(&str, &mut Self) -> Result; /// map_one provides access to a owned copy of the next argument - fn map_owned(self: &mut Self, f: F) -> Result + fn map_owned(&mut self, f: F) -> Result where F: FnOnce(String) -> T, { @@ -1035,13 +1035,13 @@ pub mod simple { impl<'a> Iterator for Args<'a> { type Item = Result; - fn next(self: &mut Self) -> Option { + fn next(&mut self) -> Option { Some(self.0.next()?.map(str::to_owned)) } } impl<'a> Arguments for Args<'a> { - fn map_next(self: &mut Self, f: F) -> Result + fn map_next(&mut self, f: F) -> Result where F: FnOnce(&str, &mut Self) -> Result, {