From 8aa9f346efde2a1c573ef0c1348f036201831baf Mon Sep 17 00:00:00 2001 From: Ed Smith Date: Thu, 12 Jan 2023 15:42:06 +0000 Subject: [PATCH 1/3] Support legacy argument syntax for nice This introduces an argument preprocessing step for the nice tool in order to support the legacy nice adjustment syntax (`nice -1 foo`, `nice --1 foo`, `nice -+1 foo` and so forth). This is a preprocessing step because the interaction between these arguments and the existing `nice -n -1` syntax becomes context dependent, in a way that is currently difficult to capture through clap. Signed-off-by: Ed Smith --- src/uu/nice/src/nice.rs | 83 +++++++++++++++++++++++++++++++++++++- tests/by-util/test_nice.rs | 19 +++++++++ 2 files changed, 101 insertions(+), 1 deletion(-) diff --git a/src/uu/nice/src/nice.rs b/src/uu/nice/src/nice.rs index a2c8ceeef..ddd804cf2 100644 --- a/src/uu/nice/src/nice.rs +++ b/src/uu/nice/src/nice.rs @@ -8,7 +8,7 @@ // spell-checker:ignore (ToDO) getpriority execvp setpriority nstr PRIO cstrs ENOENT use libc::{c_char, c_int, execvp, PRIO_PROCESS}; -use std::ffi::CString; +use std::ffi::{CString, OsString}; use std::io::Error; use std::ptr; @@ -30,8 +30,89 @@ const ABOUT: &str = "\ process)."; const USAGE: &str = "{} [OPTIONS] [COMMAND [ARGS]]"; +fn is_prefix_of(maybe_prefix: &str, target: &str, min_match: usize) -> bool { + if maybe_prefix.len() < min_match || maybe_prefix.len() > target.len() { + return false; + } + + &target[0..maybe_prefix.len()] == maybe_prefix +} + +/// Transform legacy arguments into a standardized form. +/// +/// The following are all legal argument sequences to GNU nice: +/// - "-1" +/// - "-n1" +/// - "-+1" +/// - "--1" +/// - "-n -1" +/// +/// It looks initially like we could add handling for "-{i}", "--{i}" +/// and "-+{i}" for integers {i} and process them normally using clap. +/// However, the meaning of "-1", for example, changes depending on +/// its context with legacy argument parsing. clap will not prioritize +/// hyphenated values to previous arguments over matching a known +/// argument. So "-n" "-1" in this case is picked up as two +/// arguments, not one argument with a value. +/// +/// Given this context dependency, and the deep hole we end up digging +/// with clap in this case, it's much simpler to just normalize the +/// arguments to nice before clap starts work. Here, we insert a +/// prefix of "-n" onto all arguments of the form "-{i}", "--{i}" and +/// "-+{i}" which are not already preceded by "-n". +fn standardize_nice_args(mut args: impl uucore::Args) -> impl uucore::Args { + let mut v = Vec::::new(); + let mut saw_n = false; + let mut saw_command = false; + if let Some(cmd) = args.next() { + v.push(cmd); + } + for s in args { + if saw_command { + v.push(s); + } else if saw_n { + let mut new_arg: OsString = "-n".into(); + new_arg.push(s); + v.push(new_arg); + saw_n = false; + } else if s.to_str() == Some("-n") + || s.to_str() + .map(|s| is_prefix_of(s, "--adjustment", "--a".len())) + .unwrap_or_default() + { + saw_n = true; + } else if let Ok(s) = s.clone().into_string() { + if let Some(stripped) = s.strip_prefix('-') { + match stripped.parse::() { + Ok(ix) => { + let mut new_arg: OsString = "-n".into(); + new_arg.push(ix.to_string()); + v.push(new_arg); + } + Err(_) => { + v.push(s.into()); + } + } + } else { + saw_command = true; + v.push(s.into()); + } + } else { + saw_command = true; + v.push(s); + } + } + if saw_n { + v.push("-n".into()); + } + + v.into_iter() +} + #[uucore::main] pub fn uumain(args: impl uucore::Args) -> UResult<()> { + let args = standardize_nice_args(args); + let matches = uu_app().try_get_matches_from(args).with_exit_code(125)?; nix::errno::Errno::clear(); diff --git a/tests/by-util/test_nice.rs b/tests/by-util/test_nice.rs index 036723cde..6fca05ab4 100644 --- a/tests/by-util/test_nice.rs +++ b/tests/by-util/test_nice.rs @@ -63,3 +63,22 @@ fn test_command_where_command_takes_n_flag() { fn test_invalid_argument() { new_ucmd!().arg("--invalid").fails().code_is(125); } + +#[test] +fn test_bare_adjustment() { + new_ucmd!() + .args(&["-1", "echo", "-n", "a"]) + .run() + .stdout_is("a"); +} + +#[test] +fn test_trailing_empty_adjustment() { + new_ucmd!() + .args(&["-n", "1", "-n"]) + .fails() + .stderr_str() + .starts_with( + "error: The argument '--adjustment ' requires a value but none was supplied", + ); +} From 07e737231141fede40131b2f50e0a3a6d3f5b106 Mon Sep 17 00:00:00 2001 From: Ed Smith Date: Thu, 12 Jan 2023 15:43:05 +0000 Subject: [PATCH 2/3] nice: Permit --adjustment to be specified multiple times This is tested by the GNU coreutils test suite, and the correct behaviour is the last specification wins. Signed-off-by: Ed Smith --- src/uu/nice/src/nice.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/uu/nice/src/nice.rs b/src/uu/nice/src/nice.rs index ddd804cf2..dd507e33f 100644 --- a/src/uu/nice/src/nice.rs +++ b/src/uu/nice/src/nice.rs @@ -190,6 +190,8 @@ pub fn uu_app() -> Command { .short('n') .long(options::ADJUSTMENT) .help("add N to the niceness (default is 10)") + .action(ArgAction::Set) + .overrides_with(options::ADJUSTMENT) .allow_hyphen_values(true), ) .arg( From 34e10f9aa83ebf6b93b11f40095dd70eb7b12156 Mon Sep 17 00:00:00 2001 From: Ed Smith Date: Sun, 15 Jan 2023 12:26:14 +0000 Subject: [PATCH 3/3] nice: Remove use of show_warning This is required to pass the GNU nice test suite. Failure to produce the advisory message when unable to change the process priority must be fatal, and without this commit our version of nice will exit, but before the commit the exit code will be 101 (due to eprintln! panicking), and it must be 125 to pass the test suite. Signed-off-by: Ed Smith --- src/uu/nice/src/nice.rs | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/uu/nice/src/nice.rs b/src/uu/nice/src/nice.rs index dd507e33f..e6efb5140 100644 --- a/src/uu/nice/src/nice.rs +++ b/src/uu/nice/src/nice.rs @@ -9,13 +9,13 @@ use libc::{c_char, c_int, execvp, PRIO_PROCESS}; use std::ffi::{CString, OsString}; -use std::io::Error; +use std::io::{Error, Write}; use std::ptr; use clap::{crate_version, Arg, ArgAction, Command}; use uucore::{ error::{set_exit_code, UClapError, UResult, USimpleError, UUsageError}, - format_usage, show_error, show_warning, + format_usage, show_error, }; pub mod options { @@ -152,8 +152,21 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { }; niceness += adjustment; - if unsafe { libc::setpriority(PRIO_PROCESS, 0, niceness) } == -1 { - show_warning!("setpriority: {}", Error::last_os_error()); + // We can't use `show_warning` because that will panic if stderr + // isn't writable. The GNU test suite checks specifically that the + // exit code when failing to write the advisory is 125, but Rust + // will produce an exit code of 101 when it panics. + if unsafe { libc::setpriority(PRIO_PROCESS, 0, niceness) } == -1 + && write!( + std::io::stderr(), + "{}: warning: setpriority: {}", + uucore::util_name(), + Error::last_os_error() + ) + .is_err() + { + set_exit_code(125); + return Ok(()); } let cstrs: Vec = matches