Merge pull request #2155 from nthery/kill_clap

kill: migrate to clap

Cargo.lock

@@ -2043,6 +2043,7 @@ dependencies = [
 name = "uu_kill"
 version = "0.0.6"
 dependencies = [
+ "clap",
  "libc",
  "uucore",
  "uucore_procs",

src/uu/kill/Cargo.toml

@@ -15,6 +15,7 @@ edition = "2018"
 path = "src/kill.rs"
 
 [dependencies]
+clap = "2.33"
 libc = "0.2.42"
 uucore = { version=">=0.0.8", package="uucore", path="../../uucore", features=["signals"] }
 uucore_procs = { version=">=0.0.5", package="uucore_procs", path="../../uucore_procs" }

src/uu/kill/src/kill.rs

@@ -10,18 +10,26 @@
 #[macro_use]
 extern crate uucore;
 
+use clap::{App, Arg};
 use libc::{c_int, pid_t};
 use std::io::Error;
 use uucore::signals::ALL_SIGNALS;
 use uucore::InvalidEncodingHandling;
 
-static SYNTAX: &str = "[options] <pid> [...]";
-static SUMMARY: &str = "";
-static LONG_HELP: &str = "";
+static VERSION: &str = env!("CARGO_PKG_VERSION");
+static ABOUT: &str = "Send signal to processes or list information about signals.";
 
 static EXIT_OK: i32 = 0;
 static EXIT_ERR: i32 = 1;
 
+pub mod options {
+    pub static PIDS_OR_SIGNALS: &str = "pids_of_signals";
+    pub static LIST: &str = "list";
+    pub static TABLE: &str = "table";
+    pub static TABLE_OLD: &str = "table_old";
+    pub static SIGNAL: &str = "signal";
+}
+
 #[derive(Clone, Copy)]
 pub enum Mode {
     Kill,
@@ -33,41 +41,70 @@ pub fn uumain(args: impl uucore::Args) -> i32 {
     let args = args
         .collect_str(InvalidEncodingHandling::Ignore)
         .accept_any();
-
     let (args, obs_signal) = handle_obsolete(args);
-    let matches = app!(SYNTAX, SUMMARY, LONG_HELP)
-        .optopt("s", "signal", "specify the <signal> to be sent", "SIGNAL")
-        .optflagopt(
-            "l",
-            "list",
-            "list all signal names, or convert one to a name",
-            "LIST",
-        )
-        .optflag("L", "table", "list all signal names in a nice table")
-        .parse(args);
 
-    let mode = if matches.opt_present("table") {
+    let usage = format!("{} [OPTIONS]... PID...", executable!());
+    let matches = App::new(executable!())
+        .version(VERSION)
+        .about(ABOUT)
+        .usage(&usage[..])
+        .arg(
+            Arg::with_name(options::LIST)
+                .short("l")
+                .long(options::LIST)
+                .help("Lists signals")
+                .conflicts_with(options::TABLE)
+                .conflicts_with(options::TABLE_OLD),
+        )
+        .arg(
+            Arg::with_name(options::TABLE)
+                .short("t")
+                .long(options::TABLE)
+                .help("Lists table of signals"),
+        )
+        .arg(Arg::with_name(options::TABLE_OLD).short("L").hidden(true))
+        .arg(
+            Arg::with_name(options::SIGNAL)
+                .short("s")
+                .long(options::SIGNAL)
+                .help("Sends given signal")
+                .takes_value(true),
+        )
+        .arg(
+            Arg::with_name(options::PIDS_OR_SIGNALS)
+                .hidden(true)
+                .multiple(true),
+        )
+        .get_matches_from(args);
+
+    let mode = if matches.is_present(options::TABLE) || matches.is_present(options::TABLE_OLD) {
         Mode::Table
-    } else if matches.opt_present("list") {
+    } else if matches.is_present(options::LIST) {
         Mode::List
     } else {
         Mode::Kill
     };
 
+    let pids_or_signals: Vec<String> = matches
+        .values_of(options::PIDS_OR_SIGNALS)
+        .map(|v| v.map(ToString::to_string).collect())
+        .unwrap_or_default();
+
     match mode {
         Mode::Kill => {
-            return kill(
-                &matches
-                    .opt_str("signal")
-                    .unwrap_or_else(|| obs_signal.unwrap_or_else(|| "9".to_owned())),
-                matches.free,
-            )
+            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(),
+            };
+            return kill(&sig, &pids_or_signals);
         }
         Mode::Table => table(),
-        Mode::List => list(matches.opt_str("list")),
+        Mode::List => list(pids_or_signals.get(0).cloned()),
     }
 
-    0
+    EXIT_OK
 }
 
 fn handle_obsolete(mut args: Vec<String>) -> (Vec<String>, Option<String>) {
@@ -148,14 +185,14 @@ fn list(arg: Option<String>) {
     };
 }
 
-fn kill(signalname: &str, pids: std::vec::Vec<String>) -> i32 {
+fn kill(signalname: &str, pids: &[String]) -> i32 {
     let mut status = 0;
     let optional_signal_value = uucore::signals::signal_by_name_or_value(signalname);
     let signal_value = match optional_signal_value {
         Some(x) => x,
         None => crash!(EXIT_ERR, "unknown signal name {}", signalname),
     };
-    for pid in &pids {
+    for pid in pids {
         match pid.parse::<usize>() {
             Ok(x) => {
                 if unsafe { libc::kill(x as pid_t, signal_value as c_int) } != 0 {

tests/by-util/test_kill.rs

@@ -1 +1,126 @@
-// ToDO: add tests
+use crate::common::util::*;
+use regex::Regex;
+use std::os::unix::process::ExitStatusExt;
+use std::process::{Child, Command};
+
+// A child process the tests will try to kill.
+struct Target {
+    child: Child,
+    killed: bool,
+}
+
+impl Target {
+    // Creates a target that will naturally die after some time if not killed
+    // fast enough.
+    // This timeout avoids hanging failing tests.
+    fn new() -> Target {
+        Target {
+            child: Command::new("sleep")
+                .arg("30")
+                .spawn()
+                .expect("cannot spawn target"),
+            killed: false,
+        }
+    }
+
+    // Waits for the target to complete and returns the signal it received if any.
+    fn wait_for_signal(&mut self) -> Option<i32> {
+        let sig = self.child.wait().expect("cannot wait on target").signal();
+        self.killed = true;
+        sig
+    }
+
+    fn pid(&self) -> u32 {
+        self.child.id()
+    }
+}
+
+impl Drop for Target {
+    // Terminates this target to avoid littering test boxes with zombi processes
+    // when a test fails after creating a target but before killing it.
+    fn drop(&mut self) {
+        if !self.killed {
+            self.child.kill().expect("cannot kill target");
+        }
+    }
+}
+
+#[test]
+fn test_kill_list_all_signals() {
+    // Check for a few signals.  Do not try to be comprehensive.
+    new_ucmd!()
+        .arg("-l")
+        .succeeds()
+        .stdout_contains("KILL")
+        .stdout_contains("TERM")
+        .stdout_contains("HUP");
+}
+
+#[test]
+fn test_kill_list_all_signals_as_table() {
+    // Check for a few signals.  Do not try to be comprehensive.
+    new_ucmd!()
+        .arg("-t")
+        .succeeds()
+        .stdout_contains("KILL")
+        .stdout_contains("TERM")
+        .stdout_contains("HUP");
+}
+
+#[test]
+fn test_kill_list_one_signal_from_name() {
+    // Use SIGKILL because it is 9 on all unixes.
+    new_ucmd!()
+        .arg("-l")
+        .arg("KILL")
+        .succeeds()
+        .stdout_matches(&Regex::new("\\b9\\b").unwrap());
+}
+
+#[test]
+fn test_kill_set_bad_signal_name() {
+    new_ucmd!()
+        .arg("-s")
+        .arg("IAMNOTASIGNAL")
+        .fails()
+        .stderr_contains("unknown signal");
+}
+
+#[test]
+fn test_kill_with_default_signal() {
+    let mut target = Target::new();
+    new_ucmd!().arg(format!("{}", target.pid())).succeeds();
+    assert_eq!(target.wait_for_signal(), Some(libc::SIGTERM));
+}
+
+#[test]
+fn test_kill_with_signal_number_old_form() {
+    let mut target = Target::new();
+    new_ucmd!()
+        .arg("-9")
+        .arg(format!("{}", target.pid()))
+        .succeeds();
+    assert_eq!(target.wait_for_signal(), Some(9));
+}
+
+#[test]
+fn test_kill_with_signal_number_new_form() {
+    let mut target = Target::new();
+    new_ucmd!()
+        .arg("-s")
+        .arg("9")
+        .arg(format!("{}", target.pid()))
+        .succeeds();
+    assert_eq!(target.wait_for_signal(), Some(9));
+}
+
+#[test]
+fn test_kill_with_signal_name_new_form() {
+    let mut target = Target::new();
+    new_ucmd!()
+        .arg("-s")
+        .arg("KILL")
+        .arg(format!("{}", target.pid()))
+        .succeeds();
+    assert_eq!(target.wait_for_signal(), Some(libc::SIGKILL));
+}