From 231bb7be93639576bdc553ae7a6fa2f7f5568ddc Mon Sep 17 00:00:00 2001 From: rethab Date: Wed, 5 May 2021 22:59:40 +0200 Subject: [PATCH] Migrate mknod to clap, closes #2051 (#2056) * mknod: add tests for fifo * mknod: add test for character device --- .gitignore | 1 + Cargo.lock | 2 +- src/uu/mknod/Cargo.toml | 2 +- src/uu/mknod/src/mknod.rs | 304 +++++++++++++++------------- src/uu/mknod/src/parsemode.rs | 54 +++++ src/uucore/src/lib/features/mode.rs | 33 ++- tests/by-util/test_mknod.rs | 125 +++++++++++- tests/common/util.rs | 16 +- 8 files changed, 371 insertions(+), 166 deletions(-) create mode 100644 src/uu/mknod/src/parsemode.rs diff --git a/.gitignore b/.gitignore index b1ac52506..11f46e13e 100644 --- a/.gitignore +++ b/.gitignore @@ -12,3 +12,4 @@ target/ Cargo.lock lib*.a /docs/_build +*.iml diff --git a/Cargo.lock b/Cargo.lock index 6ff3cd5c1..62fa80c2d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2116,7 +2116,7 @@ dependencies = [ name = "uu_mknod" version = "0.0.6" dependencies = [ - "getopts", + "clap", "libc", "uucore", "uucore_procs", diff --git a/src/uu/mknod/Cargo.toml b/src/uu/mknod/Cargo.toml index 2c3ac8fb9..1320e3546 100644 --- a/src/uu/mknod/Cargo.toml +++ b/src/uu/mknod/Cargo.toml @@ -16,7 +16,7 @@ name = "uu_mknod" path = "src/mknod.rs" [dependencies] -getopts = "0.2.18" +clap = "2.33" libc = "^0.2.42" uucore = { version=">=0.0.8", package="uucore", path="../../uucore", features=["mode"] } uucore_procs = { version=">=0.0.5", package="uucore_procs", path="../../uucore_procs" } diff --git a/src/uu/mknod/src/mknod.rs b/src/uu/mknod/src/mknod.rs index fc6fb0870..5b6c2fa8c 100644 --- a/src/uu/mknod/src/mknod.rs +++ b/src/uu/mknod/src/mknod.rs @@ -5,21 +5,41 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. -// spell-checker:ignore (ToDO) parsemode makedev sysmacros makenod newmode perror IFBLK IFCHR IFIFO +// spell-checker:ignore (ToDO) parsemode makedev sysmacros perror IFBLK IFCHR IFIFO #[macro_use] extern crate uucore; +use std::ffi::CString; + +use clap::{App, Arg, ArgMatches}; use libc::{dev_t, mode_t}; use libc::{S_IFBLK, S_IFCHR, S_IFIFO, S_IRGRP, S_IROTH, S_IRUSR, S_IWGRP, S_IWOTH, S_IWUSR}; -use getopts::Options; - -use std::ffi::CString; use uucore::InvalidEncodingHandling; static NAME: &str = "mknod"; static VERSION: &str = env!("CARGO_PKG_VERSION"); +static ABOUT: &str = "Create the special file NAME of the given TYPE."; +static USAGE: &str = "mknod [OPTION]... NAME TYPE [MAJOR MINOR]"; +static LONG_HELP: &str = "Mandatory arguments to long options are mandatory for short options too. +-m, --mode=MODE set file permission bits to MODE, not a=rw - umask +--help display this help and exit +--version output version information and exit + +Both MAJOR and MINOR must be specified when TYPE is b, c, or u, and they +must be omitted when TYPE is p. If MAJOR or MINOR begins with 0x or 0X, +it is interpreted as hexadecimal; otherwise, if it begins with 0, as octal; +otherwise, as decimal. TYPE may be: + +b create a block (buffered) special file +c, u create a character (unbuffered) special file +p create a FIFO + +NOTE: your shell may have its own version of mknod, which usually supersedes +the version described here. Please refer to your shell's documentation +for details about the options it supports. +"; const MODE_RW_UGO: mode_t = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH; @@ -30,13 +50,35 @@ fn makedev(maj: u64, min: u64) -> dev_t { } #[cfg(windows)] -fn _makenod(path: CString, mode: mode_t, dev: dev_t) -> i32 { +fn _makenod(file_name: &str, mode: mode_t, dev: dev_t) -> i32 { panic!("Unsupported for windows platform") } #[cfg(unix)] -fn _makenod(path: CString, mode: mode_t, dev: dev_t) -> i32 { - unsafe { libc::mknod(path.as_ptr(), mode, dev) } +fn _makenod(file_name: &str, mode: mode_t, dev: dev_t) -> i32 { + let c_str = CString::new(file_name).expect("Failed to convert to CString"); + + // the user supplied a mode + let set_umask = mode & MODE_RW_UGO != MODE_RW_UGO; + + unsafe { + // store prev umask + let last_umask = if set_umask { libc::umask(0) } else { 0 }; + + let errno = libc::mknod(c_str.as_ptr(), mode, dev); + + // set umask back to original value + if set_umask { + libc::umask(last_umask); + } + + if errno == -1 { + let c_str = CString::new(NAME).expect("Failed to convert to CString"); + // shows the error from the mknod syscall + libc::perror(c_str.as_ptr()); + } + errno + } } #[allow(clippy::cognitive_complexity)] @@ -44,156 +86,136 @@ pub fn uumain(args: impl uucore::Args) -> i32 { let args = args .collect_str(InvalidEncodingHandling::Ignore) .accept_any(); - - let mut opts = Options::new(); - // Linux-specific options, not implemented // opts.optflag("Z", "", "set the SELinux security context to default type"); // opts.optopt("", "context", "like -Z, or if CTX is specified then set the SELinux or SMACK security context to CTX"); - opts.optopt( - "m", - "mode", - "set file permission bits to MODE, not a=rw - umask", - "MODE", - ); - opts.optflag("", "help", "display this help and exit"); - opts.optflag("", "version", "output version information and exit"); + let matches = App::new(executable!()) + .version(VERSION) + .usage(USAGE) + .after_help(LONG_HELP) + .about(ABOUT) + .arg( + Arg::with_name("mode") + .short("m") + .long("mode") + .value_name("MODE") + .help("set file permission bits to MODE, not a=rw - umask"), + ) + .arg( + Arg::with_name("name") + .value_name("NAME") + .help("name of the new file") + .required(true) + .index(1), + ) + .arg( + Arg::with_name("type") + .value_name("TYPE") + .help("type of the new file (b, c, u or p)") + .required(true) + .validator(valid_type) + .index(2), + ) + .arg( + Arg::with_name("major") + .value_name("MAJOR") + .help("major file type") + .validator(valid_u64) + .index(3), + ) + .arg( + Arg::with_name("minor") + .value_name("MINOR") + .help("minor file type") + .validator(valid_u64) + .index(4), + ) + .get_matches_from(args); - let matches = match opts.parse(&args[1..]) { - Ok(m) => m, - Err(f) => crash!(1, "{}\nTry '{} --help' for more information.", f, NAME), + let mode = match get_mode(&matches) { + Ok(mode) => mode, + Err(err) => { + show_info!("{}", err); + return 1; + } }; - if matches.opt_present("help") { - println!( - "Usage: {0} [OPTION]... NAME TYPE [MAJOR MINOR] + let file_name = matches.value_of("name").expect("Missing argument 'NAME'"); -Mandatory arguments to long options are mandatory for short options too. - -m, --mode=MODE set file permission bits to MODE, not a=rw - umask - --help display this help and exit - --version output version information and exit + // Only check the first character, to allow mnemonic usage like + // 'mknod /dev/rst0 character 18 0'. + let ch = matches + .value_of("type") + .expect("Missing argument 'TYPE'") + .chars() + .next() + .expect("Failed to get the first char"); -Both MAJOR and MINOR must be specified when TYPE is b, c, or u, and they -must be omitted when TYPE is p. If MAJOR or MINOR begins with 0x or 0X, -it is interpreted as hexadecimal; otherwise, if it begins with 0, as octal; -otherwise, as decimal. TYPE may be: - - b create a block (buffered) special file - c, u create a character (unbuffered) special file - p create a FIFO - -NOTE: your shell may have its own version of mknod, which usually supersedes -the version described here. Please refer to your shell's documentation -for details about the options it supports.", - NAME - ); - return 0; - } - - if matches.opt_present("version") { - println!("{} {}", NAME, VERSION); - return 0; - } - - let mut last_umask: mode_t = 0; - let mut newmode: mode_t = MODE_RW_UGO; - if matches.opt_present("mode") { - match uucore::mode::parse_mode(matches.opt_str("mode")) { - Ok(parsed) => { - if parsed > 0o777 { - show_info!("mode must specify only file permission bits"); - return 1; - } - newmode = parsed; - } - Err(e) => { - show_info!("{}", e); - return 1; - } + if ch == 'p' { + if matches.is_present("major") || matches.is_present("minor") { + eprintln!("Fifos do not have major and minor device numbers."); + eprintln!("Try '{} --help' for more information.", NAME); + 1 + } else { + _makenod(file_name, S_IFIFO | mode, 0) } - unsafe { - last_umask = libc::umask(0); - } - } + } else { + match (matches.value_of("major"), matches.value_of("minor")) { + (None, None) | (_, None) | (None, _) => { + eprintln!("Special files require major and minor device numbers."); + eprintln!("Try '{} --help' for more information.", NAME); + 1 + } + (Some(major), Some(minor)) => { + let major = major.parse::().expect("validated by clap"); + let minor = minor.parse::().expect("validated by clap"); - let mut ret = 0i32; - match matches.free.len() { - 0 => show_usage_error!("missing operand"), - 1 => show_usage_error!("missing operand after ‘{}’", matches.free[0]), - _ => { - let args = &matches.free; - let c_str = CString::new(args[0].as_str()).expect("Failed to convert to CString"); - - // Only check the first character, to allow mnemonic usage like - // 'mknod /dev/rst0 character 18 0'. - let ch = args[1] - .chars() - .next() - .expect("Failed to get the first char"); - - if ch == 'p' { - if args.len() > 2 { - show_info!("{}: extra operand ‘{}’", NAME, args[2]); - if args.len() == 4 { - eprintln!("Fifos do not have major and minor device numbers."); - } - eprintln!("Try '{} --help' for more information.", NAME); - return 1; - } - - ret = _makenod(c_str, S_IFIFO | newmode, 0); - } else { - if args.len() < 4 { - show_info!("missing operand after ‘{}’", args[args.len() - 1]); - if args.len() == 2 { - eprintln!("Special files require major and minor device numbers."); - } - eprintln!("Try '{} --help' for more information.", NAME); - return 1; - } else if args.len() > 4 { - show_usage_error!("extra operand ‘{}’", args[4]); - return 1; - } else if !"bcu".contains(ch) { - show_usage_error!("invalid device type ‘{}’", args[1]); - return 1; - } - - let maj = args[2].parse::(); - let min = args[3].parse::(); - if maj.is_err() { - show_info!("invalid major device number ‘{}’", args[2]); - return 1; - } else if min.is_err() { - show_info!("invalid minor device number ‘{}’", args[3]); - return 1; - } - - let (maj, min) = (maj.unwrap(), min.unwrap()); - let dev = makedev(maj, min); + let dev = makedev(major, minor); if ch == 'b' { // block special file - ret = _makenod(c_str, S_IFBLK | newmode, dev); - } else { + _makenod(file_name, S_IFBLK | mode, dev) + } else if ch == 'c' || ch == 'u' { // char special file - ret = _makenod(c_str, S_IFCHR | newmode, dev); + _makenod(file_name, S_IFCHR | mode, dev) + } else { + unreachable!("{} was validated to be only b, c or u", ch); } } } } - - if last_umask != 0 { - unsafe { - libc::umask(last_umask); - } - } - if ret == -1 { - let c_str = CString::new(format!("{}: {}", NAME, matches.free[0]).as_str()) - .expect("Failed to convert to CString"); - unsafe { - libc::perror(c_str.as_ptr()); - } - } - - ret +} + +fn get_mode(matches: &ArgMatches) -> Result { + match matches.value_of("mode") { + None => Ok(MODE_RW_UGO), + Some(str_mode) => uucore::mode::parse_mode(str_mode) + .map_err(|e| format!("invalid mode ({})", e)) + .and_then(|mode| { + if mode > 0o777 { + Err("mode must specify only file permission bits".to_string()) + } else { + Ok(mode) + } + }), + } +} + +fn valid_type(tpe: String) -> Result<(), String> { + // Only check the first character, to allow mnemonic usage like + // 'mknod /dev/rst0 character 18 0'. + tpe.chars() + .next() + .ok_or_else(|| "missing device type".to_string()) + .and_then(|first_char| { + if vec!['b', 'c', 'u', 'p'].contains(&first_char) { + Ok(()) + } else { + Err(format!("invalid device type ‘{}’", tpe)) + } + }) +} + +fn valid_u64(num: String) -> Result<(), String> { + num.parse::().map(|_| ()).map_err(|_| num) } diff --git a/src/uu/mknod/src/parsemode.rs b/src/uu/mknod/src/parsemode.rs new file mode 100644 index 000000000..026fc4a56 --- /dev/null +++ b/src/uu/mknod/src/parsemode.rs @@ -0,0 +1,54 @@ +// spell-checker:ignore (ToDO) fperm + +use libc::{mode_t, S_IRGRP, S_IROTH, S_IRUSR, S_IWGRP, S_IWOTH, S_IWUSR}; + +use uucore::mode; + +pub const MODE_RW_UGO: mode_t = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH; + +pub fn parse_mode(mode: &str) -> Result { + let arr: &[char] = &['0', '1', '2', '3', '4', '5', '6', '7', '8', '9']; + let result = if mode.contains(arr) { + mode::parse_numeric(MODE_RW_UGO as u32, mode) + } else { + mode::parse_symbolic(MODE_RW_UGO as u32, mode, true) + }; + result.map(|mode| mode as mode_t) +} + +#[cfg(test)] +mod test { + /// Test if the program is running under WSL + // ref: @@ + // ToDO: test on WSL2 which likely doesn't need special handling; plan change to `is_wsl_1()` if WSL2 is less needy + pub fn is_wsl() -> bool { + #[cfg(target_os = "linux")] + { + if let Ok(b) = std::fs::read("/proc/sys/kernel/osrelease") { + if let Ok(s) = std::str::from_utf8(&b) { + let a = s.to_ascii_lowercase(); + return a.contains("microsoft") || a.contains("wsl"); + } + } + } + false + } + + #[test] + fn symbolic_modes() { + assert_eq!(super::parse_mode("u+x").unwrap(), 0o766); + assert_eq!( + super::parse_mode("+x").unwrap(), + if !is_wsl() { 0o777 } else { 0o776 } + ); + assert_eq!(super::parse_mode("a-w").unwrap(), 0o444); + assert_eq!(super::parse_mode("g-r").unwrap(), 0o626); + } + + #[test] + fn numeric_modes() { + assert_eq!(super::parse_mode("644").unwrap(), 0o644); + assert_eq!(super::parse_mode("+100").unwrap(), 0o766); + assert_eq!(super::parse_mode("-4").unwrap(), 0o662); + } +} diff --git a/src/uucore/src/lib/features/mode.rs b/src/uucore/src/lib/features/mode.rs index 1bb79ac03..4fb5a6509 100644 --- a/src/uucore/src/lib/features/mode.rs +++ b/src/uucore/src/lib/features/mode.rs @@ -132,19 +132,15 @@ fn parse_change(mode: &str, fperm: u32, considering_dir: bool) -> (u32, usize) { (srwx, pos) } -pub fn parse_mode(mode: Option) -> Result { +pub fn parse_mode(mode: &str) -> Result { let fperm = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH; - if let Some(mode) = mode { - let arr: &[char] = &['0', '1', '2', '3', '4', '5', '6', '7', '8', '9']; - let result = if mode.contains(arr) { - parse_numeric(fperm as u32, mode.as_str()) - } else { - parse_symbolic(fperm as u32, mode.as_str(), true) - }; - result.map(|mode| mode as mode_t) + let arr: &[char] = &['0', '1', '2', '3', '4', '5', '6', '7', '8', '9']; + let result = if mode.contains(arr) { + parse_numeric(fperm as u32, mode) } else { - Ok(fperm) - } + parse_symbolic(fperm as u32, mode, true) + }; + result.map(|mode| mode as mode_t) } #[cfg(test)] @@ -152,20 +148,19 @@ mod test { #[test] fn symbolic_modes() { - assert_eq!(super::parse_mode(Some("u+x".to_owned())).unwrap(), 0o766); + assert_eq!(super::parse_mode("u+x").unwrap(), 0o766); assert_eq!( - super::parse_mode(Some("+x".to_owned())).unwrap(), + super::parse_mode("+x").unwrap(), if !crate::os::is_wsl_1() { 0o777 } else { 0o776 } ); - assert_eq!(super::parse_mode(Some("a-w".to_owned())).unwrap(), 0o444); - assert_eq!(super::parse_mode(Some("g-r".to_owned())).unwrap(), 0o626); + assert_eq!(super::parse_mode("a-w").unwrap(), 0o444); + assert_eq!(super::parse_mode("g-r").unwrap(), 0o626); } #[test] fn numeric_modes() { - assert_eq!(super::parse_mode(Some("644".to_owned())).unwrap(), 0o644); - assert_eq!(super::parse_mode(Some("+100".to_owned())).unwrap(), 0o766); - assert_eq!(super::parse_mode(Some("-4".to_owned())).unwrap(), 0o662); - assert_eq!(super::parse_mode(None).unwrap(), 0o666); + assert_eq!(super::parse_mode("644").unwrap(), 0o644); + assert_eq!(super::parse_mode("+100").unwrap(), 0o766); + assert_eq!(super::parse_mode("-4").unwrap(), 0o662); } } diff --git a/tests/by-util/test_mknod.rs b/tests/by-util/test_mknod.rs index 651491045..1d39372ac 100644 --- a/tests/by-util/test_mknod.rs +++ b/tests/by-util/test_mknod.rs @@ -1 +1,124 @@ -// ToDO: add tests +use crate::common::util::*; + +#[cfg(not(windows))] +#[test] +fn test_mknod_help() { + new_ucmd!() + .arg("--help") + .succeeds() + .no_stderr() + .stdout_contains("USAGE:"); +} + +#[test] +#[cfg(not(windows))] +fn test_mknod_version() { + assert!(new_ucmd!() + .arg("--version") + .succeeds() + .no_stderr() + .stdout_str() + .starts_with("mknod")); +} + +#[test] +#[cfg(not(windows))] +fn test_mknod_fifo_default_writable() { + let ts = TestScenario::new(util_name!()); + ts.ucmd().arg("test_file").arg("p").succeeds(); + assert!(ts.fixtures.is_fifo("test_file")); + assert!(!ts.fixtures.metadata("test_file").permissions().readonly()); +} + +#[test] +#[cfg(not(windows))] +fn test_mknod_fifo_mnemonic_usage() { + let ts = TestScenario::new(util_name!()); + ts.ucmd().arg("test_file").arg("pipe").succeeds(); + assert!(ts.fixtures.is_fifo("test_file")); +} + +#[test] +#[cfg(not(windows))] +fn test_mknod_fifo_read_only() { + let ts = TestScenario::new(util_name!()); + ts.ucmd() + .arg("-m") + .arg("a=r") + .arg("test_file") + .arg("p") + .succeeds(); + assert!(ts.fixtures.is_fifo("test_file")); + assert!(ts.fixtures.metadata("test_file").permissions().readonly()); +} + +#[test] +#[cfg(not(windows))] +fn test_mknod_fifo_invalid_extra_operand() { + new_ucmd!() + .arg("test_file") + .arg("p") + .arg("1") + .arg("2") + .fails() + .stderr_contains(&"Fifos do not have major and minor device numbers"); +} + +#[test] +#[cfg(not(windows))] +fn test_mknod_character_device_requires_major_and_minor() { + new_ucmd!() + .arg("test_file") + .arg("c") + .fails() + .status_code(1) + .stderr_contains(&"Special files require major and minor device numbers."); + new_ucmd!() + .arg("test_file") + .arg("c") + .arg("1") + .fails() + .status_code(1) + .stderr_contains(&"Special files require major and minor device numbers."); + new_ucmd!() + .arg("test_file") + .arg("c") + .arg("1") + .arg("c") + .fails() + .status_code(1) + .stderr_contains(&"Invalid value for ''"); + new_ucmd!() + .arg("test_file") + .arg("c") + .arg("c") + .arg("1") + .fails() + .status_code(1) + .stderr_contains(&"Invalid value for ''"); +} + +#[test] +#[cfg(not(windows))] +fn test_mknod_invalid_arg() { + new_ucmd!() + .arg("--foo") + .fails() + .status_code(1) + .no_stdout() + .stderr_contains(&"Found argument '--foo' which wasn't expected"); +} + +#[test] +#[cfg(not(windows))] +fn test_mknod_invalid_mode() { + new_ucmd!() + .arg("--mode") + .arg("rw") + .arg("test_file") + .arg("p") + .fails() + .no_stdout() + .status_code(1) + .stderr_contains(&"invalid mode"); +} diff --git a/tests/common/util.rs b/tests/common/util.rs index 1ade70127..719849afc 100644 --- a/tests/common/util.rs +++ b/tests/common/util.rs @@ -163,7 +163,7 @@ impl CmdResult { /// asserts that the command's exit code is the same as the given one pub fn status_code(&self, code: i32) -> &CmdResult { - assert!(self.code == Some(code)); + assert_eq!(self.code, Some(code)); self } @@ -295,12 +295,22 @@ impl CmdResult { } pub fn stdout_contains>(&self, cmp: T) -> &CmdResult { - assert!(self.stdout_str().contains(cmp.as_ref())); + assert!( + self.stdout_str().contains(cmp.as_ref()), + "'{}' does not contain '{}'", + self.stdout_str(), + cmp.as_ref() + ); self } pub fn stderr_contains>(&self, cmp: T) -> &CmdResult { - assert!(self.stderr_str().contains(cmp.as_ref())); + assert!( + self.stderr_str().contains(cmp.as_ref()), + "'{}' does not contain '{}'", + self.stderr_str(), + cmp.as_ref() + ); self }