stty: set control characters #7931

willshuttleworth · 2025-05-13T14:39:41Z

Added functionality to set mappings between control characters (erase, intr, kill, etc.) and specific characters given by the user (^C, for example).

I had to rework the argument processing as it was previously processing one flag at a time, but this feature requires processing of consecutive args (ie stty erase ^C). Added string_to_control_char() to convert strings like "^C", "X", "0x7F" to their ASCII value if they are valid control char mappings. Also added apply_char_mapping() to set new control char mappings using termios.

I'm a relatively new Rust programmer, so I'd love feedback on things that could be written more idiomatically or concisely. This PR is related to issue #7357.

github-actions · 2025-05-13T15:31:34Z

GNU testsuite comparison:

Skip an intermittent issue tests/misc/stdbuf (fails in this run but passes in the 'main' branch)

cakebaker · 2025-05-15T13:34:14Z

src/uu/stty/src/stty.rs

+                match apply_char_mapping(&mut termios, char_index, new_cc) {
+                    Ok(_) => {}
+                    Err(e) => match e {
+                        ControlCharMappingError::IntOutOfRange => {
+                            return Err(USimpleError::new(
+                                1,
+                                format!(
+                                    "invalid integer argument: '{new_cc}': Numerical result out of range"
+                                ),
+                            ));
+                        }
+                        ControlCharMappingError::MultipleChars => {
+                            return Err(USimpleError::new(
+                                1,
+                                format!("invalid integer argument: '{new_cc}'"),
+                            ));
+                        }
+                    },
+                }


As you only handle the error case, you can simplify it with map_err:

Suggested change

match apply_char_mapping(&mut termios, char_index, new_cc) {

Ok(_) => {}

Err(e) => match e {

ControlCharMappingError::IntOutOfRange => {

return Err(USimpleError::new(

1,

format!(

"invalid integer argument: '{new_cc}': Numerical result out of range"

),

));

}

ControlCharMappingError::MultipleChars => {

return Err(USimpleError::new(

1,

format!("invalid integer argument: '{new_cc}'"),

));

}

},

}

apply_char_mapping(&mut termios, char_index, new_cc).map_err(|e| {

let message = match e {

ControlCharMappingError::IntOutOfRange => format!(

"invalid integer argument: '{new_cc}': Numerical result out of range"

),

ControlCharMappingError::MultipleChars => {

format!("invalid integer argument: '{new_cc}'")

}

};

USimpleError::new(1, message)

})?;

cakebaker · 2025-05-15T13:39:41Z

src/uu/stty/src/stty.rs

    Ok(())
 }

+fn is_control_char(option: &str) -> Option<SpecialCharacterIndices> {


This function name is a bit misleading: functions starting with is_ usually return a bool.

initially i was returning a bool but then changed it to an option, good catch

cakebaker · 2025-05-15T14:16:34Z

src/uu/stty/src/stty.rs

+    match string_to_control_char(new_val) {
+        Ok(val) => {
+            termios.control_chars[control_char_index as usize] = val;
+            Ok(())
+        }
+        Err(e) => Err(e),
+    }


I think you can simplify it to something like:

Suggested change

match string_to_control_char(new_val) {

Ok(val) => {

termios.control_chars[control_char_index as usize] = val;

Ok(())

}

Err(e) => Err(e),

}

string_to_control_char(new_val).and_then(|val| {

termios.control_chars[control_char_index as usize] = val;

Ok(())

})

cakebaker · 2025-05-15T14:19:43Z

src/uu/stty/src/stty.rs

+//      c. decimal, no prefix
+// 3. Disabling the control character: '^-' or 'undef'
+//
+// This function returns the ascii value of valid control chars, or None if the input is invalid


The function returns a ControlCharMappingError in the error case, not None.

cakebaker · 2025-05-15T14:39:20Z

src/uu/stty/src/stty.rs

+    let mut ascii_num: Option<u32> = None;
+    if let Some(hex) = s.strip_prefix("0x") {
+        ascii_num = u32::from_str_radix(hex, 16).ok();
+    } else if let Some(octal) = s.strip_prefix("0") {
+        ascii_num = u32::from_str_radix(octal, 8).ok();
+    } else if let Ok(decimal) = s.parse::<u32>() {
+        ascii_num = Some(decimal);
+    }


ascii_num doesn't have to be mutable. And you can get rid of the last if let:

Suggested change

let mut ascii_num: Option<u32> = None;

if let Some(hex) = s.strip_prefix("0x") {

ascii_num = u32::from_str_radix(hex, 16).ok();

} else if let Some(octal) = s.strip_prefix("0") {

ascii_num = u32::from_str_radix(octal, 8).ok();

} else if let Ok(decimal) = s.parse::<u32>() {

ascii_num = Some(decimal);

}

let ascii_num = if let Some(hex) = s.strip_prefix("0x") {

u32::from_str_radix(hex, 16).ok()

} else if let Some(octal) = s.strip_prefix("0") {

u32::from_str_radix(octal, 8).ok()

} else {

s.parse::<u32>().ok()

};

github-actions · 2025-05-15T14:57:03Z

GNU testsuite comparison:

Skip an intermittent issue tests/misc/usage_vs_getopt (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

willshuttleworth · 2025-05-15T15:45:11Z

anything else you'd like me to fix? i can push the final review commit now if we're all good

cakebaker · 2025-05-15T15:58:38Z

@willshuttleworth from my side it looks good, maybe @tertsdiepraam has something to add as he wrote most of the current implementation.

willshuttleworth · 2025-05-16T20:30:10Z

@cakebaker i have committed the final code review related changes

github-actions · 2025-05-17T05:52:14Z

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

sylvestre · 2025-05-20T07:55:57Z

it would be nice to have tests

willshuttleworth · 2025-05-20T18:07:41Z

the one issue with testing is that cargo test doesnt run in a tty, so i would need to do some reworking to test properly. there are two main options i can imagine now. this first is reworking uumain() so that control char parsing is done before the call to tcgetattr() and i can test for argument combinations that fail. or, i could rework the way testing is done so that a separate tty process is spawned and used. this crate seems to accomplish that. the first option seems to be the easier and more pragmatic approach, but i could be ignorant of better options.

sylvestre · 2025-05-20T18:44:50Z

that would be ideal
you could also start by testing at function level (like cc_to_index)

willshuttleworth · 2025-05-24T17:44:08Z

ok, i have reworked the earlier changes so that some testing can be done. the new approach is to parse and collect all arguments, return errors if necessary, then apply the new flags. none of the tty related calls are made until parsing is complete, so errors during parsing can be tested for.

github-actions · 2025-05-24T18:29:27Z

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

github-actions · 2025-05-25T21:45:35Z

GNU testsuite comparison:

Skip an intermittent issue tests/misc/stdbuf (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/misc/tee (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

github-actions · 2025-06-02T09:57:18Z

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

cakebaker · 2025-06-03T09:00:32Z

src/uu/stty/src/stty.rs

+                let mut remove_group = false;
+                match flag {
+                    AllFlags::Baud(_) => {}
+                    AllFlags::ControlFlags((flag, remove)) => {
+                        remove_group = check_flag_group(flag, remove);
+                    }
+                    AllFlags::InputFlags((flag, remove)) => {
+                        remove_group = check_flag_group(flag, remove);
+                    }
+                    AllFlags::LocalFlags((flag, remove)) => {
+                        remove_group = check_flag_group(flag, remove);
+                    }
+                    AllFlags::OutputFlags((flag, remove)) => {
+                        remove_group = check_flag_group(flag, remove);
+                    }
+                }


You can simplify the match to:

Suggested change

let mut remove_group = false;

match flag {

AllFlags::Baud(_) => {}

AllFlags::ControlFlags((flag, remove)) => {

remove_group = check_flag_group(flag, remove);

}

AllFlags::InputFlags((flag, remove)) => {

remove_group = check_flag_group(flag, remove);

}

AllFlags::LocalFlags((flag, remove)) => {

remove_group = check_flag_group(flag, remove);

}

AllFlags::OutputFlags((flag, remove)) => {

remove_group = check_flag_group(flag, remove);

}

}

let remove_group = match flag {

AllFlags::Baud(_) => false,

AllFlags::ControlFlags((flag, remove)) => check_flag_group(flag, remove),

AllFlags::InputFlags((flag, remove)) => check_flag_group(flag, remove),

AllFlags::LocalFlags((flag, remove)) => check_flag_group(flag, remove),

AllFlags::OutputFlags((flag, remove)) => check_flag_group(flag, remove),

};

cakebaker · 2025-06-03T09:04:20Z

src/uu/stty/src/stty.rs

+    if remove && flag.group.is_some() {
+        return true;
+    }
+    false


You can simplify it to:

Suggested change

if remove && flag.group.is_some() {

return true;

}

false

remove && flag.group.is_some()

cakebaker · 2025-06-03T09:27:27Z

src/uu/stty/src/stty.rs

+    let (remove, name) = match option.strip_prefix('-') {
+        Some(s) => (true, s),
+        None => (false, option),
+    };


I think you can simplify this with something like:

let remove = option.starts_with('-'); let name = option.trim_start_matches('-');

This assumes that option doesn't start with multiple -. Otherwise you could use strip_prefix with unwrap_or_else.

willshuttleworth · 2025-06-03T14:06:29Z

@cakebaker i have integrated the changes from your review

github-actions · 2025-06-03T14:37:17Z

GNU testsuite comparison:

Skip an intermittent issue tests/misc/tee (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

…uped now, ie 'stty erase ^H'

…ings

github-actions · 2025-06-05T09:32:03Z

GNU testsuite comparison:

Skip an intermittent issue tests/misc/tee (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

cakebaker · 2025-06-05T09:39:01Z

Thanks!

willshuttleworth · 2025-06-05T14:09:21Z

thank you for all the help :) also, we can close this issue now.

cakebaker reviewed May 15, 2025

View reviewed changes

willshuttleworth force-pushed the stty-set-control-chars branch from 9d0ca57 to 8a6e390 Compare May 15, 2025 14:08

cakebaker reviewed May 15, 2025

View reviewed changes

sylvestre force-pushed the stty-set-control-chars branch 2 times, most recently from 23f7c1b to fdbd248 Compare May 20, 2025 07:45

willshuttleworth force-pushed the stty-set-control-chars branch from 550aa8b to 20134d2 Compare May 24, 2025 17:41

willshuttleworth mentioned this pull request May 27, 2025

stty: fix output redirection #8016

Merged

sylvestre force-pushed the stty-set-control-chars branch from 0f4067e to e1eee82 Compare June 2, 2025 09:13

cakebaker reviewed Jun 3, 2025

View reviewed changes

willshuttleworth force-pushed the stty-set-control-chars branch from e1eee82 to 0bd75fa Compare June 3, 2025 13:46

willshuttleworth added 12 commits June 5, 2025 10:33

reworked arg processing. control character mappings are correctly gro…

62a3976

…uped now, ie 'stty erase ^H'

stty: setting control chars to undefined (disabling them) is implemented

ae0c61d

setting control chars

eab54ec

stty: can now set control chars. need to improve checks on valid mapp…

39b28b2

…ings

stty: matches GNU in what control character mappings are allowed

3789d0e

stty: run rustfmt and remove extra comments

6619857

stty: setting control char code review fixes

da94aa9

stty: fix rustfmt errors

061a4e4

stty: more small edits after review

a40c389

stty: refactor set control char changes for better testing

7c1d7bc

stty: fix ci error

c99dc33

stty: fix issues from code review

7ce7b5c

cakebaker force-pushed the stty-set-control-chars branch from 0bd75fa to 7ce7b5c Compare June 5, 2025 08:40

cakebaker merged commit 61bd11a into uutils:main Jun 5, 2025
74 checks passed

cakebaker mentioned this pull request Jun 5, 2025

stty erase returns "invalid argument" on MacOS #7357

Closed

This was referenced Jun 9, 2025

stty: special characters #3861

Closed

stty: implement "Special Control Character Assignments" #6799

Closed

BrewTestBot mentioned this pull request Sep 6, 2025

uutils-coreutils 0.2.0 Homebrew/homebrew-core#236403

Merged

Uh oh!

stty: set control characters #7931

stty: set control characters #7931

Uh oh!

Conversation

willshuttleworth commented May 13, 2025

Uh oh!

github-actions bot commented May 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 15, 2025

Uh oh!

willshuttleworth commented May 15, 2025

Uh oh!

cakebaker commented May 15, 2025

Uh oh!

willshuttleworth commented May 16, 2025

Uh oh!

github-actions bot commented May 17, 2025

Uh oh!

sylvestre commented May 20, 2025

Uh oh!

willshuttleworth commented May 20, 2025

Uh oh!

sylvestre commented May 20, 2025

Uh oh!

willshuttleworth commented May 24, 2025

Uh oh!

github-actions bot commented May 24, 2025

Uh oh!

github-actions bot commented May 25, 2025

Uh oh!

github-actions bot commented Jun 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willshuttleworth commented Jun 3, 2025

Uh oh!

github-actions bot commented Jun 3, 2025

Uh oh!

github-actions bot commented Jun 5, 2025

Uh oh!

Uh oh!

cakebaker commented Jun 5, 2025

Uh oh!

willshuttleworth commented Jun 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants