Merge pull request #9578 from martinkunkel2/mknod-multiple-modes

uucore: mode parsing: support comma-separated modes

src/uu/install/src/install.rs

@@ -338,7 +338,7 @@ fn behavior(matches: &ArgMatches) -> UResult<Behavior> {
 
     let specified_mode: Option<u32> = if matches.contains_id(OPT_MODE) {
         let x = matches.get_one::<String>(OPT_MODE).ok_or(1)?;
-        Some(mode::parse(x, considering_dir, 0).map_err(|err| {
+        Some(uucore::mode::parse(x, considering_dir, 0).map_err(|err| {
             show_error!(
                 "{}",
                 translate!("install-error-invalid-mode", "error" => err)

src/uu/install/src/mode.rs

@@ -4,32 +4,8 @@
 // file that was distributed with this source code.
 use std::fs;
 use std::path::Path;
-#[cfg(not(windows))]
-use uucore::mode;
 use uucore::translate;
 
-/// Takes a user-supplied string and tries to parse to u16 mode bitmask.
-/// Supports comma-separated mode strings like "ug+rwX,o+rX" (same as chmod).
-pub fn parse(mode_string: &str, considering_dir: bool, umask: u32) -> Result<u32, String> {
-    // Split by commas and process each mode part sequentially
-    let mut current_mode: u32 = 0;
-
-    for mode_part in mode_string.split(',') {
-        let mode_part = mode_part.trim();
-        if mode_part.is_empty() {
-            continue;
-        }
-
-        current_mode = if mode_part.chars().any(|c| c.is_ascii_digit()) {
-            mode::parse_numeric(current_mode, mode_part, considering_dir)?
-        } else {
-            mode::parse_symbolic(current_mode, mode_part, umask, considering_dir)?
-        };
-    }
-
-    Ok(current_mode)
-}
-
 /// chmod a file or directory on UNIX.
 ///
 /// Adapted from mkdir.rs.  Handles own error printing.
@@ -55,128 +31,3 @@ pub fn chmod(path: &Path, mode: u32) -> Result<(), ()> {
     // chmod on Windows only sets the readonly flag, which isn't even honored on directories
     Ok(())
 }
-
-#[cfg(test)]
-#[cfg(not(windows))]
-mod tests {
-    use super::parse;
-
-    #[test]
-    fn test_parse_numeric_mode() {
-        // Simple numeric mode
-        assert_eq!(parse("644", false, 0).unwrap(), 0o644);
-        assert_eq!(parse("755", false, 0).unwrap(), 0o755);
-        assert_eq!(parse("777", false, 0).unwrap(), 0o777);
-        assert_eq!(parse("600", false, 0).unwrap(), 0o600);
-    }
-
-    #[test]
-    fn test_parse_numeric_mode_with_operator() {
-        // Numeric mode with + operator
-        assert_eq!(parse("+100", false, 0).unwrap(), 0o100);
-        assert_eq!(parse("+644", false, 0).unwrap(), 0o644);
-
-        // Numeric mode with - operator (starting from 0, so nothing to remove)
-        assert_eq!(parse("-4", false, 0).unwrap(), 0);
-        // But if we first set a mode, then remove bits
-        assert_eq!(parse("644,-4", false, 0).unwrap(), 0o640);
-    }
-
-    #[test]
-    fn test_parse_symbolic_mode() {
-        // Simple symbolic modes
-        assert_eq!(parse("u+x", false, 0).unwrap(), 0o100);
-        assert_eq!(parse("g+w", false, 0).unwrap(), 0o020);
-        assert_eq!(parse("o+r", false, 0).unwrap(), 0o004);
-        assert_eq!(parse("a+x", false, 0).unwrap(), 0o111);
-    }
-
-    #[test]
-    fn test_parse_symbolic_mode_multiple_permissions() {
-        // Multiple permissions in one mode
-        assert_eq!(parse("u+rw", false, 0).unwrap(), 0o600);
-        assert_eq!(parse("ug+rwx", false, 0).unwrap(), 0o770);
-        assert_eq!(parse("a+rwx", false, 0).unwrap(), 0o777);
-    }
-
-    #[test]
-    fn test_parse_comma_separated_modes() {
-        // Comma-separated mode strings (as mentioned in the doc comment)
-        assert_eq!(parse("ug+rwX,o+rX", false, 0).unwrap(), 0o664);
-        assert_eq!(parse("u+rwx,g+rx,o+r", false, 0).unwrap(), 0o754);
-        assert_eq!(parse("u+w,g+w,o+w", false, 0).unwrap(), 0o222);
-    }
-
-    #[test]
-    fn test_parse_comma_separated_with_spaces() {
-        // Comma-separated with spaces (should be trimmed)
-        assert_eq!(parse("u+rw, g+rw, o+r", false, 0).unwrap(), 0o664);
-        assert_eq!(parse(" u+x , g+x ", false, 0).unwrap(), 0o110);
-    }
-
-    #[test]
-    fn test_parse_mixed_numeric_and_symbolic() {
-        // Mix of numeric and symbolic modes
-        assert_eq!(parse("644,u+x", false, 0).unwrap(), 0o744);
-        assert_eq!(parse("u+rw,755", false, 0).unwrap(), 0o755);
-    }
-
-    #[test]
-    fn test_parse_empty_string() {
-        // Empty string should return 0
-        assert_eq!(parse("", false, 0).unwrap(), 0);
-        assert_eq!(parse("   ", false, 0).unwrap(), 0);
-        assert_eq!(parse(",,", false, 0).unwrap(), 0);
-    }
-
-    #[test]
-    fn test_parse_with_umask() {
-        // Test with umask (affects symbolic modes when no level is specified)
-        let umask = 0o022;
-        assert_eq!(parse("+w", false, umask).unwrap(), 0o200);
-        // The umask should be respected for symbolic modes without explicit level
-    }
-
-    #[test]
-    fn test_parse_considering_dir() {
-        // Test directory vs file mode differences
-        // For directories, X (capital X) should add execute permission
-        assert_eq!(parse("a+X", true, 0).unwrap(), 0o111);
-        // For files without execute, X should not add execute
-        assert_eq!(parse("a+X", false, 0).unwrap(), 0o000);
-
-        // Numeric modes for directories preserve setuid/setgid bits
-        assert_eq!(parse("755", true, 0).unwrap(), 0o755);
-    }
-
-    #[test]
-    fn test_parse_invalid_modes() {
-        // Invalid numeric mode (too large)
-        assert!(parse("10000", false, 0).is_err());
-
-        // Invalid operator
-        assert!(parse("u*rw", false, 0).is_err());
-
-        // Invalid symbolic mode
-        assert!(parse("invalid", false, 0).is_err());
-    }
-
-    #[test]
-    fn test_parse_complex_combinations() {
-        // Complex real-world examples
-        assert_eq!(parse("u=rwx,g=rx,o=r", false, 0).unwrap(), 0o754);
-        // To test removal, we need to first set permissions, then remove them
-        assert_eq!(parse("644,a-w", false, 0).unwrap(), 0o444);
-        assert_eq!(parse("644,g-r", false, 0).unwrap(), 0o604);
-    }
-
-    #[test]
-    fn test_parse_sequential_application() {
-        // Test that comma-separated modes are applied sequentially
-        // First set to 644, then add execute for user
-        assert_eq!(parse("644,u+x", false, 0).unwrap(), 0o744);
-
-        // First add user write, then set to 755 (should override)
-        assert_eq!(parse("u+w,755", false, 0).unwrap(), 0o755);
-    }
-}

src/uu/mkdir/src/mkdir.rs

@@ -57,20 +57,11 @@ fn get_mode(_matches: &ArgMatches) -> Result<u32, String> {
 #[cfg(not(windows))]
 fn get_mode(matches: &ArgMatches) -> Result<u32, String> {
     // Not tested on Windows
-    let mut new_mode = DEFAULT_PERM;
-
     if let Some(m) = matches.get_one::<String>(options::MODE) {
-        for mode in m.split(',') {
-            if mode.chars().any(|c| c.is_ascii_digit()) {
-                new_mode = mode::parse_numeric(new_mode, m, true)?;
-            } else {
-                new_mode = mode::parse_symbolic(new_mode, mode, mode::get_umask(), true)?;
-            }
-        }
-        Ok(new_mode)
+        mode::parse_chmod(DEFAULT_PERM, m, true, mode::get_umask())
     } else {
         // If no mode argument is specified return the mode derived from umask
-        Ok(!mode::get_umask() & 0o0777)
+        Ok(!mode::get_umask() & DEFAULT_PERM)
     }
 }
 

src/uu/mkfifo/src/mkfifo.rs

@@ -119,19 +119,11 @@ pub fn uu_app() -> Command {
 
 fn calculate_mode(mode_option: Option<&String>) -> Result<u32, String> {
     let umask = uucore::mode::get_umask();
-    let mut mode = 0o666; // Default mode for FIFOs
+    let mode = 0o666; // Default mode for FIFOs
 
     if let Some(m) = mode_option {
-        if m.chars().any(|c| c.is_ascii_digit()) {
-            mode = uucore::mode::parse_numeric(mode, m, false)?;
-        } else {
-            for item in m.split(',') {
-                mode = uucore::mode::parse_symbolic(mode, item, umask, false)?;
-            }
-        }
+        uucore::mode::parse_chmod(mode, m, false, umask)
     } else {
-        mode &= !umask; // Apply umask if no mode is specified
+        Ok(mode & !umask) // Apply umask if no mode is specified
     }
-
-    Ok(mode)
 }

src/uu/mknod/src/mknod.rs

@@ -225,8 +225,10 @@ pub fn uu_app() -> Command {
         )
 }
 
+#[allow(clippy::unnecessary_cast)]
 fn parse_mode(str_mode: &str) -> Result<mode_t, String> {
-    uucore::mode::parse_mode(str_mode)
+    let default_mode = (S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH) as u32;
+    uucore::mode::parse_chmod(default_mode, str_mode, true, uucore::mode::get_umask())
         .map_err(|e| {
             translate!(
                 "mknod-error-invalid-mode",
@@ -237,7 +239,7 @@ fn parse_mode(str_mode: &str) -> Result<mode_t, String> {
             if mode > 0o777 {
                 Err(translate!("mknod-error-mode-permission-bits-only"))
             } else {
-                Ok(mode)
+                Ok(mode as mode_t)
             }
         })
 }

src/uucore/src/lib/features/mode.rs

@@ -7,7 +7,7 @@
 
 // spell-checker:ignore (vars) fperm srwx
 
-use libc::{S_IRGRP, S_IROTH, S_IRUSR, S_IWGRP, S_IWOTH, S_IWUSR, mode_t, umask};
+use libc::umask;
 
 pub fn parse_numeric(fperm: u32, mut mode: &str, considering_dir: bool) -> Result<u32, String> {
     let (op, pos) = parse_op(mode).map_or_else(|_| (None, 0), |(op, pos)| (Some(op), pos));
@@ -137,23 +137,36 @@ fn parse_change(mode: &str, fperm: u32, considering_dir: bool) -> (u32, usize) {
     (srwx, pos)
 }
 
-#[allow(clippy::unnecessary_cast)]
-pub fn parse_mode(mode: &str) -> Result<mode_t, String> {
-    #[cfg(all(
-        not(target_os = "freebsd"),
-        not(target_vendor = "apple"),
-        not(target_os = "android")
-    ))]
-    let fperm = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH;
-    #[cfg(any(target_os = "freebsd", target_vendor = "apple", target_os = "android"))]
-    let fperm = (S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH) as u32;
+/// Modify a file mode based on a user-supplied string.
+/// Supports comma-separated mode strings like "ug+rwX,o+rX" (same as chmod).
+pub fn parse_chmod(
+    current_mode: u32,
+    mode_string: &str,
+    considering_dir: bool,
+    umask: u32,
+) -> Result<u32, String> {
+    let mut new_mode: u32 = current_mode;
 
-    let result = if mode.chars().any(|c| c.is_ascii_digit()) {
-        parse_numeric(fperm as u32, mode, true)
-    } else {
-        parse_symbolic(fperm as u32, mode, get_umask(), true)
-    };
-    result.map(|mode| mode as mode_t)
+    // Split by commas and process each mode part sequentially
+    for mode_part in mode_string.split(',') {
+        let mode_part = mode_part.trim();
+        if mode_part.is_empty() {
+            continue;
+        }
+
+        new_mode = if mode_part.chars().any(|c| c.is_ascii_digit()) {
+            parse_numeric(new_mode, mode_part, considering_dir)?
+        } else {
+            parse_symbolic(new_mode, mode_part, umask, considering_dir)?
+        };
+    }
+
+    Ok(new_mode)
+}
+
+/// Takes a user-supplied string and tries to parse to u32 mode bitmask.
+pub fn parse(mode_string: &str, considering_dir: bool, umask: u32) -> Result<u32, String> {
+    parse_chmod(0, mode_string, considering_dir, umask)
 }
 
 pub fn get_umask() -> u32 {
@@ -183,23 +196,142 @@ pub fn get_umask() -> u32 {
 }
 
 #[cfg(test)]
-mod test {
+mod tests {
+
+    use super::parse;
+    use super::parse_chmod;
 
     #[test]
-    fn symbolic_modes() {
-        assert_eq!(super::parse_mode("u+x").unwrap(), 0o766);
-        assert_eq!(
-            super::parse_mode("+x").unwrap(),
-            if crate::os::is_wsl_1() { 0o776 } else { 0o777 }
-        );
-        assert_eq!(super::parse_mode("a-w").unwrap(), 0o444);
-        assert_eq!(super::parse_mode("g-r").unwrap(), 0o626);
+    fn test_chmod_symbolic_modes() {
+        assert_eq!(parse_chmod(0o666, "u+x", false, 0).unwrap(), 0o766);
+        assert_eq!(parse_chmod(0o666, "+x", false, 0).unwrap(), 0o777);
+        assert_eq!(parse_chmod(0o666, "a-w", false, 0).unwrap(), 0o444);
+        assert_eq!(parse_chmod(0o666, "g-r", false, 0).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);
+    fn test_chmod_numeric_modes() {
+        assert_eq!(parse_chmod(0o666, "644", false, 0).unwrap(), 0o644);
+        assert_eq!(parse_chmod(0o666, "+100", false, 0).unwrap(), 0o766);
+        assert_eq!(parse_chmod(0o666, "-4", false, 0).unwrap(), 0o662);
+    }
+
+    #[test]
+    fn test_parse_numeric_mode() {
+        // Simple numeric mode
+        assert_eq!(parse("644", false, 0).unwrap(), 0o644);
+        assert_eq!(parse("755", false, 0).unwrap(), 0o755);
+        assert_eq!(parse("777", false, 0).unwrap(), 0o777);
+        assert_eq!(parse("600", false, 0).unwrap(), 0o600);
+    }
+
+    #[test]
+    fn test_parse_numeric_mode_with_operator() {
+        // Numeric mode with + operator
+        assert_eq!(parse("+100", false, 0).unwrap(), 0o100);
+        assert_eq!(parse("+644", false, 0).unwrap(), 0o644);
+
+        // Numeric mode with - operator (starting from 0, so nothing to remove)
+        assert_eq!(parse("-4", false, 0).unwrap(), 0);
+        // But if we first set a mode, then remove bits
+        assert_eq!(parse("644,-4", false, 0).unwrap(), 0o640);
+    }
+
+    #[test]
+    fn test_parse_symbolic_mode() {
+        // Simple symbolic modes
+        assert_eq!(parse("u+x", false, 0).unwrap(), 0o100);
+        assert_eq!(parse("g+w", false, 0).unwrap(), 0o020);
+        assert_eq!(parse("o+r", false, 0).unwrap(), 0o004);
+        assert_eq!(parse("a+x", false, 0).unwrap(), 0o111);
+    }
+
+    #[test]
+    fn test_parse_symbolic_mode_multiple_permissions() {
+        // Multiple permissions in one mode
+        assert_eq!(parse("u+rw", false, 0).unwrap(), 0o600);
+        assert_eq!(parse("ug+rwx", false, 0).unwrap(), 0o770);
+        assert_eq!(parse("a+rwx", false, 0).unwrap(), 0o777);
+    }
+
+    #[test]
+    fn test_parse_comma_separated_modes() {
+        // Comma-separated mode strings (as mentioned in the doc comment)
+        assert_eq!(parse("ug+rwX,o+rX", false, 0).unwrap(), 0o664);
+        assert_eq!(parse("u+rwx,g+rx,o+r", false, 0).unwrap(), 0o754);
+        assert_eq!(parse("u+w,g+w,o+w", false, 0).unwrap(), 0o222);
+    }
+
+    #[test]
+    fn test_parse_comma_separated_with_spaces() {
+        // Comma-separated with spaces (should be trimmed)
+        assert_eq!(parse("u+rw, g+rw, o+r", false, 0).unwrap(), 0o664);
+        assert_eq!(parse(" u+x , g+x ", false, 0).unwrap(), 0o110);
+    }
+
+    #[test]
+    fn test_parse_mixed_numeric_and_symbolic() {
+        // Mix of numeric and symbolic modes
+        assert_eq!(parse("644,u+x", false, 0).unwrap(), 0o744);
+        assert_eq!(parse("u+rw,755", false, 0).unwrap(), 0o755);
+    }
+
+    #[test]
+    fn test_parse_empty_string() {
+        // Empty string should return 0
+        assert_eq!(parse("", false, 0).unwrap(), 0);
+        assert_eq!(parse("   ", false, 0).unwrap(), 0);
+        assert_eq!(parse(",,", false, 0).unwrap(), 0);
+    }
+
+    #[test]
+    fn test_parse_with_umask() {
+        // Test with umask (affects symbolic modes when no level is specified)
+        let umask = 0o022;
+        assert_eq!(parse("+w", false, umask).unwrap(), 0o200);
+        // The umask should be respected for symbolic modes without explicit level
+    }
+
+    #[test]
+    fn test_parse_considering_dir() {
+        // Test directory vs file mode differences
+        // For directories, X (capital X) should add execute permission
+        assert_eq!(parse("a+X", true, 0).unwrap(), 0o111);
+        // For files without execute, X should not add execute
+        assert_eq!(parse("a+X", false, 0).unwrap(), 0o000);
+
+        // Numeric modes for directories preserve setuid/setgid bits
+        assert_eq!(parse("755", true, 0).unwrap(), 0o755);
+    }
+
+    #[test]
+    fn test_parse_invalid_modes() {
+        // Invalid numeric mode (too large)
+        assert!(parse("10000", false, 0).is_err());
+
+        // Invalid operator
+        assert!(parse("u*rw", false, 0).is_err());
+
+        // Invalid symbolic mode
+        assert!(parse("invalid", false, 0).is_err());
+    }
+
+    #[test]
+    fn test_parse_complex_combinations() {
+        // Complex real-world examples
+        assert_eq!(parse("u=rwx,g=rx,o=r", false, 0).unwrap(), 0o754);
+        // To test removal, we need to first set permissions, then remove them
+        assert_eq!(parse("644,a-w", false, 0).unwrap(), 0o444);
+        assert_eq!(parse("644,g-r", false, 0).unwrap(), 0o604);
+    }
+
+    #[test]
+    fn test_parse_sequential_application() {
+        // Test that comma-separated modes are applied sequentially
+        // First set to 644, then add execute for user
+        assert_eq!(parse("644,u+x", false, 0).unwrap(), 0o744);
+
+        // First add user write, then set to 755 (should override)
+        assert_eq!(parse("u+w,755", false, 0).unwrap(), 0o755);
     }
 }

tests/by-util/test_mkfifo.rs

@@ -99,6 +99,8 @@ fn test_create_fifo_with_mode_and_umask() {
     test_fifo_creation("u-r,g-w,o+x", 0o022, "p-w-r--rwx"); // spell-checker:disable-line
     test_fifo_creation("a=rwx,o-w", 0o022, "prwxrwxr-x"); // spell-checker:disable-line
     test_fifo_creation("=rwx,o-w", 0o022, "prwxr-xr-x"); // spell-checker:disable-line
+    test_fifo_creation("ug+rw,o+r", 0o022, "prw-rw-rw-"); // spell-checker:disable-line
+    test_fifo_creation("u=rwx,g=rx,o=", 0o022, "prwxr-x---"); // spell-checker:disable-line
 }
 
 #[test]

tests/by-util/test_mknod.rs

@@ -154,6 +154,22 @@ fn test_mknod_mode_permissions() {
     }
 }
 
+#[test]
+fn test_mknod_mode_comma_separated() {
+    let ts = TestScenario::new(util_name!());
+    ts.ucmd()
+        .arg("-m")
+        .arg("u=rwx,g=rx,o=")
+        .arg("test_file")
+        .arg("p")
+        .succeeds();
+    assert!(ts.fixtures.is_fifo("test_file"));
+    assert_eq!(
+        ts.fixtures.metadata("test_file").permissions().mode() & 0o777,
+        0o750
+    );
+}
+
 #[test]
 #[cfg(feature = "feat_selinux")]
 fn test_mknod_selinux() {