From 3024ade0717ec3406582c3d04e4a3faffd149be8 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Wed, 18 Nov 2020 23:06:43 +0100 Subject: [PATCH] refactor(chgrp, install): Show the error in the program instead of the lib --- src/uu/chgrp/src/chgrp.rs | 31 ++++++++++++++++++-- src/uu/chown/src/chown.rs | 35 ++++++++++++++++++++--- src/uu/install/src/install.rs | 20 +++++++++++-- src/uucore/src/lib/features/perms.rs | 42 ++++++++++++++++------------ tests/by-util/test_chgrp.rs | 3 +- 5 files changed, 101 insertions(+), 30 deletions(-) diff --git a/src/uu/chgrp/src/chgrp.rs b/src/uu/chgrp/src/chgrp.rs index 9fc72ab20..b4c3360c5 100644 --- a/src/uu/chgrp/src/chgrp.rs +++ b/src/uu/chgrp/src/chgrp.rs @@ -239,13 +239,24 @@ impl Chgrper { } } - let ret = wrap_chgrp( + let ret = match wrap_chgrp( path, &meta, self.dest_gid, follow_arg, self.verbosity.clone(), - ); + ) { + Ok(n) => { + show_info!("{}", n); + 0 + } + Err(e) => { + if self.verbosity != Verbosity::Silent { + show_info!("{}", e); + } + 1 + } + }; if !self.recursive { ret @@ -273,8 +284,22 @@ impl Chgrper { } }; - ret = wrap_chgrp(path, &meta, self.dest_gid, follow, self.verbosity.clone()); + ret = match wrap_chgrp(path, &meta, self.dest_gid, follow, self.verbosity.clone()) { + Ok(n) => { + if n != "" { + show_info!("{}", n); + } + 0 + } + Err(e) => { + if self.verbosity != Verbosity::Silent { + show_info!("{}", e); + } + 1 + } + } } + ret } diff --git a/src/uu/chown/src/chown.rs b/src/uu/chown/src/chown.rs index 8dba8e174..47279747f 100644 --- a/src/uu/chown/src/chown.rs +++ b/src/uu/chown/src/chown.rs @@ -375,14 +375,28 @@ impl Chowner { } let ret = if self.matched(meta.uid(), meta.gid()) { - wrap_chown( + match wrap_chown( path, &meta, self.dest_uid, self.dest_gid, follow_arg, self.verbosity.clone(), - ) + ) { + Ok(n) => { + if n != "" { + show_info!("{}", n); + } + 0 + } + Err(e) => { + + if self.verbosity != Verbosity::Silent { + show_info!("{}", e); + } + 1 + } + } } else { 0 }; @@ -417,14 +431,27 @@ impl Chowner { continue; } - ret = wrap_chown( + ret = match wrap_chown( path, &meta, self.dest_uid, self.dest_gid, follow, self.verbosity.clone(), - ) + ) { + Ok(n) => { + if n != "" { + show_info!("{}", n); + } + 0 + } + Err(e) => { + if self.verbosity != Verbosity::Silent { + show_info!("{}", e); + } + 1 + } + } } ret } diff --git a/src/uu/install/src/install.rs b/src/uu/install/src/install.rs index 6260baa29..ce6ba3bf1 100644 --- a/src/uu/install/src/install.rs +++ b/src/uu/install/src/install.rs @@ -516,14 +516,21 @@ fn copy(from: &PathBuf, to: &PathBuf, b: &Behavior) -> Result<(), ()> { _ => crash!(1, "no such user: {}", b.owner), }; let gid = meta.gid(); - wrap_chown( + match wrap_chown( to.as_path(), &meta, Some(owner_id), Some(gid), false, Verbosity::Normal, - ); + ) { + Ok(n) => { + if n != "" { + show_info!("{}", n); + } + } + Err(e) => show_info!("{}", e), + } } if b.group != "" { @@ -536,7 +543,14 @@ fn copy(from: &PathBuf, to: &PathBuf, b: &Behavior) -> Result<(), ()> { Ok(g) => g, _ => crash!(1, "no such group: {}", b.group), }; - wrap_chgrp(to.as_path(), &meta, group_id, false, Verbosity::Normal); + match wrap_chgrp(to.as_path(), &meta, group_id, false, Verbosity::Normal) { + Ok(n) => { + if n != "" { + show_info!("{}", n); + } + } + Err(e) => show_info!("{}", e), + } } if b.verbose { diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index 6ea1cfe8f..25f49012c 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -19,6 +19,8 @@ use std::os::unix::fs::MetadataExt; use std::os::unix::ffi::OsStrExt; use std::path::Path; +//type PermResult = Result; + #[derive(PartialEq, Clone, Debug)] pub enum Verbosity { Silent, @@ -50,18 +52,20 @@ pub fn wrap_chgrp>( dest_gid: gid_t, follow: bool, verbosity: Verbosity, -) -> i32 { +) -> Result { use self::Verbosity::*; - let mut ret = 0; let path = path.as_ref(); + let mut out: String = String::new(); + if let Err(e) = chgrp(path, dest_gid, follow) { match verbosity { Silent => (), _ => { - show_info!("changing group of '{}': {}", path.display(), e); + out = format!("changing group of '{}': {}", path.display(), e); if verbosity == Verbose { - println!( - "failed to change group of {} from {} to {}", + out = format!( + "{}\nfailed to change group of {} from {} to {}", + out, path.display(), entries::gid2grp(meta.gid()).unwrap(), entries::gid2grp(dest_gid).unwrap() @@ -69,13 +73,13 @@ pub fn wrap_chgrp>( }; } } - ret = 1; + return Err(out); } else { let changed = dest_gid != meta.gid(); if changed { match verbosity { Changes | Verbose => { - println!( + out = format!( "changed group of {} from {} to {}", path.display(), entries::gid2grp(meta.gid()).unwrap(), @@ -85,14 +89,14 @@ pub fn wrap_chgrp>( _ => (), }; } else if verbosity == Verbose { - println!( + out = format!( "group of {} retained as {}", path.display(), entries::gid2grp(dest_gid).unwrap() ); } } - ret + Ok(out) } fn chown>(path: P, duid: uid_t, dgid: gid_t, follow: bool) -> IOResult<()> { @@ -119,20 +123,22 @@ pub fn wrap_chown>( dest_gid: Option, follow: bool, verbosity: Verbosity, -) -> i32 { +) -> Result { use self::Verbosity::*; - let mut ret = 0; let dest_uid = dest_uid.unwrap_or_else(|| meta.uid()); let dest_gid = dest_gid.unwrap_or_else(|| meta.gid()); let path = path.as_ref(); + let mut out: String = String::new(); + if let Err(e) = chown(path, dest_uid, dest_gid, follow) { match verbosity { Silent => (), _ => { - show_info!("changing ownership of '{}': {}", path.display(), e); + out = format!("changing ownership of '{}': {}", path.display(), e); if verbosity == Verbose { - println!( - "failed to change ownership of {} from {}:{} to {}:{}", + out = format!( + "{}\nfailed to change ownership of {} from {}:{} to {}:{}", + out, path.display(), entries::uid2usr(meta.uid()).unwrap(), entries::gid2grp(meta.gid()).unwrap(), @@ -142,13 +148,13 @@ pub fn wrap_chown>( }; } } - ret = 1; + return Err(out); } else { let changed = dest_uid != meta.uid() || dest_gid != meta.gid(); if changed { match verbosity { Changes | Verbose => { - println!( + out = format!( "changed ownership of {} from {}:{} to {}:{}", path.display(), entries::uid2usr(meta.uid()).unwrap(), @@ -160,7 +166,7 @@ pub fn wrap_chown>( _ => (), }; } else if verbosity == Verbose { - println!( + out = format!( "ownership of {} retained as {}:{}", path.display(), entries::uid2usr(dest_uid).unwrap(), @@ -168,5 +174,5 @@ pub fn wrap_chown>( ); } } - ret + Ok(out) } diff --git a/tests/by-util/test_chgrp.rs b/tests/by-util/test_chgrp.rs index 3bd0c69e5..adc0f6981 100644 --- a/tests/by-util/test_chgrp.rs +++ b/tests/by-util/test_chgrp.rs @@ -110,8 +110,7 @@ fn test_reference() { .arg("--reference=/etc/passwd") .arg("/etc") .fails() - .stderr_is("chgrp: changing group of '/etc': Operation not permitted (os error 1)\n") - .stdout_is("failed to change group of /etc from root to root\n"); + .stderr_is("chgrp: changing group of '/etc': Operation not permitted (os error 1)\nfailed to change group of /etc from root to root"); } }