Migrate mknod to clap, closes #2051 (#2056)

* mknod: add tests for fifo

* mknod: add test for character device

.gitignore

@@ -12,3 +12,4 @@ target/
 Cargo.lock
 lib*.a
 /docs/_build
+*.iml

Cargo.lock

@@ -2116,7 +2116,7 @@ dependencies = [
 name = "uu_mknod"
 version = "0.0.6"
 dependencies = [
- "getopts",
+ "clap",
  "libc",
  "uucore",
  "uucore_procs",

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" }

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 {
+fn _makenod(file_name: &str, mode: mode_t, dev: dev_t) -> i32 {
-    unsafe { libc::mknod(path.as_ptr(), mode, dev) }
+    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");
+    let matches = App::new(executable!())
-    opts.optflag("", "version", "output version information and exit");
+        .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..]) {
+    let mode = match get_mode(&matches) {
-        Ok(m) => m,
+        Ok(mode) => mode,
-        Err(f) => crash!(1, "{}\nTry '{} --help' for more information.", f, NAME),
+        Err(err) => {
+            show_info!("{}", err);
+            return 1;
+        }
     };
 
-    if matches.opt_present("help") {
+    let file_name = matches.value_of("name").expect("Missing argument 'NAME'");
-        println!(
-            "Usage: {0} [OPTION]... NAME TYPE [MAJOR MINOR]
 
-Mandatory arguments to long options are mandatory for short options too.
+    // Only check the first character, to allow mnemonic usage like
-  -m, --mode=MODE    set file permission bits to MODE, not a=rw - umask
+    // 'mknod /dev/rst0 character 18 0'.
-      --help     display this help and exit
+    let ch = matches
-      --version  output version information and exit
+        .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
+    if ch == 'p' {
-must be omitted when TYPE is p.  If MAJOR or MINOR begins with 0x or 0X,
+        if matches.is_present("major") || matches.is_present("minor") {
-it is interpreted as hexadecimal; otherwise, if it begins with 0, as octal;
+            eprintln!("Fifos do not have major and minor device numbers.");
-otherwise, as decimal.  TYPE may be:
+            eprintln!("Try '{} --help' for more information.", NAME);
-
+            1
-  b      create a block (buffered) special file
+        } else {
-  c, u   create a character (unbuffered) special file
+            _makenod(file_name, S_IFIFO | mode, 0)
-  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;
-            }
         }
-        unsafe {
+    } else {
-            last_umask = libc::umask(0);
+        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::<u64>().expect("validated by clap");
+                let minor = minor.parse::<u64>().expect("validated by clap");
 
-    let mut ret = 0i32;
+                let dev = makedev(major, minor);
-    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::<u64>();
-                let min = args[3].parse::<u64>();
-                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);
                 if ch == 'b' {
                     // block special file
-                    ret = _makenod(c_str, S_IFBLK | newmode, dev);
+                    _makenod(file_name, S_IFBLK | mode, dev)
-                } else {
+                } 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 {
+fn get_mode(matches: &ArgMatches) -> Result<mode_t, String> {
-            libc::umask(last_umask);
+    match matches.value_of("mode") {
-        }
+        None => Ok(MODE_RW_UGO),
-    }
+        Some(str_mode) => uucore::mode::parse_mode(str_mode)
-    if ret == -1 {
+            .map_err(|e| format!("invalid mode ({})", e))
-        let c_str = CString::new(format!("{}: {}", NAME, matches.free[0]).as_str())
+            .and_then(|mode| {
-            .expect("Failed to convert to CString");
+                if mode > 0o777 {
-        unsafe {
+                    Err("mode must specify only file permission bits".to_string())
-            libc::perror(c_str.as_ptr());
+                } else {
-        }
+                    Ok(mode)
-    }
+                }
-
+            }),
-    ret
+    }
+}
+
+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::<u64>().map(|_| ()).map_err(|_| num)
 }

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<mode_t, String> {
+    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: <https://github.com/microsoft/WSL/issues/4555> @@ <https://archive.is/dP0bz>
+    // 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);
+    }
+}

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<String>) -> Result<mode_t, String> {
+pub fn parse_mode(mode: &str) -> Result<mode_t, String> {
     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 arr: &[char] = &['0', '1', '2', '3', '4', '5', '6', '7', '8', '9'];
+    let result = if mode.contains(arr) {
-        let result = if mode.contains(arr) {
+        parse_numeric(fperm as u32, mode)
-            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)
     } 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("a-w").unwrap(), 0o444);
-        assert_eq!(super::parse_mode(Some("g-r".to_owned())).unwrap(), 0o626);
+        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("644").unwrap(), 0o644);
-        assert_eq!(super::parse_mode(Some("+100".to_owned())).unwrap(), 0o766);
+        assert_eq!(super::parse_mode("+100").unwrap(), 0o766);
-        assert_eq!(super::parse_mode(Some("-4".to_owned())).unwrap(), 0o662);
+        assert_eq!(super::parse_mode("-4").unwrap(), 0o662);
-        assert_eq!(super::parse_mode(None).unwrap(), 0o666);
     }
 }

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 '<MINOR>'");
+    new_ucmd!()
+        .arg("test_file")
+        .arg("c")
+        .arg("c")
+        .arg("1")
+        .fails()
+        .status_code(1)
+        .stderr_contains(&"Invalid value for '<MAJOR>'");
+}
+
+#[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");
+}

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<T: AsRef<str>>(&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<T: AsRef<str>>(&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
     }