From 195f827cd46929f4dedba341442151e1291bdbf7 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Thu, 2 Sep 2021 13:12:42 +0200 Subject: [PATCH 1/4] chown/chgrp: share more code Also share argument parsing code between `chgrp` and `chown` --- src/uu/chgrp/src/chgrp.rs | 158 +++---------------------- src/uu/chown/src/chown.rs | 165 ++++++--------------------- src/uucore/src/lib/features/perms.rs | 153 ++++++++++++++++++++++++- 3 files changed, 204 insertions(+), 272 deletions(-) diff --git a/src/uu/chgrp/src/chgrp.rs b/src/uu/chgrp/src/chgrp.rs index d37da578e..a077ab7a4 100644 --- a/src/uu/chgrp/src/chgrp.rs +++ b/src/uu/chgrp/src/chgrp.rs @@ -11,46 +11,16 @@ extern crate uucore; pub use uucore::entries; use uucore::error::{FromIo, UResult, USimpleError}; -use uucore::perms::{ - ChownExecutor, IfFrom, Verbosity, VerbosityLevel, FTS_COMFOLLOW, FTS_LOGICAL, FTS_PHYSICAL, -}; +use uucore::perms::{chown_base, options, IfFrom}; -use clap::{App, Arg}; +use clap::{App, Arg, ArgMatches}; use std::fs; use std::os::unix::fs::MetadataExt; -use uucore::InvalidEncodingHandling; - static ABOUT: &str = "Change the group of each FILE to GROUP."; static VERSION: &str = env!("CARGO_PKG_VERSION"); -pub mod options { - pub mod verbosity { - pub static CHANGES: &str = "changes"; - pub static QUIET: &str = "quiet"; - pub static SILENT: &str = "silent"; - pub static VERBOSE: &str = "verbose"; - } - pub mod preserve_root { - pub static PRESERVE: &str = "preserve-root"; - pub static NO_PRESERVE: &str = "no-preserve-root"; - } - pub mod dereference { - pub static DEREFERENCE: &str = "dereference"; - pub static NO_DEREFERENCE: &str = "no-dereference"; - } - pub static RECURSIVE: &str = "recursive"; - pub mod traverse { - pub static TRAVERSE: &str = "H"; - pub static NO_TRAVERSE: &str = "P"; - pub static EVERY: &str = "L"; - } - pub static REFERENCE: &str = "reference"; - pub static ARG_GROUP: &str = "GROUP"; - pub static ARG_FILES: &str = "FILE"; -} - fn get_usage() -> String { format!( "{0} [OPTION]... GROUP FILE...\n {0} [OPTION]... --reference=RFILE FILE...", @@ -58,101 +28,7 @@ fn get_usage() -> String { ) } -#[uucore_procs::gen_uumain] -pub fn uumain(args: impl uucore::Args) -> UResult<()> { - let args = args - .collect_str(InvalidEncodingHandling::ConvertLossy) - .accept_any(); - - let usage = get_usage(); - - let mut app = uu_app().usage(&usage[..]); - - // we change the positional args based on whether - // --reference was used. - let mut reference = false; - let mut help = false; - // stop processing options on -- - for arg in args.iter().take_while(|s| *s != "--") { - if arg.starts_with("--reference=") || arg == "--reference" { - reference = true; - } else if arg == "--help" { - // we stop processing once we see --help, - // as it doesn't matter if we've seen reference or not - help = true; - break; - } - } - - if help || !reference { - // add both positional arguments - app = app.arg( - Arg::with_name(options::ARG_GROUP) - .value_name(options::ARG_GROUP) - .required(true) - .takes_value(true) - .multiple(false), - ) - } - app = app.arg( - Arg::with_name(options::ARG_FILES) - .value_name(options::ARG_FILES) - .multiple(true) - .takes_value(true) - .required(true) - .min_values(1), - ); - - let matches = app.get_matches_from(args); - - /* Get the list of files */ - let files: Vec = matches - .values_of(options::ARG_FILES) - .map(|v| v.map(ToString::to_string).collect()) - .unwrap_or_default(); - - let preserve_root = matches.is_present(options::preserve_root::PRESERVE); - - let mut derefer = if matches.is_present(options::dereference::DEREFERENCE) { - 1 - } else if matches.is_present(options::dereference::NO_DEREFERENCE) { - 0 - } else { - -1 - }; - - let mut bit_flag = if matches.is_present(options::traverse::TRAVERSE) { - FTS_COMFOLLOW | FTS_PHYSICAL - } else if matches.is_present(options::traverse::EVERY) { - FTS_LOGICAL - } else { - FTS_PHYSICAL - }; - - let recursive = matches.is_present(options::RECURSIVE); - if recursive { - if bit_flag == FTS_PHYSICAL { - if derefer == 1 { - return Err(USimpleError::new(1, "-R --dereference requires -H or -L")); - } - derefer = 0; - } - } else { - bit_flag = FTS_PHYSICAL; - } - - let verbosity_level = if matches.is_present(options::verbosity::CHANGES) { - VerbosityLevel::Changes - } else if matches.is_present(options::verbosity::SILENT) - || matches.is_present(options::verbosity::QUIET) - { - VerbosityLevel::Silent - } else if matches.is_present(options::verbosity::VERBOSE) { - VerbosityLevel::Verbose - } else { - VerbosityLevel::Normal - }; - +fn parse_gid_and_uid(matches: &ArgMatches) -> UResult<(Option, Option, IfFrom)> { let dest_gid = if let Some(file) = matches.value_of(options::REFERENCE) { fs::metadata(&file) .map(|meta| Some(meta.gid())) @@ -168,22 +44,20 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { } } }; + Ok((dest_gid, None, IfFrom::All)) +} - let executor = ChownExecutor { - bit_flag, - dest_gid, - verbosity: Verbosity { - groups_only: true, - level: verbosity_level, - }, - recursive, - dereference: derefer != 0, - preserve_root, - files, - filter: IfFrom::All, - dest_uid: None, - }; - executor.exec() +#[uucore_procs::gen_uumain] +pub fn uumain(args: impl uucore::Args) -> UResult<()> { + let usage = get_usage(); + + chown_base( + uu_app().usage(&usage[..]), + args, + options::ARG_GROUP, + parse_gid_and_uid, + true, + ) } pub fn uu_app() -> App<'static, 'static> { diff --git a/src/uu/chown/src/chown.rs b/src/uu/chown/src/chown.rs index 06f0c6a32..20f3f8174 100644 --- a/src/uu/chown/src/chown.rs +++ b/src/uu/chown/src/chown.rs @@ -10,49 +10,17 @@ #[macro_use] extern crate uucore; pub use uucore::entries::{self, Group, Locate, Passwd}; -use uucore::perms::{ - ChownExecutor, IfFrom, Verbosity, VerbosityLevel, FTS_COMFOLLOW, FTS_LOGICAL, FTS_PHYSICAL, -}; +use uucore::perms::{chown_base, options, IfFrom}; use uucore::error::{FromIo, UResult, USimpleError}; -use clap::{crate_version, App, Arg}; +use clap::{crate_version, App, Arg, ArgMatches}; use std::fs; use std::os::unix::fs::MetadataExt; -use uucore::InvalidEncodingHandling; - static ABOUT: &str = "change file owner and group"; -pub mod options { - pub mod verbosity { - pub static CHANGES: &str = "changes"; - pub static QUIET: &str = "quiet"; - pub static SILENT: &str = "silent"; - pub static VERBOSE: &str = "verbose"; - } - pub mod preserve_root { - pub static PRESERVE: &str = "preserve-root"; - pub static NO_PRESERVE: &str = "no-preserve-root"; - } - pub mod dereference { - pub static DEREFERENCE: &str = "dereference"; - pub static NO_DEREFERENCE: &str = "no-dereference"; - } - pub static FROM: &str = "from"; - pub static RECURSIVE: &str = "recursive"; - pub mod traverse { - pub static TRAVERSE: &str = "H"; - pub static NO_TRAVERSE: &str = "P"; - pub static EVERY: &str = "L"; - } - pub static REFERENCE: &str = "reference"; -} - -static ARG_OWNER: &str = "owner"; -static ARG_FILES: &str = "files"; - fn get_usage() -> String { format!( "{0} [OPTION]... [OWNER][:[GROUP]] FILE...\n{0} [OPTION]... --reference=RFILE FILE...", @@ -60,65 +28,7 @@ fn get_usage() -> String { ) } -#[uucore_procs::gen_uumain] -pub fn uumain(args: impl uucore::Args) -> UResult<()> { - let args = args - .collect_str(InvalidEncodingHandling::Ignore) - .accept_any(); - - let usage = get_usage(); - - let matches = uu_app().usage(&usage[..]).get_matches_from(args); - - /* First arg is the owner/group */ - let owner = matches.value_of(ARG_OWNER).unwrap(); - - /* Then the list of files */ - let files: Vec = matches - .values_of(ARG_FILES) - .map(|v| v.map(ToString::to_string).collect()) - .unwrap_or_default(); - - let preserve_root = matches.is_present(options::preserve_root::PRESERVE); - - let mut derefer = if matches.is_present(options::dereference::NO_DEREFERENCE) { - 1 - } else { - 0 - }; - - let mut bit_flag = if matches.is_present(options::traverse::TRAVERSE) { - FTS_COMFOLLOW | FTS_PHYSICAL - } else if matches.is_present(options::traverse::EVERY) { - FTS_LOGICAL - } else { - FTS_PHYSICAL - }; - - let recursive = matches.is_present(options::RECURSIVE); - if recursive { - if bit_flag == FTS_PHYSICAL { - if derefer == 1 { - return Err(USimpleError::new(1, "-R --dereference requires -H or -L")); - } - derefer = 0; - } - } else { - bit_flag = FTS_PHYSICAL; - } - - let verbosity = if matches.is_present(options::verbosity::CHANGES) { - VerbosityLevel::Changes - } else if matches.is_present(options::verbosity::SILENT) - || matches.is_present(options::verbosity::QUIET) - { - VerbosityLevel::Silent - } else if matches.is_present(options::verbosity::VERBOSE) { - VerbosityLevel::Verbose - } else { - VerbosityLevel::Normal - }; - +fn parse_gid_uid_and_filter(matches: &ArgMatches) -> UResult<(Option, Option, IfFrom)> { let filter = if let Some(spec) = matches.value_of(options::FROM) { match parse_spec(spec)? { (Some(uid), None) => IfFrom::User(uid), @@ -138,25 +48,24 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { dest_gid = Some(meta.gid()); dest_uid = Some(meta.uid()); } else { - let (u, g) = parse_spec(owner)?; + let (u, g) = parse_spec(matches.value_of(options::ARG_OWNER).unwrap())?; dest_uid = u; dest_gid = g; } - let executor = ChownExecutor { - bit_flag, - dest_uid, - dest_gid, - verbosity: Verbosity { - groups_only: false, - level: verbosity, - }, - recursive, - dereference: derefer != 0, - filter, - preserve_root, - files, - }; - executor.exec() + Ok((dest_gid, dest_uid, filter)) +} + +#[uucore_procs::gen_uumain] +pub fn uumain(args: impl uucore::Args) -> UResult<()> { + let usage = get_usage(); + + chown_base( + uu_app().usage(&usage[..]), + args, + options::ARG_OWNER, + parse_gid_uid_and_filter, + false, + ) } pub fn uu_app() -> App<'static, 'static> { @@ -169,22 +78,31 @@ pub fn uu_app() -> App<'static, 'static> { .long(options::verbosity::CHANGES) .help("like verbose but report only when a change is made"), ) - .arg(Arg::with_name(options::dereference::DEREFERENCE).long(options::dereference::DEREFERENCE).help( - "affect the referent of each symbolic link (this is the default), rather than the symbolic link itself", - )) + .arg( + Arg::with_name(options::dereference::DEREFERENCE) + .long(options::dereference::DEREFERENCE) + .help( + "affect the referent of each symbolic link (this is the default), \ + rather than the symbolic link itself", + ), + ) .arg( Arg::with_name(options::dereference::NO_DEREFERENCE) .short("h") .long(options::dereference::NO_DEREFERENCE) .help( - "affect symbolic links instead of any referenced file (useful only on systems that can change the ownership of a symlink)", + "affect symbolic links instead of any referenced file \ + (useful only on systems that can change the ownership of a symlink)", ), ) .arg( Arg::with_name(options::FROM) .long(options::FROM) .help( - "change the owner and/or group of each file only if its current owner and/or group match those specified here. Either may be omitted, in which case a match is not required for the omitted attribute", + "change the owner and/or group of each file only if its \ + current owner and/or group match those specified here. \ + Either may be omitted, in which case a match is not required \ + for the omitted attribute", ) .value_name("CURRENT_OWNER:CURRENT_GROUP"), ) @@ -216,7 +134,11 @@ pub fn uu_app() -> App<'static, 'static> { .value_name("RFILE") .min_values(1), ) - .arg(Arg::with_name(options::verbosity::SILENT).short("f").long(options::verbosity::SILENT)) + .arg( + Arg::with_name(options::verbosity::SILENT) + .short("f") + .long(options::verbosity::SILENT), + ) .arg( Arg::with_name(options::traverse::TRAVERSE) .short(options::traverse::TRAVERSE) @@ -240,19 +162,6 @@ pub fn uu_app() -> App<'static, 'static> { .long(options::verbosity::VERBOSE) .help("output a diagnostic for every file processed"), ) - .arg( - Arg::with_name(ARG_OWNER) - .multiple(false) - .takes_value(true) - .required(true), - ) - .arg( - Arg::with_name(ARG_FILES) - .multiple(true) - .takes_value(true) - .required(true) - .min_values(1), - ) } fn parse_spec(spec: &str) -> UResult<(Option, Option)> { diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index fe1c97a82..5f470d5a8 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -7,10 +7,14 @@ use crate::error::strip_errno; use crate::error::UResult; +use crate::error::USimpleError; pub use crate::features::entries; use crate::fs::resolve_relative_path; use crate::show_error; -use libc::{self, gid_t, lchown, uid_t}; +use clap::App; +use clap::Arg; +use clap::ArgMatches; +use libc::{self, gid_t, uid_t}; use walkdir::WalkDir; use std::io::Error as IOError; @@ -45,7 +49,7 @@ fn chown>(path: P, uid: uid_t, gid: gid_t, follow: bool) -> IORes if follow { libc::chown(s.as_ptr(), uid, gid) } else { - lchown(s.as_ptr(), uid, gid) + libc::lchown(s.as_ptr(), uid, gid) } }; if ret == 0 { @@ -359,3 +363,148 @@ impl ChownExecutor { } } } + +pub mod options { + pub mod verbosity { + pub const CHANGES: &str = "changes"; + pub const QUIET: &str = "quiet"; + pub const SILENT: &str = "silent"; + pub const VERBOSE: &str = "verbose"; + } + pub mod preserve_root { + pub const PRESERVE: &str = "preserve-root"; + pub const NO_PRESERVE: &str = "no-preserve-root"; + } + pub mod dereference { + pub const DEREFERENCE: &str = "dereference"; + pub const NO_DEREFERENCE: &str = "no-dereference"; + } + pub const FROM: &str = "from"; + pub const RECURSIVE: &str = "recursive"; + pub mod traverse { + pub const TRAVERSE: &str = "H"; + pub const NO_TRAVERSE: &str = "P"; + pub const EVERY: &str = "L"; + } + pub const REFERENCE: &str = "reference"; + pub const ARG_OWNER: &str = "OWNER"; + pub const ARG_GROUP: &str = "GROUP"; + pub const ARG_FILES: &str = "FILE"; +} + +type GidUidFilterParser<'a> = fn(&ArgMatches<'a>) -> UResult<(Option, Option, IfFrom)>; + +/// Base implementation for `chgrp` and `chown`. +/// +/// An argument called `add_arg_if_not_reference` will be added to `app` if +/// `args` does not contain the `--reference` option. +/// `parse_gid_uid_and_filter` will be called to obtain the target gid and uid, and the filter, +/// from `ArgMatches`. +/// `groups_only` determines whether verbose output will only mention the group. +pub fn chown_base<'a>( + mut app: App<'a, 'a>, + args: impl crate::Args, + add_arg_if_not_reference: &'a str, + parse_gid_uid_and_filter: GidUidFilterParser<'a>, + groups_only: bool, +) -> UResult<()> { + let args: Vec<_> = args.collect(); + let mut reference = false; + let mut help = false; + // stop processing options on -- + for arg in args.iter().take_while(|s| *s != "--") { + if arg.to_string_lossy().starts_with("--reference=") || arg == "--reference" { + reference = true; + } else if arg == "--help" { + // we stop processing once we see --help, + // as it doesn't matter if we've seen reference or not + help = true; + break; + } + } + + if help || !reference { + // add both positional arguments + // arg_group is only required if + app = app.arg( + Arg::with_name(add_arg_if_not_reference) + .value_name(add_arg_if_not_reference) + .required(true) + .takes_value(true) + .multiple(false), + ) + } + app = app.arg( + Arg::with_name(options::ARG_FILES) + .value_name(options::ARG_FILES) + .multiple(true) + .takes_value(true) + .required(true) + .min_values(1), + ); + let matches = app.get_matches_from(args); + + let files: Vec = matches + .values_of(options::ARG_FILES) + .map(|v| v.map(ToString::to_string).collect()) + .unwrap_or_default(); + + let preserve_root = matches.is_present(options::preserve_root::PRESERVE); + + let mut dereference = if matches.is_present(options::dereference::DEREFERENCE) { + 1 + } else if matches.is_present(options::dereference::NO_DEREFERENCE) { + 0 + } else { + -1 + }; + + let mut bit_flag = if matches.is_present(options::traverse::TRAVERSE) { + FTS_COMFOLLOW | FTS_PHYSICAL + } else if matches.is_present(options::traverse::EVERY) { + FTS_LOGICAL + } else { + FTS_PHYSICAL + }; + + let recursive = matches.is_present(options::RECURSIVE); + if recursive { + if bit_flag == FTS_PHYSICAL { + if dereference == 1 { + return Err(USimpleError::new(1, "-R --dereference requires -H or -L")); + } + dereference = 0; + } + } else { + bit_flag = FTS_PHYSICAL; + } + + let verbosity_level = if matches.is_present(options::verbosity::CHANGES) { + VerbosityLevel::Changes + } else if matches.is_present(options::verbosity::SILENT) + || matches.is_present(options::verbosity::QUIET) + { + VerbosityLevel::Silent + } else if matches.is_present(options::verbosity::VERBOSE) { + VerbosityLevel::Verbose + } else { + VerbosityLevel::Normal + }; + let (dest_gid, dest_uid, filter) = parse_gid_uid_and_filter(&matches)?; + + let executor = ChownExecutor { + bit_flag, + dest_gid, + dest_uid, + verbosity: Verbosity { + groups_only: true, + level: verbosity_level, + }, + recursive, + dereference: dereference != 0, + preserve_root, + files, + filter, + }; + executor.exec() +} From a4fca2d4fc4718909658f1b0ab83734a54c8d6bc Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Thu, 2 Sep 2021 18:27:01 +0200 Subject: [PATCH 2/4] uucore/perms: remove flags in favor of enums Part of the code was transliterated from GNU's implementation in C, and used flags to store settings. Instead, we can use enums to avoid magic values or binary operations to extract flags. --- src/uu/chown/src/chown.rs | 1 + src/uucore/src/lib/features/perms.rs | 56 ++++++++++++++-------------- 2 files changed, 30 insertions(+), 27 deletions(-) diff --git a/src/uu/chown/src/chown.rs b/src/uu/chown/src/chown.rs index 20f3f8174..4abb9ac61 100644 --- a/src/uu/chown/src/chown.rs +++ b/src/uu/chown/src/chown.rs @@ -160,6 +160,7 @@ pub fn uu_app() -> App<'static, 'static> { .arg( Arg::with_name(options::verbosity::VERBOSE) .long(options::verbosity::VERBOSE) + .short("v") .help("output a diagnostic for every file processed"), ) } diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index 5f470d5a8..b605c0cdf 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -165,22 +165,26 @@ pub enum IfFrom { UserGroup(u32, u32), } +#[derive(PartialEq, Eq)] +pub enum TraverseSymlinks { + None, + First, + All, +} + pub struct ChownExecutor { pub dest_uid: Option, pub dest_gid: Option, - pub bit_flag: u8, + pub traverse_symlinks: TraverseSymlinks, pub verbosity: Verbosity, pub filter: IfFrom, pub files: Vec, pub recursive: bool, pub preserve_root: bool, + // Must be true if traverse_symlinks is not None pub dereference: bool, } -pub const FTS_COMFOLLOW: u8 = 1; -pub const FTS_PHYSICAL: u8 = 1 << 1; -pub const FTS_LOGICAL: u8 = 1 << 2; - impl ChownExecutor { pub fn exec(&self) -> UResult<()> { let mut ret = 0; @@ -194,9 +198,8 @@ impl ChownExecutor { } fn traverse>(&self, root: P) -> i32 { - let follow_arg = self.dereference || self.bit_flag != FTS_PHYSICAL; let path = root.as_ref(); - let meta = match self.obtain_meta(path, follow_arg) { + let meta = match self.obtain_meta(path, self.dereference) { Some(m) => m, _ => return 1, }; @@ -208,7 +211,7 @@ impl ChownExecutor { // (argument is symlink && should follow argument && resolved to be '/') // ) if self.recursive && self.preserve_root { - let may_exist = if follow_arg { + let may_exist = if self.dereference { path.canonicalize().ok() } else { let real = resolve_relative_path(path); @@ -234,7 +237,7 @@ impl ChownExecutor { &meta, self.dest_uid, self.dest_gid, - follow_arg, + self.dereference, self.verbosity.clone(), ) { Ok(n) => { @@ -264,9 +267,8 @@ impl ChownExecutor { fn dive_into>(&self, root: P) -> i32 { let mut ret = 0; let root = root.as_ref(); - let follow = self.dereference || self.bit_flag & FTS_LOGICAL != 0; let mut iterator = WalkDir::new(root) - .follow_links(follow) + .follow_links(self.dereference) .min_depth(1) .into_iter(); // We can't use a for loop because we need to manipulate the iterator inside the loop. @@ -292,7 +294,7 @@ impl ChownExecutor { Ok(entry) => entry, }; let path = entry.path(); - let meta = match self.obtain_meta(path, follow) { + let meta = match self.obtain_meta(path, self.dereference) { Some(m) => m, _ => { ret = 1; @@ -314,7 +316,7 @@ impl ChownExecutor { &meta, self.dest_uid, self.dest_gid, - follow, + self.dereference, self.verbosity.clone(), ) { Ok(n) => { @@ -452,31 +454,31 @@ pub fn chown_base<'a>( let preserve_root = matches.is_present(options::preserve_root::PRESERVE); let mut dereference = if matches.is_present(options::dereference::DEREFERENCE) { - 1 + Some(true) } else if matches.is_present(options::dereference::NO_DEREFERENCE) { - 0 + Some(false) } else { - -1 + None }; - let mut bit_flag = if matches.is_present(options::traverse::TRAVERSE) { - FTS_COMFOLLOW | FTS_PHYSICAL + let mut traverse_symlinks = if matches.is_present(options::traverse::TRAVERSE) { + TraverseSymlinks::First } else if matches.is_present(options::traverse::EVERY) { - FTS_LOGICAL + TraverseSymlinks::All } else { - FTS_PHYSICAL + TraverseSymlinks::None }; let recursive = matches.is_present(options::RECURSIVE); if recursive { - if bit_flag == FTS_PHYSICAL { - if dereference == 1 { + if traverse_symlinks == TraverseSymlinks::None { + if dereference == Some(true) { return Err(USimpleError::new(1, "-R --dereference requires -H or -L")); } - dereference = 0; + dereference = Some(false); } } else { - bit_flag = FTS_PHYSICAL; + traverse_symlinks = TraverseSymlinks::None; } let verbosity_level = if matches.is_present(options::verbosity::CHANGES) { @@ -493,15 +495,15 @@ pub fn chown_base<'a>( let (dest_gid, dest_uid, filter) = parse_gid_uid_and_filter(&matches)?; let executor = ChownExecutor { - bit_flag, + traverse_symlinks, dest_gid, dest_uid, verbosity: Verbosity { - groups_only: true, + groups_only, level: verbosity_level, }, recursive, - dereference: dereference != 0, + dereference: dereference.unwrap_or(true), preserve_root, files, filter, From a7f6b4420a4a28148657c437aed13dda798176bd Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Thu, 2 Sep 2021 22:31:49 +0200 Subject: [PATCH 3/4] uucore/perms: take traverse_symlinks into account --- src/uucore/src/lib/features/perms.rs | 15 ++++++- tests/by-util/test_chgrp.rs | 65 ++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index b605c0cdf..a4a01c499 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -265,10 +265,21 @@ impl ChownExecutor { } fn dive_into>(&self, root: P) -> i32 { - let mut ret = 0; let root = root.as_ref(); + + // walkdir always dereferences the root directory, so we have to check it ourselves + // TODO: replace with `root.is_symlink()` once it is stable + if self.traverse_symlinks == TraverseSymlinks::None + && std::fs::symlink_metadata(root) + .map(|m| m.file_type().is_symlink()) + .unwrap_or(false) + { + return 0; + } + + let mut ret = 0; let mut iterator = WalkDir::new(root) - .follow_links(self.dereference) + .follow_links(self.traverse_symlinks == TraverseSymlinks::All) .min_depth(1) .into_iter(); // We can't use a for loop because we need to manipulate the iterator inside the loop. diff --git a/tests/by-util/test_chgrp.rs b/tests/by-util/test_chgrp.rs index 0fc73520e..1d047cfe2 100644 --- a/tests/by-util/test_chgrp.rs +++ b/tests/by-util/test_chgrp.rs @@ -288,3 +288,68 @@ fn test_subdir_permission_denied() { .stderr_only("chgrp: cannot access 'dir/subdir': Permission denied"); } } + +#[test] +#[cfg(not(target_vendor = "apple"))] +fn test_traverse_symlinks() { + use std::os::unix::prelude::MetadataExt; + let groups = nix::unistd::getgroups().unwrap(); + if groups.len() < 2 { + return; + } + let (first_group, second_group) = (groups[0], groups[1]); + + for &(args, traverse_first, traverse_second) in &[ + (&[][..] as &[&str], false, false), + (&["-H"][..], true, false), + (&["-P"][..], false, false), + (&["-L"][..], true, true), + ] { + let scenario = TestScenario::new("chgrp"); + + let (at, mut ucmd) = (scenario.fixtures.clone(), scenario.ucmd()); + + at.mkdir("dir"); + at.mkdir("dir2"); + at.touch("dir2/file"); + at.mkdir("dir3"); + at.touch("dir3/file"); + at.symlink_dir("dir2", "dir/dir2_ln"); + at.symlink_dir("dir3", "dir3_ln"); + + scenario + .ccmd("chgrp") + .arg(first_group.to_string()) + .arg("dir2/file") + .arg("dir3/file") + .succeeds(); + + assert!(at.plus("dir2/file").metadata().unwrap().gid() == first_group.as_raw()); + assert!(at.plus("dir3/file").metadata().unwrap().gid() == first_group.as_raw()); + + ucmd.arg("-R") + .args(args) + .arg(second_group.to_string()) + .arg("dir") + .arg("dir3_ln") + .succeeds() + .no_stderr(); + + assert_eq!( + at.plus("dir2/file").metadata().unwrap().gid(), + if traverse_second { + second_group.as_raw() + } else { + first_group.as_raw() + } + ); + assert_eq!( + at.plus("dir3/file").metadata().unwrap().gid(), + if traverse_first { + second_group.as_raw() + } else { + first_group.as_raw() + } + ); + } +} From 435b7a22fb8d64d1c2c0d4ddca0b387f25984c1d Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Thu, 2 Sep 2021 22:42:09 +0200 Subject: [PATCH 4/4] uucore/perms: add more information to an error message This reverts part of https://github.com/uutils/coreutils/pull/2628, because (even though it got the test passing) it was the wrong bug fix. --- src/uucore/src/lib/features/perms.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index a4a01c499..b071cedaa 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -358,7 +358,12 @@ impl ChownExecutor { Err(e) => { match self.verbosity.level { VerbosityLevel::Silent => (), - _ => show_error!("cannot access '{}': {}", path.display(), strip_errno(&e)), + _ => show_error!( + "cannot {} '{}': {}", + if follow { "dereference" } else { "access" }, + path.display(), + strip_errno(&e) + ), } None }