From 0130a07579b737a28c307801232b09ed9f211258 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mo=C3=AFse=20Valvassori?= Date: Sun, 21 May 2023 17:54:32 +0200 Subject: [PATCH] chown: show message if file doesn't exist * print the message "failed to change ownership of" when we try to change a non existing file. * replace the 4-tuple returned by parse_gid_uid_and_filter by GidUidOwnerFilter struct. * chain the test in one expression. * chown: remove unused var "result" in test --------- Co-authored-by: Daniel Hofstetter --- src/uu/chgrp/src/chgrp.rs | 19 ++++++++++++++---- src/uu/chown/src/chown.rs | 29 +++++++++++++++++++++------ src/uucore/src/lib/features/perms.rs | 30 ++++++++++++++++++++++++---- tests/by-util/test_chown.rs | 12 +++++------ 4 files changed, 70 insertions(+), 20 deletions(-) diff --git a/src/uu/chgrp/src/chgrp.rs b/src/uu/chgrp/src/chgrp.rs index e63dd9eb7d7..bd01e132b47 100644 --- a/src/uu/chgrp/src/chgrp.rs +++ b/src/uu/chgrp/src/chgrp.rs @@ -10,7 +10,7 @@ use uucore::display::Quotable; pub use uucore::entries; use uucore::error::{FromIo, UResult, USimpleError}; -use uucore::perms::{chown_base, options, IfFrom}; +use uucore::perms::{chown_base, options, GidUidOwnerFilter, IfFrom}; use uucore::{format_usage, help_about, help_usage}; use clap::{crate_version, Arg, ArgAction, ArgMatches, Command}; @@ -21,16 +21,22 @@ use std::os::unix::fs::MetadataExt; const ABOUT: &str = help_about!("chgrp.md"); const USAGE: &str = help_usage!("chgrp.md"); -fn parse_gid_and_uid(matches: &ArgMatches) -> UResult<(Option, Option, IfFrom)> { +fn parse_gid_and_uid(matches: &ArgMatches) -> UResult { + let mut raw_group: String = String::new(); let dest_gid = if let Some(file) = matches.get_one::(options::REFERENCE) { fs::metadata(file) - .map(|meta| Some(meta.gid())) + .map(|meta| { + let gid = meta.gid(); + raw_group = entries::gid2grp(gid).unwrap_or_else(|_| gid.to_string()); + Some(gid) + }) .map_err_context(|| format!("failed to get attributes of {}", file.quote()))? } else { let group = matches .get_one::(options::ARG_GROUP) .map(|s| s.as_str()) .unwrap_or_default(); + raw_group = group.to_string(); if group.is_empty() { None } else { @@ -45,7 +51,12 @@ fn parse_gid_and_uid(matches: &ArgMatches) -> UResult<(Option, Option, } } }; - Ok((dest_gid, None, IfFrom::All)) + Ok(GidUidOwnerFilter { + dest_gid, + dest_uid: None, + raw_owner: raw_group, + filter: IfFrom::All, + }) } #[uucore::main] diff --git a/src/uu/chown/src/chown.rs b/src/uu/chown/src/chown.rs index 2d810564e33..67e71b815b9 100644 --- a/src/uu/chown/src/chown.rs +++ b/src/uu/chown/src/chown.rs @@ -9,7 +9,7 @@ use uucore::display::Quotable; pub use uucore::entries::{self, Group, Locate, Passwd}; -use uucore::perms::{chown_base, options, IfFrom}; +use uucore::perms::{chown_base, options, GidUidOwnerFilter, IfFrom}; use uucore::{format_usage, help_about, help_usage}; use uucore::error::{FromIo, UResult, USimpleError}; @@ -23,7 +23,7 @@ static ABOUT: &str = help_about!("chown.md"); const USAGE: &str = help_usage!("chown.md"); -fn parse_gid_uid_and_filter(matches: &ArgMatches) -> UResult<(Option, Option, IfFrom)> { +fn parse_gid_uid_and_filter(matches: &ArgMatches) -> UResult { let filter = if let Some(spec) = matches.get_one::(options::FROM) { match parse_spec(spec, ':')? { (Some(uid), None) => IfFrom::User(uid), @@ -37,17 +37,34 @@ fn parse_gid_uid_and_filter(matches: &ArgMatches) -> UResult<(Option, Optio let dest_uid: Option; let dest_gid: Option; + let raw_owner: String; if let Some(file) = matches.get_one::(options::REFERENCE) { let meta = fs::metadata(file) .map_err_context(|| format!("failed to get attributes of {}", file.quote()))?; - dest_gid = Some(meta.gid()); - dest_uid = Some(meta.uid()); + let gid = meta.gid(); + let uid = meta.uid(); + dest_gid = Some(gid); + dest_uid = Some(uid); + raw_owner = format!( + "{}:{}", + entries::uid2usr(uid).unwrap_or_else(|_| uid.to_string()), + entries::gid2grp(gid).unwrap_or_else(|_| gid.to_string()) + ); } else { - let (u, g) = parse_spec(matches.get_one::(options::ARG_OWNER).unwrap(), ':')?; + raw_owner = matches + .get_one::(options::ARG_OWNER) + .unwrap() + .into(); + let (u, g) = parse_spec(&raw_owner, ':')?; dest_uid = u; dest_gid = g; } - Ok((dest_gid, dest_uid, filter)) + Ok(GidUidOwnerFilter { + dest_gid, + dest_uid, + raw_owner, + filter, + }) } #[uucore::main] diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index 984d6dd402d..6383477371e 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -182,6 +182,7 @@ pub enum TraverseSymlinks { pub struct ChownExecutor { pub dest_uid: Option, pub dest_gid: Option, + pub raw_owner: String, // The owner of the file as input by the user in the command line. pub traverse_symlinks: TraverseSymlinks, pub verbosity: Verbosity, pub filter: IfFrom, @@ -208,7 +209,16 @@ impl ChownExecutor { let path = root.as_ref(); let meta = match self.obtain_meta(path, self.dereference) { Some(m) => m, - _ => return 1, + _ => { + if self.verbosity.level == VerbosityLevel::Verbose { + println!( + "failed to change ownership of {} to {}", + path.quote(), + self.raw_owner + ); + } + return 1; + } }; // Prohibit only if: @@ -414,7 +424,13 @@ pub mod options { pub const ARG_FILES: &str = "FILE"; } -type GidUidFilterParser = fn(&ArgMatches) -> UResult<(Option, Option, IfFrom)>; +pub struct GidUidOwnerFilter { + pub dest_gid: Option, + pub dest_uid: Option, + pub raw_owner: String, + pub filter: IfFrom, +} +type GidUidFilterOwnerParser = fn(&ArgMatches) -> UResult; /// Base implementation for `chgrp` and `chown`. /// @@ -428,7 +444,7 @@ pub fn chown_base( mut command: Command, args: impl crate::Args, add_arg_if_not_reference: &'static str, - parse_gid_uid_and_filter: GidUidFilterParser, + parse_gid_uid_and_filter: GidUidFilterOwnerParser, groups_only: bool, ) -> UResult<()> { let args: Vec<_> = args.collect(); @@ -511,12 +527,18 @@ pub fn chown_base( } else { VerbosityLevel::Normal }; - let (dest_gid, dest_uid, filter) = parse_gid_uid_and_filter(&matches)?; + let GidUidOwnerFilter { + dest_gid, + dest_uid, + raw_owner, + filter, + } = parse_gid_uid_and_filter(&matches)?; let executor = ChownExecutor { traverse_symlinks, dest_gid, dest_uid, + raw_owner, verbosity: Verbosity { groups_only, level: verbosity_level, diff --git a/tests/by-util/test_chown.rs b/tests/by-util/test_chown.rs index fce4fa6359f..fd7e377224b 100644 --- a/tests/by-util/test_chown.rs +++ b/tests/by-util/test_chown.rs @@ -730,15 +730,15 @@ fn test_chown_file_notexisting() { let user_name = String::from(result.stdout_str().trim()); assert!(!user_name.is_empty()); - let _result = scene + scene .ucmd() - .arg(user_name) + .arg(&user_name) .arg("--verbose") .arg("not_existing") - .fails(); - - // TODO: uncomment once "failed to change ownership of '{}' to {}" added to stdout - // result.stderr_contains("retained as"); + .fails() + .stdout_contains(format!( + "failed to change ownership of 'not_existing' to {user_name}" + )); // TODO: uncomment once message changed from "cannot dereference" to "cannot access" // result.stderr_contains("cannot access 'not_existing': No such file or directory"); }