From 872c0fac1d08850e2fffbe3abcbb65df4f9f5711 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Olivier=20EBL=C3=89?= Date: Sat, 11 Sep 2021 22:37:55 +0200 Subject: [PATCH 1/2] tests/kill: test old form args with signal name Add two tests of the old form signal arg using the signal name instead of the number. Bonus: add a test for the new form but with the prefix SIG- --- tests/by-util/test_kill.rs | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/tests/by-util/test_kill.rs b/tests/by-util/test_kill.rs index fe5d4557a..f5166c428 100644 --- a/tests/by-util/test_kill.rs +++ b/tests/by-util/test_kill.rs @@ -104,6 +104,26 @@ fn test_kill_with_signal_number_old_form() { assert_eq!(target.wait_for_signal(), Some(9)); } +#[test] +fn test_kill_with_signal_name_old_form() { + let mut target = Target::new(); + new_ucmd!() + .arg("-KILL") + .arg(format!("{}", target.pid())) + .succeeds(); + assert_eq!(target.wait_for_signal(), Some(libc::SIGKILL)); +} + +#[test] +fn test_kill_with_signal_prefixed_name_old_form() { + let mut target = Target::new(); + new_ucmd!() + .arg("-SIGKILL") + .arg(format!("{}", target.pid())) + .succeeds(); + assert_eq!(target.wait_for_signal(), Some(libc::SIGKILL)); +} + #[test] fn test_kill_with_signal_number_new_form() { let mut target = Target::new(); @@ -125,3 +145,14 @@ fn test_kill_with_signal_name_new_form() { .succeeds(); assert_eq!(target.wait_for_signal(), Some(libc::SIGKILL)); } + +#[test] +fn test_kill_with_signal_prefixed_name_new_form() { + let mut target = Target::new(); + new_ucmd!() + .arg("-s") + .arg("SIGKILL") + .arg(format!("{}", target.pid())) + .succeeds(); + assert_eq!(target.wait_for_signal(), Some(libc::SIGKILL)); +} From df64c191078f613726ba347e30320d4fac2ff284 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Olivier=20EBL=C3=89?= Date: Sun, 12 Sep 2021 00:44:11 +0200 Subject: [PATCH 2/2] kill: kill old form with signal name This commit aim to correct #2645. The origin of the problem was that `handle_obsolete` was not looking for named signals but only for numbers preceded by '-'. I have made a few changes: - Change `handle_obsolete` to use `signal_by_name_or_value` to parse the possible signal. - Since now the signal is already parsed return an `Option` instead of `Option<&str>` to parse later. - Change the way to decide the pid to use from a `match` to a `if` because the tested element are actually not the same for each branch. - Parse the signal from the `-s` argument outside of `kill` function for consistency with the "obsolete" signal case. - Again for consistency, parse the pids outside the `kill` function. --- src/uu/kill/src/kill.rs | 99 ++++++++++++++++++++--------------------- 1 file changed, 49 insertions(+), 50 deletions(-) diff --git a/src/uu/kill/src/kill.rs b/src/uu/kill/src/kill.rs index 494dc0602..f269f7283 100644 --- a/src/uu/kill/src/kill.rs +++ b/src/uu/kill/src/kill.rs @@ -15,7 +15,7 @@ use libc::{c_int, pid_t}; use std::io::Error; use uucore::display::Quotable; use uucore::error::{UResult, USimpleError}; -use uucore::signals::ALL_SIGNALS; +use uucore::signals::{signal_by_name_or_value, ALL_SIGNALS}; use uucore::InvalidEncodingHandling; static ABOUT: &str = "Send signal to processes or list information about signals."; @@ -37,10 +37,10 @@ pub enum Mode { #[uucore_procs::gen_uumain] pub fn uumain(args: impl uucore::Args) -> UResult<()> { - let args = args + let mut args = args .collect_str(InvalidEncodingHandling::Ignore) .accept_any(); - let (args, obs_signal) = handle_obsolete(args); + let obs_signal = handle_obsolete(&mut args); let usage = format!("{} [OPTIONS]... PID...", uucore::execution_phrase()); let matches = uu_app().usage(&usage[..]).get_matches_from(args); @@ -60,13 +60,15 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { match mode { Mode::Kill => { - let sig = match (obs_signal, matches.value_of(options::SIGNAL)) { - (Some(s), Some(_)) => s, // -s takes precedence - (Some(s), None) => s, - (None, Some(s)) => s.to_owned(), - (None, None) => "TERM".to_owned(), + let sig = if let Some(signal) = obs_signal { + signal + } else if let Some(signal) = matches.value_of(options::SIGNAL) { + parse_signal_value(signal)? + } else { + 15_usize //SIGTERM }; - kill(&sig, &pids_or_signals) + let pids = parse_pids(&pids_or_signals)?; + kill(sig, &pids) } Mode::Table => { table(); @@ -109,26 +111,22 @@ pub fn uu_app() -> App<'static, 'static> { ) } -fn handle_obsolete(mut args: Vec) -> (Vec, Option) { - let mut i = 0; - while i < args.len() { - // this is safe because slice is valid when it is referenced - let slice = &args[i].clone(); - if slice.starts_with('-') && slice.chars().nth(1).map_or(false, |c| c.is_digit(10)) { - let val = &slice[1..]; - match val.parse() { - Ok(num) => { - if uucore::signals::is_signal(num) { - args.remove(i); - return (args, Some(val.to_owned())); - } - } - Err(_) => break, /* getopts will error out for us */ +fn handle_obsolete(args: &mut Vec) -> Option { + // Sanity check + if args.len() > 2 { + // Old signal can only be in the first argument position + let slice = args[1].as_str(); + if let Some(signal) = slice.strip_prefix('-') { + // Check if it is a valid signal + let opt_signal = signal_by_name_or_value(signal); + if opt_signal.is_some() { + // remove the signal before return + args.remove(1); + return opt_signal; } } - i += 1; } - (args, None) + None } fn table() { @@ -184,31 +182,32 @@ fn list(arg: Option) -> UResult<()> { } } -fn kill(signalname: &str, pids: &[String]) -> UResult<()> { - let optional_signal_value = uucore::signals::signal_by_name_or_value(signalname); - let signal_value = match optional_signal_value { - Some(x) => x, - None => { - return Err(USimpleError::new( - 1, - format!("unknown signal name {}", signalname.quote()), - )); +fn parse_signal_value(signal_name: &str) -> UResult { + let optional_signal_value = signal_by_name_or_value(signal_name); + match optional_signal_value { + Some(x) => Ok(x), + None => Err(USimpleError::new( + 1, + format!("unknown signal name {}", signal_name.quote()), + )), + } +} + +fn parse_pids(pids: &[String]) -> UResult> { + pids.iter() + .map(|x| { + x.parse::().map_err(|e| { + USimpleError::new(1, format!("failed to parse argument {}: {}", x.quote(), e)) + }) + }) + .collect() +} + +fn kill(signal_value: usize, pids: &[usize]) -> UResult<()> { + for &pid in pids { + if unsafe { libc::kill(pid as pid_t, signal_value as c_int) } != 0 { + show!(USimpleError::new(1, format!("{}", Error::last_os_error()))); } - }; - for pid in pids { - match pid.parse::() { - Ok(x) => { - if unsafe { libc::kill(x as pid_t, signal_value as c_int) } != 0 { - show!(USimpleError::new(1, format!("{}", Error::last_os_error()))); - } - } - Err(e) => { - return Err(USimpleError::new( - 1, - format!("failed to parse argument {}: {}", pid.quote(), e), - )); - } - }; } Ok(()) }