From 2b18e45ecec277f8511299d8294eb3f48cb19faf Mon Sep 17 00:00:00 2001 From: Andreas Hartmann Date: Tue, 13 Jul 2021 15:57:07 +0200 Subject: [PATCH 1/5] install: Use UResult Related to uutils#2464 --- src/uu/install/src/install.rs | 233 ++++++++++++++++++++-------------- 1 file changed, 138 insertions(+), 95 deletions(-) diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index 81d44bb6c..6000837cb 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -5,7 +5,7 @@ // * For the full copyright and license information, please view the LICENSE file // * that was distributed with this source code. -// spell-checker:ignore (ToDO) rwxr sourcepath targetpath +// spell-checker:ignore (ToDO) rwxr sourcepath targetpath Isnt mod mode; @@ -17,15 +17,17 @@ use file_diff::diff; use filetime::{set_file_times, FileTime}; use uucore::backup_control::{self, BackupMode}; use uucore::entries::{grp2gid, usr2uid}; +use uucore::error::{FromIo, UCustomError, UIoError, UResult, USimpleError}; use uucore::perms::{wrap_chgrp, wrap_chown, Verbosity}; use libc::{getegid, geteuid}; +use std::error::Error; +use std::fmt::{Debug, Display}; use std::fs; use std::fs::File; use std::os::unix::fs::MetadataExt; use std::path::{Path, PathBuf}; use std::process::Command; -use std::result::Result; const DEFAULT_MODE: u32 = 0o755; const DEFAULT_STRIP_PROGRAM: &str = "strip"; @@ -47,6 +49,81 @@ pub struct Behavior { target_dir: Option, } +#[derive(Debug)] +enum InstallError { + Unimplemented(String), + DirNeedsArg(), + CreateDirFailed(PathBuf, std::io::Error), + ChmodFailed(PathBuf), + InvalidTarget(PathBuf), + TargetDirIsntDir(PathBuf), + BackupFailed(PathBuf, PathBuf, String), + InstallFailed(PathBuf, PathBuf, std::io::Error), + StripProgramFailed(String), + MetadataFailed(std::io::Error), + NoSuchUser(String), + NoSuchGroup(String), + SilentError(), +} + +impl UCustomError for InstallError { + fn code(&self) -> i32 { + match self { + InstallError::Unimplemented(_) => 2, + _ => 1, + } + } + + fn usage(&self) -> bool { + false + } +} + +impl Error for InstallError {} + +impl Display for InstallError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + use InstallError as IE; + match self { + IE::Unimplemented(opt) => write!(f, "Unimplemented feature: {}", opt), + IE::DirNeedsArg() => write!( + f, + "{} with -d requires at least one argument.", + executable!() + ), + IE::CreateDirFailed(dir, e) => write!(f, "failed to create {}: {}", dir.display(), e), + IE::ChmodFailed(file) => write!(f, "failed to chmod {}", file.display()), + IE::InvalidTarget(target) => write!( + f, + "invalid target {}: No such file or directory", + target.display() + ), + IE::TargetDirIsntDir(target) => { + write!(f, "target '{}' is not a directory", target.display()) + } + IE::BackupFailed(from, to, e) => write!( + f, + "cannot backup file '{}' to '{}': {}", + from.display(), + to.display(), + e + ), + IE::InstallFailed(from, to, e) => write!( + f, + "cannot install '{}' to '{}': {}", + from.display(), + to.display(), + e + ), + IE::StripProgramFailed(msg) => write!(f, "strip program failed: {}", msg), + IE::MetadataFailed(e) => write!(f, "{}", e.to_string()), + IE::NoSuchUser(user) => write!(f, "no such user: {}", user), + IE::NoSuchGroup(group) => write!(f, "no such group: {}", group), + IE::SilentError() => write!(f, ""), + } + } +} + #[derive(Clone, Eq, PartialEq)] pub enum MainFunction { /// Create directories @@ -97,7 +174,8 @@ fn get_usage() -> String { /// /// Returns a program return code. /// -pub fn uumain(args: impl uucore::Args) -> i32 { +#[uucore_procs::gen_uumain] +pub fn uumain(args: impl uucore::Args) -> UResult<()> { let usage = get_usage(); let matches = uu_app().usage(&usage[..]).get_matches_from(args); @@ -107,17 +185,9 @@ pub fn uumain(args: impl uucore::Args) -> i32 { .map(|v| v.map(ToString::to_string).collect()) .unwrap_or_default(); - if let Err(s) = check_unimplemented(&matches) { - show_error!("Unimplemented feature: {}", s); - return 2; - } + check_unimplemented(&matches)?; - let behavior = match behavior(&matches) { - Ok(x) => x, - Err(ret) => { - return ret; - } - }; + let behavior = behavior(&matches)?; match behavior.main_function { MainFunction::Directory => directory(paths, behavior), @@ -269,13 +339,13 @@ pub fn uu_app() -> App<'static, 'static> { /// Error datum is a string of the unimplemented argument. /// /// -fn check_unimplemented<'a>(matches: &ArgMatches) -> Result<(), &'a str> { +fn check_unimplemented(matches: &ArgMatches) -> UResult<()> { if matches.is_present(OPT_NO_TARGET_DIRECTORY) { - Err("--no-target-directory, -T") + Err(InstallError::Unimplemented(String::from("--no-target-directory, -T")).into()) } else if matches.is_present(OPT_PRESERVE_CONTEXT) { - Err("--preserve-context, -P") + Err(InstallError::Unimplemented(String::from("--preserve-context, -P")).into()) } else if matches.is_present(OPT_CONTEXT) { - Err("--context, -Z") + Err(InstallError::Unimplemented(String::from("--context, -Z")).into()) } else { Ok(()) } @@ -289,7 +359,7 @@ fn check_unimplemented<'a>(matches: &ArgMatches) -> Result<(), &'a str> { /// /// In event of failure, returns an integer intended as a program return code. /// -fn behavior(matches: &ArgMatches) -> Result { +fn behavior(matches: &ArgMatches) -> UResult { let main_function = if matches.is_present(OPT_DIRECTORY) { MainFunction::Directory } else { @@ -314,10 +384,7 @@ fn behavior(matches: &ArgMatches) -> Result { matches.value_of(OPT_BACKUP), ); let backup_mode = match backup_mode { - Err(err) => { - show_usage_error!("{}", err); - return Err(1); - } + Err(err) => return Err(USimpleError::new(1, err)), Ok(mode) => mode, }; @@ -351,10 +418,9 @@ fn behavior(matches: &ArgMatches) -> Result { /// /// Returns an integer intended as a program return code. /// -fn directory(paths: Vec, b: Behavior) -> i32 { +fn directory(paths: Vec, b: Behavior) -> UResult<()> { if paths.is_empty() { - println!("{} with -d requires at least one argument.", executable!()); - 1 + Err(InstallError::DirNeedsArg().into()) } else { let mut all_successful = true; @@ -384,9 +450,9 @@ fn directory(paths: Vec, b: Behavior) -> i32 { } } if all_successful { - 0 + Ok(()) } else { - 1 + Err(InstallError::SilentError().into()) } } } @@ -403,7 +469,7 @@ fn is_new_file_path(path: &Path) -> bool { /// /// Returns an integer intended as a program return code. /// -fn standard(mut paths: Vec, b: Behavior) -> i32 { +fn standard(mut paths: Vec, b: Behavior) -> UResult<()> { let target: PathBuf = b .target_dir .clone() @@ -418,13 +484,11 @@ fn standard(mut paths: Vec, b: Behavior) -> i32 { if let Some(parent) = target.parent() { if !parent.exists() && b.create_leading { if let Err(e) = fs::create_dir_all(parent) { - show_error!("failed to create {}: {}", parent.display(), e); - return 1; + return Err(InstallError::CreateDirFailed(parent.to_path_buf(), e).into()); } if mode::chmod(parent, b.mode()).is_err() { - show_error!("failed to chmod {}", parent.display()); - return 1; + return Err(InstallError::ChmodFailed(parent.to_path_buf()).into()); } } } @@ -432,11 +496,7 @@ fn standard(mut paths: Vec, b: Behavior) -> i32 { if target.is_file() || is_new_file_path(&target) { copy_file_to_file(&sources[0], &target, &b) } else { - show_error!( - "invalid target {}: No such file or directory", - target.display() - ); - 1 + Err(InstallError::InvalidTarget(target).into()) } } } @@ -451,10 +511,9 @@ fn standard(mut paths: Vec, b: Behavior) -> i32 { /// _files_ must all exist as non-directories. /// _target_dir_ must be a directory. /// -fn copy_files_into_dir(files: &[PathBuf], target_dir: &Path, b: &Behavior) -> i32 { +fn copy_files_into_dir(files: &[PathBuf], target_dir: &Path, b: &Behavior) -> UResult<()> { if !target_dir.is_dir() { - show_error!("target '{}' is not a directory", target_dir.display()); - return 1; + return Err(InstallError::TargetDirIsntDir(target_dir.to_path_buf()).into()); } let mut all_successful = true; @@ -484,9 +543,9 @@ fn copy_files_into_dir(files: &[PathBuf], target_dir: &Path, b: &Behavior) -> i3 } } if all_successful { - 0 + Ok(()) } else { - 1 + Err(InstallError::SilentError().into()) } } @@ -500,12 +559,8 @@ fn copy_files_into_dir(files: &[PathBuf], target_dir: &Path, b: &Behavior) -> i3 /// _file_ must exist as a non-directory. /// _target_ must be a non-directory /// -fn copy_file_to_file(file: &Path, target: &Path, b: &Behavior) -> i32 { - if copy(file, target, b).is_err() { - 1 - } else { - 0 - } +fn copy_file_to_file(file: &Path, target: &Path, b: &Behavior) -> UResult<()> { + copy(file, target, b) } /// Copy one file to a new location, changing metadata. @@ -520,8 +575,8 @@ fn copy_file_to_file(file: &Path, target: &Path, b: &Behavior) -> i32 { /// If the copy system call fails, we print a verbose error and return an empty error value. /// #[allow(clippy::cognitive_complexity)] -fn copy(from: &Path, to: &Path, b: &Behavior) -> Result<(), ()> { - if b.compare && !need_copy(from, to, b) { +fn copy(from: &Path, to: &Path, b: &Behavior) -> UResult<()> { + if b.compare && !need_copy(from, to, b)? { return Ok(()); } // Declare the path here as we may need it for the verbose output below. @@ -536,13 +591,12 @@ fn copy(from: &Path, to: &Path, b: &Behavior) -> Result<(), ()> { if let Some(ref backup_path) = backup_path { // TODO!! if let Err(err) = fs::rename(to, backup_path) { - show_error!( - "install: cannot backup file '{}' to '{}': {}", - to.display(), - backup_path.display(), - err - ); - return Err(()); + return Err(InstallError::BackupFailed( + to.to_path_buf(), + backup_path.to_path_buf(), + err.to_string(), + ) + .into()); } } } @@ -552,52 +606,41 @@ fn copy(from: &Path, to: &Path, b: &Behavior) -> Result<(), ()> { * https://github.com/rust-lang/rust/issues/79390 */ if let Err(err) = File::create(to) { - show_error!( - "install: cannot install '{}' to '{}': {}", - from.display(), - to.display(), - err + return Err( + InstallError::InstallFailed(from.to_path_buf(), to.to_path_buf(), err).into(), ); - return Err(()); } } else if let Err(err) = fs::copy(from, to) { - show_error!( - "cannot install '{}' to '{}': {}", - from.display(), - to.display(), - err - ); - return Err(()); + return Err(InstallError::InstallFailed(from.to_path_buf(), to.to_path_buf(), err).into()); } if b.strip && cfg!(not(windows)) { match Command::new(&b.strip_program).arg(to).output() { Ok(o) => { if !o.status.success() { - crash!( - 1, - "strip program failed: {}", - String::from_utf8(o.stderr).unwrap_or_default() - ); + return Err(InstallError::StripProgramFailed( + String::from_utf8(o.stderr).unwrap_or_default(), + ) + .into()); } } - Err(e) => crash!(1, "strip program execution failed: {}", e), + Err(e) => return Err(InstallError::StripProgramFailed(e.to_string()).into()), } } if mode::chmod(to, b.mode()).is_err() { - return Err(()); + return Err(InstallError::ChmodFailed(to.to_path_buf()).into()); } if !b.owner.is_empty() { let meta = match fs::metadata(to) { Ok(meta) => meta, - Err(f) => crash!(1, "{}", f.to_string()), + Err(e) => return Err(InstallError::MetadataFailed(e).into()), }; let owner_id = match usr2uid(&b.owner) { Ok(g) => g, - _ => crash!(1, "no such user: {}", b.owner), + _ => return Err(InstallError::NoSuchUser(b.owner.clone()).into()), }; let gid = meta.gid(); match wrap_chown( @@ -620,12 +663,12 @@ fn copy(from: &Path, to: &Path, b: &Behavior) -> Result<(), ()> { if !b.group.is_empty() { let meta = match fs::metadata(to) { Ok(meta) => meta, - Err(f) => crash!(1, "{}", f.to_string()), + Err(e) => return Err(InstallError::MetadataFailed(e).into()), }; let group_id = match grp2gid(&b.group) { Ok(g) => g, - _ => crash!(1, "no such group: {}", b.group), + _ => return Err(InstallError::NoSuchGroup(b.group.clone()).into()), }; match wrap_chgrp(to, &meta, group_id, false, Verbosity::Normal) { Ok(n) => { @@ -640,7 +683,7 @@ fn copy(from: &Path, to: &Path, b: &Behavior) -> Result<(), ()> { if b.preserve_timestamps { let meta = match fs::metadata(from) { Ok(meta) => meta, - Err(f) => crash!(1, "{}", f.to_string()), + Err(e) => return Err(InstallError::MetadataFailed(e).into()), }; let modified_time = FileTime::from_last_modification_time(&meta); @@ -679,14 +722,14 @@ fn copy(from: &Path, to: &Path, b: &Behavior) -> Result<(), ()> { /// /// Crashes the program if a nonexistent owner or group is specified in _b_. /// -fn need_copy(from: &Path, to: &Path, b: &Behavior) -> bool { +fn need_copy(from: &Path, to: &Path, b: &Behavior) -> UResult { let from_meta = match fs::metadata(from) { Ok(meta) => meta, - Err(_) => return true, + Err(_) => return Ok(true), }; let to_meta = match fs::metadata(to) { Ok(meta) => meta, - Err(_) => return true, + Err(_) => return Ok(true), }; // setuid || setgid || sticky @@ -696,15 +739,15 @@ fn need_copy(from: &Path, to: &Path, b: &Behavior) -> bool { || from_meta.mode() & extra_mode != 0 || to_meta.mode() & extra_mode != 0 { - return true; + return Ok(true); } if !from_meta.is_file() || !to_meta.is_file() { - return true; + return Ok(true); } if from_meta.len() != to_meta.len() { - return true; + return Ok(true); } // TODO: if -P (#1809) and from/to contexts mismatch, return true. @@ -712,31 +755,31 @@ fn need_copy(from: &Path, to: &Path, b: &Behavior) -> bool { if !b.owner.is_empty() { let owner_id = match usr2uid(&b.owner) { Ok(id) => id, - _ => crash!(1, "no such user: {}", b.owner), + _ => return Err(InstallError::NoSuchUser(b.owner.clone()).into()), }; if owner_id != to_meta.uid() { - return true; + return Ok(true); } } else if !b.group.is_empty() { let group_id = match grp2gid(&b.group) { Ok(id) => id, - _ => crash!(1, "no such group: {}", b.group), + _ => return Err(InstallError::NoSuchGroup(b.group.clone()).into()), }; if group_id != to_meta.gid() { - return true; + return Ok(true); } } else { #[cfg(not(target_os = "windows"))] unsafe { if to_meta.uid() != geteuid() || to_meta.gid() != getegid() { - return true; + return Ok(true); } } } if !diff(from.to_str().unwrap(), to.to_str().unwrap()) { - return true; + return Ok(true); } - false + Ok(false) } From 0461a45c9ab5969fea47ac03b490b2135819a24b Mon Sep 17 00:00:00 2001 From: Andreas Hartmann Date: Wed, 14 Jul 2021 08:39:12 +0200 Subject: [PATCH 2/5] install: Use show! macro for noncritical errors during execution Drop the previous flags that would tell whether a noncritical error occured during execution in favor of the `show!` macro from the error submodule. This allows us to generate regular error types during execution to signify failures inside the program, but without prematurely aborting program execution if not needed or specified. Also make verbose outputs use `print!` and friends instead of `show_error!` to ensure verbose output is redirected to stdout, not stderr. --- src/uu/install/src/install.rs | 81 ++++++++++++++++++----------------- 1 file changed, 41 insertions(+), 40 deletions(-) diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index 6000837cb..7de69df00 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -63,7 +63,7 @@ enum InstallError { MetadataFailed(std::io::Error), NoSuchUser(String), NoSuchGroup(String), - SilentError(), + OmittingDirectory(PathBuf), } impl UCustomError for InstallError { @@ -119,7 +119,7 @@ impl Display for InstallError { IE::MetadataFailed(e) => write!(f, "{}", e.to_string()), IE::NoSuchUser(user) => write!(f, "no such user: {}", user), IE::NoSuchGroup(group) => write!(f, "no such group: {}", group), - IE::SilentError() => write!(f, ""), + IE::OmittingDirectory(dir) => write!(f, "omitting directory '{}'", dir.display()), } } } @@ -416,44 +416,46 @@ fn behavior(matches: &ArgMatches) -> UResult { /// GNU man pages describe this functionality as creating 'all components of /// the specified directories'. /// -/// Returns an integer intended as a program return code. +/// Returns a Result type with the Err variant containing the error message. /// fn directory(paths: Vec, b: Behavior) -> UResult<()> { if paths.is_empty() { Err(InstallError::DirNeedsArg().into()) } else { - let mut all_successful = true; - for path in paths.iter().map(Path::new) { // if the path already exist, don't try to create it again if !path.exists() { - // Differently than the primary functionality (MainFunction::Standard), the directory - // functionality should create all ancestors (or components) of a directory regardless - // of the presence of the "-D" flag. - // NOTE: the GNU "install" sets the expected mode only for the target directory. All - // created ancestor directories will have the default mode. Hence it is safe to use - // fs::create_dir_all and then only modify the target's dir mode. - if let Err(e) = fs::create_dir_all(path) { - show_error!("{}: {}", path.display(), e); - all_successful = false; + // Differently than the primary functionality + // (MainFunction::Standard), the directory functionality should + // create all ancestors (or components) of a directory + // regardless of the presence of the "-D" flag. + // + // NOTE: the GNU "install" sets the expected mode only for the + // target directory. All created ancestor directories will have + // the default mode. Hence it is safe to use fs::create_dir_all + // and then only modify the target's dir mode. + if let Err(e) = + fs::create_dir_all(path).map_err_context(|| format!("{}", path.display())) + { + show!(e); continue; } if b.verbose { - show_error!("creating directory '{}'", path.display()); + println!("creating directory '{}'", path.display()); } } if mode::chmod(path, b.mode()).is_err() { - all_successful = false; + // Error messages are printed by the mode::chmod function! + uucore::error::set_exit_code(1); continue; } } - if all_successful { - Ok(()) - } else { - Err(InstallError::SilentError().into()) - } + // If the exit code was set, or show! has been called at least once + // (which sets the exit code as well), function execution will end after + // this return. + Ok(()) } } @@ -467,7 +469,7 @@ fn is_new_file_path(path: &Path) -> bool { /// Perform an install, given a list of paths and behavior. /// -/// Returns an integer intended as a program return code. +/// Returns a Result type with the Err variant containing the error message. /// fn standard(mut paths: Vec, b: Behavior) -> UResult<()> { let target: PathBuf = b @@ -504,7 +506,7 @@ fn standard(mut paths: Vec, b: Behavior) -> UResult<()> { /// Copy some files into a directory. /// /// Prints verbose information and error messages. -/// Returns an integer intended as a program return code. +/// Returns a Result type with the Err variant containing the error message. /// /// # Parameters /// @@ -515,22 +517,19 @@ fn copy_files_into_dir(files: &[PathBuf], target_dir: &Path, b: &Behavior) -> UR if !target_dir.is_dir() { return Err(InstallError::TargetDirIsntDir(target_dir.to_path_buf()).into()); } - - let mut all_successful = true; for sourcepath in files.iter() { if !sourcepath.exists() { - show_error!( - "cannot stat '{}': No such file or directory", - sourcepath.display() + let err = UIoError::new( + std::io::ErrorKind::NotFound, + format!("cannot stat '{}'", sourcepath.display()), ); - - all_successful = false; + show!(err); continue; } if sourcepath.is_dir() { - show_error!("omitting directory '{}'", sourcepath.display()); - all_successful = false; + let err = InstallError::OmittingDirectory(sourcepath.to_path_buf()); + show!(err); continue; } @@ -538,21 +537,20 @@ fn copy_files_into_dir(files: &[PathBuf], target_dir: &Path, b: &Behavior) -> UR let filename = sourcepath.components().last().unwrap(); targetpath.push(filename); - if copy(sourcepath, &targetpath, b).is_err() { - all_successful = false; + if let Err(e) = copy(sourcepath, &targetpath, b) { + show!(e); } } - if all_successful { - Ok(()) - } else { - Err(InstallError::SilentError().into()) - } + // If the exit code was set, or show! has been called at least once + // (which sets the exit code as well), function execution will end after + // this return. + Ok(()) } /// Copy a file to another file. /// /// Prints verbose information and error messages. -/// Returns an integer intended as a program return code. +/// Returns a Result type with the Err variant containing the error message. /// /// # Parameters /// @@ -565,6 +563,8 @@ fn copy_file_to_file(file: &Path, target: &Path, b: &Behavior) -> UResult<()> { /// Copy one file to a new location, changing metadata. /// +/// Returns a Result type with the Err variant containing the error message. +/// /// # Parameters /// /// _from_ must exist as a non-directory. @@ -707,6 +707,7 @@ fn copy(from: &Path, to: &Path, b: &Behavior) -> UResult<()> { } /// Return true if a file is necessary to copy. This is the case when: +/// /// - _from_ or _to_ is nonexistent; /// - either file has a sticky bit or set[ug]id bit, or the user specified one; /// - either file isn't a regular file; From 8066ea87cc704680a11c299b7b67e9ecf14f155e Mon Sep 17 00:00:00 2001 From: Andreas Hartmann Date: Tue, 20 Jul 2021 08:38:13 +0200 Subject: [PATCH 3/5] error: Implement `uio_error`-macro Adds a new macro `uio_error` that acts as a convenience wrapper for constructing `UIoError` instances from `std::io::Error`s with a custom error message prepended. It constructs a new `UIoError` instance for the user. Usage examples are included in the docs. --- src/uucore/src/lib/mods/error.rs | 69 ++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/src/uucore/src/lib/mods/error.rs b/src/uucore/src/lib/mods/error.rs index c64e7df66..2cf09f9bf 100644 --- a/src/uucore/src/lib/mods/error.rs +++ b/src/uucore/src/lib/mods/error.rs @@ -48,6 +48,8 @@ //! * Using [`ExitCode`] is not recommended but can be useful for converting utils to use //! [`UResult`]. +// spell-checker:ignore uioerror + use std::{ error::Error, fmt::{Display, Formatter}, @@ -578,6 +580,73 @@ impl From for UError { } } +/// Shorthand to construct [`UIoError`]-instances. +/// +/// This macro serves as a convenience call to quickly construct instances of +/// [`UIoError`]. It takes: +/// +/// - An instance of [`std::io::Error`] +/// - A `format!`-compatible string and +/// - An arbitrary number of arguments to the format string +/// +/// In exactly this order. It is equivalent to the more verbose code seen in the +/// example. +/// +/// # Examples +/// +/// ``` +/// use uucore::error::UIoError; +/// use uucore::uio_error; +/// +/// let io_err = std::io::Error::new( +/// std::io::ErrorKind::PermissionDenied, "fix me please!" +/// ); +/// +/// let uio_err = UIoError::new( +/// io_err.kind(), +/// format!("Error code: {}", 2) +/// ); +/// +/// let other_uio_err = uio_error!(io_err, "Error code: {}", 2); +/// +/// // prints "fix me please!: Permission denied" +/// println!("{}", uio_err); +/// // prints "Error code: 2: Permission denied" +/// println!("{}", other_uio_err); +/// ``` +/// +/// The [`std::fmt::Display`] impl of [`UIoError`] will then ensure that an +/// appropriate error message relating to the actual error kind of the +/// [`std::io::Error`] is appended to whatever error message is defined in +/// addition (as secondary argument). +/// +/// If you want to show only the error message for the [`std::io::ErrorKind`] +/// that's contained in [`UIoError`], pass the second argument as empty string: +/// +/// ``` +/// use uucore::error::UIoError; +/// use uucore::uio_error; +/// +/// let io_err = std::io::Error::new( +/// std::io::ErrorKind::PermissionDenied, "fix me please!" +/// ); +/// +/// let other_uio_err = uio_error!(io_err, ""); +/// +/// // prints: ": Permission denied" +/// println!("{}", other_uio_err); +/// ``` +//#[macro_use] +#[macro_export] +macro_rules! uio_error( + ($err:expr, $($args:tt)+) => ({ + UIoError::new( + $err.kind(), + format!($($args)+) + ) + }) +); + /// Common errors for utilities. /// /// If identical errors appear across multiple utilities, they should be added here. From 4f235e9e95640227f67330ef00f983ec13c530a4 Mon Sep 17 00:00:00 2001 From: Andreas Hartmann Date: Mon, 19 Jul 2021 16:51:14 +0200 Subject: [PATCH 4/5] install: Use `uio_error` to construct errors from std::io::Error Use new macro to construct UIoError types from `std::io::Error` before printing. This gives consistent error messages across all utilities as it prepends custom errors to the error description for the respective application and error type. This saves the developers from manually appending the `std::io::Error`-specific error messages. --- src/uu/install/src/install.rs | 36 ++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index 7de69df00..2186738c4 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -5,7 +5,7 @@ // * For the full copyright and license information, please view the LICENSE file // * that was distributed with this source code. -// spell-checker:ignore (ToDO) rwxr sourcepath targetpath Isnt +// spell-checker:ignore (ToDO) rwxr sourcepath targetpath Isnt uioerror mod mode; @@ -57,7 +57,7 @@ enum InstallError { ChmodFailed(PathBuf), InvalidTarget(PathBuf), TargetDirIsntDir(PathBuf), - BackupFailed(PathBuf, PathBuf, String), + BackupFailed(PathBuf, PathBuf, std::io::Error), InstallFailed(PathBuf, PathBuf, std::io::Error), StripProgramFailed(String), MetadataFailed(std::io::Error), @@ -91,7 +91,9 @@ impl Display for InstallError { "{} with -d requires at least one argument.", executable!() ), - IE::CreateDirFailed(dir, e) => write!(f, "failed to create {}: {}", dir.display(), e), + IE::CreateDirFailed(dir, e) => { + Display::fmt(&uio_error!(e, "failed to create {}", dir.display()), f) + } IE::ChmodFailed(file) => write!(f, "failed to chmod {}", file.display()), IE::InvalidTarget(target) => write!( f, @@ -101,22 +103,26 @@ impl Display for InstallError { IE::TargetDirIsntDir(target) => { write!(f, "target '{}' is not a directory", target.display()) } - IE::BackupFailed(from, to, e) => write!( + IE::BackupFailed(from, to, e) => Display::fmt( + &uio_error!( + e, + "cannot backup '{}' to '{}'", + from.display(), + to.display() + ), f, - "cannot backup file '{}' to '{}': {}", - from.display(), - to.display(), - e ), - IE::InstallFailed(from, to, e) => write!( + IE::InstallFailed(from, to, e) => Display::fmt( + &uio_error!( + e, + "cannot install '{}' to '{}'", + from.display(), + to.display() + ), f, - "cannot install '{}' to '{}': {}", - from.display(), - to.display(), - e ), IE::StripProgramFailed(msg) => write!(f, "strip program failed: {}", msg), - IE::MetadataFailed(e) => write!(f, "{}", e.to_string()), + IE::MetadataFailed(e) => Display::fmt(&uio_error!(e, ""), f), IE::NoSuchUser(user) => write!(f, "no such user: {}", user), IE::NoSuchGroup(group) => write!(f, "no such group: {}", group), IE::OmittingDirectory(dir) => write!(f, "omitting directory '{}'", dir.display()), @@ -594,7 +600,7 @@ fn copy(from: &Path, to: &Path, b: &Behavior) -> UResult<()> { return Err(InstallError::BackupFailed( to.to_path_buf(), backup_path.to_path_buf(), - err.to_string(), + err, ) .into()); } From ce037bdf55bc530768c9e45e2c5da632fab80c7e Mon Sep 17 00:00:00 2001 From: Andreas Hartmann Date: Sun, 25 Jul 2021 09:40:37 +0200 Subject: [PATCH 5/5] install: remove obsolete function call The `copy_file_to_file` function was only a wrapper to a `copy` function with the exact same interface. It has now beed removed for clarity. --- src/uu/install/src/install.rs | 20 ++------------------ 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index 2186738c4..8ce219f7a 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -502,7 +502,7 @@ fn standard(mut paths: Vec, b: Behavior) -> UResult<()> { } if target.is_file() || is_new_file_path(&target) { - copy_file_to_file(&sources[0], &target, &b) + copy(&sources[0], &target, &b) } else { Err(InstallError::InvalidTarget(target).into()) } @@ -543,9 +543,7 @@ fn copy_files_into_dir(files: &[PathBuf], target_dir: &Path, b: &Behavior) -> UR let filename = sourcepath.components().last().unwrap(); targetpath.push(filename); - if let Err(e) = copy(sourcepath, &targetpath, b) { - show!(e); - } + show_if_err!(copy(sourcepath, &targetpath, b)); } // If the exit code was set, or show! has been called at least once // (which sets the exit code as well), function execution will end after @@ -553,20 +551,6 @@ fn copy_files_into_dir(files: &[PathBuf], target_dir: &Path, b: &Behavior) -> UR Ok(()) } -/// Copy a file to another file. -/// -/// Prints verbose information and error messages. -/// Returns a Result type with the Err variant containing the error message. -/// -/// # Parameters -/// -/// _file_ must exist as a non-directory. -/// _target_ must be a non-directory -/// -fn copy_file_to_file(file: &Path, target: &Path, b: &Behavior) -> UResult<()> { - copy(file, target, b) -} - /// Copy one file to a new location, changing metadata. /// /// Returns a Result type with the Err variant containing the error message.