From be1869d58482ed29fb030effd9cabd7899939feb Mon Sep 17 00:00:00 2001 From: Tuomas Tynkkynen Date: Sun, 9 Feb 2025 22:15:53 +0200 Subject: [PATCH 1/5] pgrep, pkill: Unify arg lists Most of the process matching arguments are same between pkill and pgrep. Currently pgrep has more complete list of possible arguments (with proper types), so copy over the improvements to pgrep. However, leave arg!()s that are actually not implemented commented out, so it's easier to see what the uutils version actually supports. --- src/uu/pgrep/src/pgrep.rs | 28 ++++++++++++++++----- src/uu/pkill/src/pkill.rs | 53 +++++++++++++++++++-------------------- 2 files changed, 48 insertions(+), 33 deletions(-) diff --git a/src/uu/pgrep/src/pgrep.rs b/src/uu/pgrep/src/pgrep.rs index e59c5cb1..eeb016b3 100644 --- a/src/uu/pgrep/src/pgrep.rs +++ b/src/uu/pgrep/src/pgrep.rs @@ -293,8 +293,12 @@ pub fn uu_app() -> Command { // arg!(-w --lightweight "list all TID"), arg!(-c --count "count of matching processes"), arg!(-f --full "use full process name to match"), - // arg!(-g --pgroup ... "match listed process group IDs"), - // arg!(-G --group ... "match real group IDs"), + // arg!(-g --pgroup ... "match listed process group IDs") + // .value_delimiter(',') + // .value_parser(clap::value_parser!(u64)), + // arg!(-G --group ... "match real group IDs") + // .value_delimiter(',') + // .value_parser(clap::value_parser!(u64)), arg!(-i --"ignore-case" "match case insensitively"), arg!(-n --newest "select most recently started"), arg!(-o --oldest "select least recently started"), @@ -303,17 +307,29 @@ pub fn uu_app() -> Command { arg!(-P --parent "match only child processes of the given parent") .value_delimiter(',') .value_parser(clap::value_parser!(u64)), - // arg!(-s --session "match session IDs"), + // arg!(-s --session "match session IDs") + // .value_delimiter(',') + // .value_parser(clap::value_parser!(u64)), + // arg!(--signal "signal to send (either number or name)"), arg!(-t --terminal "match by controlling terminal") .value_delimiter(','), - // arg!(-u --euid ... "match by effective IDs"), - // arg!(-U --uid ... "match by real IDs"), + // arg!(-u --euid ... "match by effective IDs") + // .value_delimiter(',') + // .value_parser(clap::value_parser!(u64)), + // arg!(-U --uid ... "match by real IDs") + // .value_delimiter(',') + // .value_parser(clap::value_parser!(u64)), arg!(-x --exact "match exactly with the command name"), // arg!(-F --pidfile "read PIDs from file"), // arg!(-L --logpidfile "fail if PID file is not locked"), arg!(-r --runstates "match runstates [D,S,Z,...]"), + // arg!(-A --"ignore-ancestors" "exclude our ancestors from results"), + // arg!(--cgroup "match by cgroup v2 names") + // .value_delimiter(','), // arg!( --ns "match the processes that belong to the same namespace as "), - // arg!( --nslist ... "list which namespaces will be considered for the --ns option."), + // arg!( --nslist ... "list which namespaces will be considered for the --ns option.") + // .value_delimiter(',') + // .value_parser(["ipc", "mnt", "net", "pid", "user", "uts"]), ]) .arg( Arg::new("pattern") diff --git a/src/uu/pkill/src/pkill.rs b/src/uu/pkill/src/pkill.rs index 9d3fb782..b534af41 100644 --- a/src/uu/pkill/src/pkill.rs +++ b/src/uu/pkill/src/pkill.rs @@ -347,16 +347,16 @@ pub fn uu_app() -> Command { .args([ // arg!(- "signal to send (either number or name)"), arg!(-H --"require-handler" "match only if signal handler is present"), - arg!(-q --queue "integer value to be sent with the signal"), + // arg!(-q --queue "integer value to be sent with the signal"), arg!(-e --echo "display what is killed"), arg!(-c --count "count of matching processes"), arg!(-f --full "use full process name to match"), - arg!(-g --pgroup "match listed process group IDs") - .value_delimiter(',') - .value_parser(clap::value_parser!(u64)), - arg!(-G --group "match real group IDs") - .value_delimiter(',') - .value_parser(clap::value_parser!(u64)), + // arg!(-g --pgroup "match listed process group IDs") + // .value_delimiter(',') + // .value_parser(clap::value_parser!(u64)), + // arg!(-G --group "match real group IDs") + // .value_delimiter(',') + // .value_parser(clap::value_parser!(u64)), arg!(-i --"ignore-case" "match case insensitively"), arg!(-n --newest "select most recently started"), arg!(-o --oldest "select least recently started"), @@ -365,29 +365,28 @@ pub fn uu_app() -> Command { arg!(-P --parent "match only child processes of the given parent") .value_delimiter(',') .value_parser(clap::value_parser!(u64)), - arg!(-s --session "match session IDs") - .value_delimiter(',') - .value_parser(clap::value_parser!(u64)), + // arg!(-s --session "match session IDs") + // .value_delimiter(',') + // .value_parser(clap::value_parser!(u64)), arg!(--signal "signal to send (either number or name)"), - arg!(-t --terminal "match by controlling terminal") - .value_delimiter(','), - arg!(-u --euid "match by effective IDs") - .value_delimiter(',') - .value_parser(clap::value_parser!(u64)), - arg!(-U --uid "match by real IDs") - .value_delimiter(',') - .value_parser(clap::value_parser!(u64)), + arg!(-t --terminal "match by controlling terminal").value_delimiter(','), + // arg!(-u --euid "match by effective IDs") + // .value_delimiter(',') + // .value_parser(clap::value_parser!(u64)), + // arg!(-U --uid "match by real IDs") + // .value_delimiter(',') + // .value_parser(clap::value_parser!(u64)), arg!(-x --exact "match exactly with the command name"), - arg!(-F --pidfile "read PIDs from file"), - arg!(-L --logpidfile "fail if PID file is not locked"), + // arg!(-F --pidfile "read PIDs from file"), + // arg!(-L --logpidfile "fail if PID file is not locked"), arg!(-r --runstates "match runstates [D,S,Z,...]"), - arg!(-A --"ignore-ancestors" "exclude our ancestors from results"), - arg!(--cgroup "match by cgroup v2 names") - .value_delimiter(','), - arg!(--ns "match the processes that belong to the same namespace as "), - arg!(--nslist "list which namespaces will be considered for the --ns option.") - .value_delimiter(',') - .value_parser(["ipc", "mnt", "net", "pid", "user", "uts"]), + // arg!(-A --"ignore-ancestors" "exclude our ancestors from results"), + // arg!(--cgroup "match by cgroup v2 names") + // .value_delimiter(','), + // arg!(--ns "match the processes that belong to the same namespace as "), + // arg!(--nslist "list which namespaces will be considered for the --ns option.") + // .value_delimiter(',') + // .value_parser(["ipc", "mnt", "net", "pid", "user", "uts"]), ]) .arg( Arg::new("pattern") From 75c2bd5ffa0fd444c8e1caf7fbcebf6e03295bd8 Mon Sep 17 00:00:00 2001 From: Tuomas Tynkkynen Date: Sun, 9 Feb 2025 22:22:10 +0200 Subject: [PATCH 2/5] pgrep: Small cleanup --- src/uu/pgrep/src/pgrep.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/uu/pgrep/src/pgrep.rs b/src/uu/pgrep/src/pgrep.rs index eeb016b3..ee3cce60 100644 --- a/src/uu/pgrep/src/pgrep.rs +++ b/src/uu/pgrep/src/pgrep.rs @@ -170,7 +170,7 @@ fn collect_matched_pids(settings: &Settings) -> Vec { let mut tmp_vec = Vec::new(); for mut pid in walk_process().collect::>() { - let run_state_matched = match (&settings.runstates, (pid).run_state()) { + let run_state_matched = match (&settings.runstates, pid.run_state()) { (Some(arg_run_states), Ok(pid_state)) => { arg_run_states.contains(&pid_state.to_string()) } From cfe4cb337eea39f36b903cac28c04e3bb4002bbf Mon Sep 17 00:00:00 2001 From: Tuomas Tynkkynen Date: Sun, 9 Feb 2025 22:22:47 +0200 Subject: [PATCH 3/5] pkill: Support --inverse pkill --help doesn't list this option, but it is listed in the manpage (and does work). Unlike pgrep, the short option '-v' doesn't exists for pkill though. --- src/uu/pkill/src/pkill.rs | 10 +++++++--- tests/by-util/test_pkill.rs | 11 +++++++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/uu/pkill/src/pkill.rs b/src/uu/pkill/src/pkill.rs index b534af41..e3ee1447 100644 --- a/src/uu/pkill/src/pkill.rs +++ b/src/uu/pkill/src/pkill.rs @@ -36,6 +36,7 @@ struct Settings { exact: bool, full: bool, ignore_case: bool, + inverse: bool, newest: bool, oldest: bool, older: Option, @@ -64,6 +65,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { exact: matches.get_flag("exact"), full: matches.get_flag("full"), ignore_case: matches.get_flag("ignore-case"), + inverse: matches.get_flag("inverse"), newest: matches.get_flag("newest"), oldest: matches.get_flag("oldest"), parent: matches @@ -235,11 +237,12 @@ fn collect_matched_pids(settings: &Settings) -> Vec { _ => true, }; - if run_state_matched + if (run_state_matched && pattern_matched && tty_matched && older_matched - && parent_matched + && parent_matched) + ^ settings.inverse { tmp_vec.push(pid); } @@ -343,12 +346,13 @@ pub fn uu_app() -> Command { .about(ABOUT) .override_usage(format_usage(USAGE)) .args_override_self(true) - .group(ArgGroup::new("oldest_newest").args(["oldest", "newest"])) + .group(ArgGroup::new("oldest_newest").args(["oldest", "newest", "inverse"])) .args([ // arg!(- "signal to send (either number or name)"), arg!(-H --"require-handler" "match only if signal handler is present"), // arg!(-q --queue "integer value to be sent with the signal"), arg!(-e --echo "display what is killed"), + arg!(--inverse "negates the matching"), arg!(-c --count "count of matching processes"), arg!(-f --full "use full process name to match"), // arg!(-g --pgroup "match listed process group IDs") diff --git a/tests/by-util/test_pkill.rs b/tests/by-util/test_pkill.rs index 84935028..801e617d 100644 --- a/tests/by-util/test_pkill.rs +++ b/tests/by-util/test_pkill.rs @@ -44,6 +44,17 @@ fn test_invalid_arg() { new_ucmd!().arg("--definitely-invalid").fails().code_is(1); } +#[cfg(target_os = "linux")] +#[test] +fn test_inverse() { + new_ucmd!() + .arg("-0") + .arg("--inverse") + .arg("NONEXISTENT") + .fails() + .stderr_contains("Permission denied"); +} + #[cfg(unix)] #[test] fn test_help() { From f0148e2e1b1f69fc50bfce6d01e79b0f914aea7f Mon Sep 17 00:00:00 2001 From: Tuomas Tynkkynen Date: Mon, 10 Feb 2025 21:45:27 +0200 Subject: [PATCH 4/5] pkill: Simplify short signal number option handling Translate short signal options like '-TERM' to '--signal=TERM' to simplify the code and also properly handle the case where both short and long form is used at the same time. Change parsing of the SigCgt field to use u64, as there are more than 32 signals. --- src/uu/pkill/src/pkill.rs | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/src/uu/pkill/src/pkill.rs b/src/uu/pkill/src/pkill.rs index e3ee1447..fcd7c1d4 100644 --- a/src/uu/pkill/src/pkill.rs +++ b/src/uu/pkill/src/pkill.rs @@ -52,7 +52,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { #[cfg(target_os = "windows")] let args = args.collect_ignore(); #[cfg(unix)] - let obs_signal = handle_obsolete(&mut args); + handle_obsolete(&mut args); let matches = uu_app().try_get_matches_from(&args)?; @@ -96,13 +96,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { // Parse signal #[cfg(unix)] - let sig_num = if let Some(signal) = obs_signal { - signal - } else if let Some(signal) = matches.get_one::("signal") { - parse_signal_value(signal)? - } else { - 15_usize //SIGTERM - }; + let sig_num = parse_signal_value(matches.get_one::("signal").unwrap())?; #[cfg(unix)] let sig_name = signal_name_by_value(sig_num); @@ -125,7 +119,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { if matches.get_flag("require-handler") { pids.retain(|pid| { let mask = - u32::from_str_radix(pid.clone().status().get("SigCgt").unwrap(), 16).unwrap(); + u64::from_str_radix(pid.clone().status().get("SigCgt").unwrap(), 16).unwrap(); mask & (1 << sig_num) != 0 }); } @@ -293,7 +287,7 @@ fn process_flag_o_n( } #[cfg(unix)] -fn handle_obsolete(args: &mut Vec) -> Option { +fn handle_obsolete(args: &mut [String]) { // Sanity check if args.len() > 2 { // Old signal can only be in the first argument position @@ -302,13 +296,11 @@ fn handle_obsolete(args: &mut Vec) -> Option { // 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; + // Replace with long option that clap can parse + args[1] = format!("--signal={}", signal); } } } - None } #[cfg(unix)] @@ -372,7 +364,8 @@ pub fn uu_app() -> Command { // arg!(-s --session "match session IDs") // .value_delimiter(',') // .value_parser(clap::value_parser!(u64)), - arg!(--signal "signal to send (either number or name)"), + arg!(--signal "signal to send (either number or name)") + .default_value("SIGTERM"), arg!(-t --terminal "match by controlling terminal").value_delimiter(','), // arg!(-u --euid "match by effective IDs") // .value_delimiter(',') From f946eb6450555535843923093cf2c70c92929eaa Mon Sep 17 00:00:00 2001 From: Tuomas Tynkkynen Date: Mon, 10 Feb 2025 13:10:04 +0200 Subject: [PATCH 5/5] pgrep: Support --signal and --require-handler This is not listed in pgrep --help, but is documented in the man page and does work. Unlike pkill the -SIGNAL short option is not supported by pgrep. Simplify parse_signal_value with ok_or_else --- src/uu/pgrep/src/pgrep.rs | 24 +++++++++++++++++++++++- src/uu/pkill/src/pkill.rs | 10 ++-------- tests/by-util/test_pgrep.rs | 21 +++++++++++++++++++++ 3 files changed, 46 insertions(+), 9 deletions(-) diff --git a/src/uu/pgrep/src/pgrep.rs b/src/uu/pgrep/src/pgrep.rs index ee3cce60..802ec507 100644 --- a/src/uu/pgrep/src/pgrep.rs +++ b/src/uu/pgrep/src/pgrep.rs @@ -10,6 +10,8 @@ use clap::{arg, crate_version, Arg, ArgAction, ArgGroup, ArgMatches, Command}; use process::{walk_process, ProcessInformation, Teletype}; use regex::Regex; use std::{collections::HashSet, sync::OnceLock}; +#[cfg(unix)] +use uucore::{display::Quotable, signals::signal_by_name_or_value}; use uucore::{ error::{UResult, USimpleError}, format_usage, help_about, help_usage, @@ -87,9 +89,21 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { )); } + // Parse signal + #[cfg(unix)] + let sig_num = parse_signal_value(matches.get_one::("signal").unwrap())?; + // Collect pids let pids = { let mut pids = collect_matched_pids(&settings); + #[cfg(unix)] + if matches.get_flag("require-handler") { + pids.retain(|pid| { + let mask = + u64::from_str_radix(pid.clone().status().get("SigCgt").unwrap(), 16).unwrap(); + mask & (1 << sig_num) != 0 + }); + } if pids.is_empty() { uucore::error::set_exit_code(1); pids @@ -275,6 +289,12 @@ fn process_flag_o_n( } } +#[cfg(unix)] +fn parse_signal_value(signal_name: &str) -> UResult { + signal_by_name_or_value(signal_name) + .ok_or_else(|| USimpleError::new(1, format!("Unknown signal {}", signal_name.quote()))) +} + #[allow(clippy::cognitive_complexity)] pub fn uu_app() -> Command { Command::new(uucore::util_name()) @@ -289,6 +309,7 @@ pub fn uu_app() -> Command { .hide_default_value(true), arg!(-l --"list-name" "list PID and process name"), arg!(-a --"list-full" "list PID and full command line"), + arg!(-H --"require-handler" "match only if signal handler is present"), arg!(-v --inverse "negates the matching"), // arg!(-w --lightweight "list all TID"), arg!(-c --count "count of matching processes"), @@ -310,7 +331,8 @@ pub fn uu_app() -> Command { // arg!(-s --session "match session IDs") // .value_delimiter(',') // .value_parser(clap::value_parser!(u64)), - // arg!(--signal "signal to send (either number or name)"), + arg!(--signal "signal to send (either number or name)") + .default_value("SIGTERM"), arg!(-t --terminal "match by controlling terminal") .value_delimiter(','), // arg!(-u --euid ... "match by effective IDs") diff --git a/src/uu/pkill/src/pkill.rs b/src/uu/pkill/src/pkill.rs index fcd7c1d4..808b3e6d 100644 --- a/src/uu/pkill/src/pkill.rs +++ b/src/uu/pkill/src/pkill.rs @@ -305,14 +305,8 @@ fn handle_obsolete(args: &mut [String]) { #[cfg(unix)] 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 {}", signal_name.quote()), - )), - } + signal_by_name_or_value(signal_name) + .ok_or_else(|| USimpleError::new(1, format!("Unknown signal {}", signal_name.quote()))) } #[cfg(unix)] diff --git a/tests/by-util/test_pgrep.rs b/tests/by-util/test_pgrep.rs index 9c21dd57..19d73f0b 100644 --- a/tests/by-util/test_pgrep.rs +++ b/tests/by-util/test_pgrep.rs @@ -347,3 +347,24 @@ fn test_parent_non_matching_parent() { .code_is(1) .no_output(); } + +#[test] +#[cfg(target_os = "linux")] +fn test_require_handler() { + new_ucmd!() + .arg("--require-handler") + .arg("--signal=INT") + .arg("NONEXISTENT") + .fails() + .no_output(); +} + +#[test] +#[cfg(target_os = "linux")] +fn test_invalid_signal() { + new_ucmd!() + .arg("--signal=foo") + .arg("NONEXISTENT") + .fails() + .stderr_contains("Unknown signal 'foo'"); +}