From cccf89a48cbac9cda450caf1767881aaa8099f37 Mon Sep 17 00:00:00 2001 From: Ricardo Iglesias Date: Tue, 6 Apr 2021 21:52:56 -0700 Subject: [PATCH 1/4] timeout: Moved argument parsing to clap Changed from optparse to clap. None of the logic within timeout has been changed, which could use some refactoring, but that's beyond the scope of this commit. --- src/uu/timeout/Cargo.toml | 2 + src/uu/timeout/src/timeout.rs | 201 +++++++++++++++++++++------------- 2 files changed, 129 insertions(+), 74 deletions(-) diff --git a/src/uu/timeout/Cargo.toml b/src/uu/timeout/Cargo.toml index 51ac0bc0e..206a98c08 100644 --- a/src/uu/timeout/Cargo.toml +++ b/src/uu/timeout/Cargo.toml @@ -15,11 +15,13 @@ edition = "2018" path = "src/timeout.rs" [dependencies] +clap = "2.33" getopts = "0.2.18" libc = "0.2.42" uucore = { version=">=0.0.8", package="uucore", path="../../uucore", features=["parse_time", "process", "signals"] } uucore_procs = { version=">=0.0.5", package="uucore_procs", path="../../uucore_procs" } + [[bin]] name = "timeout" path = "src/main.rs" diff --git a/src/uu/timeout/src/timeout.rs b/src/uu/timeout/src/timeout.rs index 0dd7c2016..3efd04c86 100644 --- a/src/uu/timeout/src/timeout.rs +++ b/src/uu/timeout/src/timeout.rs @@ -10,99 +10,152 @@ #[macro_use] extern crate uucore; +extern crate clap; + +use clap::{App, Arg, ArgMatches, AppSettings}; use std::io::ErrorKind; use std::process::{Command, Stdio}; use std::time::Duration; use uucore::process::ChildExt; +use uucore::signals::{Signal, signal_by_name_or_value}; + static NAME: &str = "timeout"; static VERSION: &str = env!("CARGO_PKG_VERSION"); const ERR_EXIT_STATUS: i32 = 125; +pub mod options { + pub static FOREGROUND: &str = "foreground"; + pub static KILL_AFTER: &str = "kill-after"; + pub static SIGNAL: &str = "signal"; + pub static VERSION: &str = "version"; + pub static PRESERVE_STATUS: &str = "preserve-status"; + + // Positional args. + pub static DURATION: &str = "duration"; + pub static COMMAND: &str = "command"; + pub static ARGS: &str = "args"; +} + +struct Config { + foreground: bool, + kill_after: Option, + signal: Option, + version: bool, + duration: Duration, + preserve_status: bool + + command: String, + command_args: &[String] +} + +impl Config { + fn from(options: Clap::ArgMatches) -> Config { + let timeout_signal = match options.value_of(options::SIGNAL) { + Some(signal_) => + { + let signal_result = signal_by_name_or_value(&signal_); + match signal_result{ + None => { + show_error!("invalid signal '{}'", signal_); + return ERR_EXIT_STATUS; + }, + _ => Some(signal_result) + } + }, + _ => None + }; + + let kill_after: Option = + match options.value_of(options::KILL_AFTER) { + Some(time) => Some(uucore::parse_time::from_str(&time)), + None => None + }; + + let duration: Duration = uucore::parse_time::from_str( + options.value_of(options::DURATION) + ); + + let preserve_status: bool = options.is_present(options::PRESERVE_STATUS); + + let command: String = options.value_of(options::COMMAND).to_str(); + let command_args: &[String] = options.values_of(options::ARGS) + .map(|x| x.as_str()); + + Config { + foreground: options.is_present(options::FOREGROUND), + kill_after, + signal: timeout_signal, + duration, + preserve_status, + command, + command_args + } + } +} + pub fn uumain(args: impl uucore::Args) -> i32 { let args = args.collect_str(); let program = args[0].clone(); let mut opts = getopts::Options::new(); - opts.optflag( - "", - "preserve-status", - "exit with the same status as COMMAND, even when the command times out", - ); - opts.optflag("", "foreground", "when not running timeout directly from a shell prompt, allow COMMAND to read from the TTY and get TTY signals; in this mode, children of COMMAND will not be timed out"); - opts.optopt("k", "kill-after", "also send a KILL signal if COMMAND is still running this long after the initial signal was sent", "DURATION"); - opts.optflag("s", "signal", "specify the signal to be sent on timeout; SIGNAL may be a name like 'HUP' or a number; see 'kill -l' for a list of signals"); - opts.optflag("h", "help", "display this help and exit"); - opts.optflag("V", "version", "output version information and exit"); - let matches = match opts.parse(&args[1..]) { - Ok(m) => m, - Err(f) => crash!(ERR_EXIT_STATUS, "{}", f), - }; - if matches.opt_present("help") { - print!( - "{} {} -Usage: - {} [OPTION] DURATION COMMAND [ARG]... + let mut app = App::new("timeout") + .version(VERSION) + .arg( + Arg::with_name(options::FOREGROUND) + .long(options::FOREGROUND) + .help("when not running timeout directly from a shell prompt, allow COMMAND to read from the TTY and get TTY signals; in this mode, children of COMMAND will not be timed out") + ) + .arg( + Arg::with_name(options::KILL_AFTER) + .short("k") + .takes_value(true)) + .arg( + Arg::with_name(options::PRESERVE_STATUS) + .long(options::PRESERVE_STATUS) + .help("exit with the same status as COMMAND, even when the command times out") + ) + .arg( + Arg::with_name(options::SIGNAL) + .short("s") + .long(options::SIGNAL) + .help("specify the signal to be sent on timeout; SIGNAL may be a name like 'HUP' or a number; see 'kill -l' for a list of signals") + .takes_value(true) + ) + .arg( + Arg::with_name(options::DURATION) + .index(1) + .required(true) + ) + .arg( + Arg::with_name(options::COMMAND) + .index(2) + .required(true) + ) + .arg( + Arg::with_name(options::ARGS).required(true).multiple(true) + ) + .setting(AppSettings::TrailingVarArg); -{}", - NAME, - VERSION, - program, - &opts.usage("Start COMMAND, and kill it if still running after DURATION.") - ); - } else if matches.opt_present("version") { - println!("{} {}", NAME, VERSION); - } else if matches.free.len() < 2 { - show_error!("missing an argument"); - show_error!("for help, try '{0} --help'", program); - return ERR_EXIT_STATUS; - } else { - let status = matches.opt_present("preserve-status"); - let foreground = matches.opt_present("foreground"); - let kill_after = match matches.opt_str("kill-after") { - Some(tstr) => match uucore::parse_time::from_str(&tstr) { - Ok(time) => time, - Err(f) => { - show_error!("{}", f); - return ERR_EXIT_STATUS; - } - }, - None => Duration::new(0, 0), - }; - let signal = match matches.opt_str("signal") { - Some(sigstr) => match uucore::signals::signal_by_name_or_value(&sigstr) { - Some(sig) => sig, - None => { - show_error!("invalid signal '{}'", sigstr); - return ERR_EXIT_STATUS; - } - }, - None => uucore::signals::signal_by_name_or_value("TERM").unwrap(), - }; - let duration = match uucore::parse_time::from_str(&matches.free[0]) { - Ok(time) => time, - Err(f) => { - show_error!("{}", f); - return ERR_EXIT_STATUS; - } - }; - return timeout( - &matches.free[1], - &matches.free[2..], - duration, - signal, - kill_after, - foreground, - status, - ); - } + let matches = app.get_matches_from(args); - 0 + let config = Config::from(matches); + timeout(config.command, + config.command_args, + config.duration, + config.signal, + config.kill_after, + config.foreground, + config.preserve_status + ) } +/// TODO: Improve exit codes, and make them consistent with the GNU Coreutil +/// exit codes. + fn timeout( cmdname: &str, args: &[String], @@ -126,10 +179,10 @@ fn timeout( Err(err) => { show_error!("failed to execute process: {}", err); if err.kind() == ErrorKind::NotFound { - // XXX: not sure which to use + // FIXME: not sure which to use return 127; } else { - // XXX: this may not be 100% correct... + // FIXME: this may not be 100% correct... return 126; } } From ea0ead6a2e8da6f76cb509c844467c956ca8a092 Mon Sep 17 00:00:00 2001 From: Ricardo Iglesias Date: Tue, 6 Apr 2021 22:16:52 -0700 Subject: [PATCH 2/4] Ran cargo lock update command. --- Cargo.lock | 1 + 1 file changed, 1 insertion(+) diff --git a/Cargo.lock b/Cargo.lock index f65ba3dff..d8aea5e08 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2416,6 +2416,7 @@ dependencies = [ name = "uu_timeout" version = "0.0.6" dependencies = [ + "clap", "getopts", "libc", "uucore", From 431a6ee1b5af6d901d41c29d10a56e0491ba91f7 Mon Sep 17 00:00:00 2001 From: Ricardo Iglesias Date: Tue, 6 Apr 2021 23:07:52 -0700 Subject: [PATCH 3/4] timeout: Fixed ownership issues Fixed some minor ownership issues in converting from the options to the arguments to the timeout COMMAND. Additionally, fixed a rustfmt issue in other files (fold/stdbuf.rs) --- src/uu/fold/src/fold.rs | 2 +- src/uu/stdbuf/src/stdbuf.rs | 3 +- src/uu/timeout/src/timeout.rs | 94 ++++++++++++++++++----------------- 3 files changed, 51 insertions(+), 48 deletions(-) diff --git a/src/uu/fold/src/fold.rs b/src/uu/fold/src/fold.rs index cfd3b74cf..c35e996f2 100644 --- a/src/uu/fold/src/fold.rs +++ b/src/uu/fold/src/fold.rs @@ -45,7 +45,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { .short("b") .help( "count using bytes rather than columns (meaning control characters \ - such as newline are not treated specially)", + such as newline are not treated specially)", ) .takes_value(false), ) diff --git a/src/uu/stdbuf/src/stdbuf.rs b/src/uu/stdbuf/src/stdbuf.rs index 67ed9a838..a61ba967b 100644 --- a/src/uu/stdbuf/src/stdbuf.rs +++ b/src/uu/stdbuf/src/stdbuf.rs @@ -80,7 +80,8 @@ fn print_version() { fn print_usage(opts: &Options) { let brief = "Run COMMAND, with modified buffering operations for its standard streams\n \ Mandatory arguments to long options are mandatory for short options too."; - let explanation = "If MODE is 'L' the corresponding stream will be line buffered.\n \ + let explanation = + "If MODE is 'L' the corresponding stream will be line buffered.\n \ This option is invalid with standard input.\n\n \ If MODE is '0' the corresponding stream will be unbuffered.\n\n \ Otherwise MODE is a number which may be followed by one of the following:\n\n \ diff --git a/src/uu/timeout/src/timeout.rs b/src/uu/timeout/src/timeout.rs index 3efd04c86..bd340a122 100644 --- a/src/uu/timeout/src/timeout.rs +++ b/src/uu/timeout/src/timeout.rs @@ -12,16 +12,19 @@ extern crate uucore; extern crate clap; -use clap::{App, Arg, ArgMatches, AppSettings}; +use clap::{App, AppSettings, Arg}; use std::io::ErrorKind; use std::process::{Command, Stdio}; use std::time::Duration; use uucore::process::ChildExt; -use uucore::signals::{Signal, signal_by_name_or_value}; +use uucore::signals::signal_by_name_or_value; - -static NAME: &str = "timeout"; static VERSION: &str = env!("CARGO_PKG_VERSION"); +static ABOUT: &str = "Start COMMAND, and kill it if still running after DURATION."; + +fn get_usage() -> String { + format!("{0} [OPTION]... [FILE]...", executable!()) +} const ERR_EXIT_STATUS: i32 = 125; @@ -40,70 +43,68 @@ pub mod options { struct Config { foreground: bool, - kill_after: Option, - signal: Option, - version: bool, + kill_after: Duration, + signal: usize, duration: Duration, - preserve_status: bool + preserve_status: bool, command: String, - command_args: &[String] + command_args: Vec, } impl Config { - fn from(options: Clap::ArgMatches) -> Config { - let timeout_signal = match options.value_of(options::SIGNAL) { - Some(signal_) => - { + fn from(options: clap::ArgMatches) -> Config { + let signal = match options.value_of(options::SIGNAL) { + Some(signal_) => { let signal_result = signal_by_name_or_value(&signal_); - match signal_result{ + match signal_result { None => { - show_error!("invalid signal '{}'", signal_); - return ERR_EXIT_STATUS; - }, - _ => Some(signal_result) + unreachable!("invalid signal '{}'", signal_); + } + Some(signal_value) => signal_value, } - }, - _ => None + } + _ => uucore::signals::signal_by_name_or_value("TERM").unwrap(), }; - let kill_after: Option = - match options.value_of(options::KILL_AFTER) { - Some(time) => Some(uucore::parse_time::from_str(&time)), - None => None - }; + let kill_after: Duration = match options.value_of(options::KILL_AFTER) { + Some(time) => uucore::parse_time::from_str(&time).unwrap(), + None => Duration::new(0, 0), + }; - let duration: Duration = uucore::parse_time::from_str( - options.value_of(options::DURATION) - ); + let duration: Duration = + uucore::parse_time::from_str(options.value_of(options::DURATION).unwrap()).unwrap(); let preserve_status: bool = options.is_present(options::PRESERVE_STATUS); + let foreground = options.is_present(options::FOREGROUND); - let command: String = options.value_of(options::COMMAND).to_str(); - let command_args: &[String] = options.values_of(options::ARGS) - .map(|x| x.as_str()); + let command: String = options.value_of(options::COMMAND).unwrap().to_string(); + let command_args: Vec = options + .values_of(options::ARGS) + .unwrap() + .map(|x| x.to_owned()) + .collect(); Config { - foreground: options.is_present(options::FOREGROUND), + foreground, kill_after, - signal: timeout_signal, + signal, duration, preserve_status, command, - command_args + command_args, } } } pub fn uumain(args: impl uucore::Args) -> i32 { let args = args.collect_str(); + let usage = get_usage(); - let program = args[0].clone(); - - let mut opts = getopts::Options::new(); - - let mut app = App::new("timeout") + let app = App::new("timeout") .version(VERSION) + .usage(&usage[..]) + .about(ABOUT) .arg( Arg::with_name(options::FOREGROUND) .long(options::FOREGROUND) @@ -143,13 +144,14 @@ pub fn uumain(args: impl uucore::Args) -> i32 { let matches = app.get_matches_from(args); let config = Config::from(matches); - timeout(config.command, - config.command_args, - config.duration, - config.signal, - config.kill_after, - config.foreground, - config.preserve_status + timeout( + &config.command, + &config.command_args, + config.duration, + config.signal, + config.kill_after, + config.foreground, + config.preserve_status, ) } From 8232c527a365050d91a6ee9d8931c0455d417474 Mon Sep 17 00:00:00 2001 From: Ricardo Iglesias Date: Tue, 6 Apr 2021 23:30:15 -0700 Subject: [PATCH 4/4] timeout: tests passing. Forgot to handle the case where no arguments were passed to the COMMAND. Because ARGS can be empty, we need two separate cases for handling options.values_of(options::ARGS) --- src/uu/timeout/src/timeout.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/uu/timeout/src/timeout.rs b/src/uu/timeout/src/timeout.rs index bd340a122..23e2ec842 100644 --- a/src/uu/timeout/src/timeout.rs +++ b/src/uu/timeout/src/timeout.rs @@ -79,11 +79,11 @@ impl Config { let foreground = options.is_present(options::FOREGROUND); let command: String = options.value_of(options::COMMAND).unwrap().to_string(); - let command_args: Vec = options - .values_of(options::ARGS) - .unwrap() - .map(|x| x.to_owned()) - .collect(); + + let command_args: Vec = match options.values_of(options::ARGS) { + Some(values) => values.map(|x| x.to_owned()).collect(), + None => vec![], + }; Config { foreground, @@ -137,7 +137,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 { .required(true) ) .arg( - Arg::with_name(options::ARGS).required(true).multiple(true) + Arg::with_name(options::ARGS).multiple(true) ) .setting(AppSettings::TrailingVarArg);