From 827abca86cd3754554a81f59f92a9c5a8b5d2faa Mon Sep 17 00:00:00 2001 From: Igor Date: Tue, 22 Oct 2024 17:44:16 +0400 Subject: [PATCH] share code with `trap` builtin --- brush-core/src/builtins/kill.rs | 80 +++++--------- brush-core/src/builtins/trap.rs | 54 ++------- brush-core/src/completion.rs | 2 +- brush-core/src/error.rs | 4 +- brush-core/src/sys/stubs/signal.rs | 8 -- brush-core/src/sys/unix/signal.rs | 16 +-- brush-core/src/traps.rs | 123 ++++++++++++++++++--- brush-shell/tests/cases/builtins/kill.yaml | 7 +- 8 files changed, 159 insertions(+), 135 deletions(-) diff --git a/brush-core/src/builtins/kill.rs b/brush-core/src/builtins/kill.rs index 7a2514c5..ad4fa2df 100644 --- a/brush-core/src/builtins/kill.rs +++ b/brush-core/src/builtins/kill.rs @@ -1,8 +1,7 @@ use clap::Parser; -use nix::sys::signal::Signal; use std::io::Write; -use std::str::FromStr; +use crate::traps::{self, TrapSignal}; use crate::{builtins, commands, error}; /// Signal a job or process. @@ -77,65 +76,44 @@ fn print_signals( signals: &[String], ) -> Result { let mut exit_code = builtins::ExitCode::Success; - // TODO: `0 EXIT` signal is missing. It is not in the posix spec, but it exists in Bash - // https://man7.org/linux/man-pages/man7/signal.7.html - // https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/signal.h.html if !signals.is_empty() { for s in signals { // If the user gives us a code, we print the name; if they give a name, we print its // code. - enum Sigspec { - Sigspec(&'static str), - Signum(i32), + enum PrintSignal { + Name(&'static str), + Num(i32), } - let signal = s - .parse::() - .ok() - .and_then(|code| { - Signal::try_from(code) - .map(|s| { - // bash compatinility. `SIGHUP` -> `HUP` - Sigspec::Sigspec(s.as_str().strip_prefix("SIG").unwrap_or(s.as_str())) - }) - .ok() + + let signal = if let Ok(n) = s.parse::() { + // bash compatibility. `SIGHUP` -> `HUP` + TrapSignal::try_from(n).map(|s| { + PrintSignal::Name(s.as_str().strip_prefix("SIG").unwrap_or(s.as_str())) }) - .or_else(|| { - // bash compatibility: - // support for names without `SIG`, for example `HUP` -> `SIGHUP` - let mut sig_str = String::with_capacity(3 + s.len()); - if s.len() >= 3 && s[..3] != *"SIG" { - sig_str.push_str("SIG"); - sig_str.push_str(s.as_str()); - } else { - sig_str.push_str(s.as_str()); - } - Signal::from_str(sig_str.as_str()) - .ok() - .map(|s| Sigspec::Signum(s as i32)) - }); - if let Some(signal) = signal { - match signal { - Sigspec::Signum(n) => { - writeln!(context.stdout(), "{n}")?; - } - Sigspec::Sigspec(s) => { - writeln!(context.stdout(), "{s}")?; - } - } } else { - writeln!( - context.stderr(), - "{}: {}: invalid signal specification", - context.command_name, - s - )?; - exit_code = builtins::ExitCode::Custom(1); + TrapSignal::try_from(s.as_str()).map(|s| PrintSignal::Num(i32::from(s))) + }; + + match signal { + Ok(PrintSignal::Num(n)) => { + writeln!(context.stdout(), "{n}")?; + } + Ok(PrintSignal::Name(s)) => { + writeln!(context.stdout(), "{s}")?; + } + Err(e) => { + writeln!(context.stderr(), "{e}")?; + exit_code = builtins::ExitCode::Custom(1); + } } } } else { - for i in Signal::iterator() { - writeln!(context.stdout(), "{i}")?; - } + return traps::format_signals( + context.stdout(), + TrapSignal::iterator() + .filter(|s| !matches!(s, TrapSignal::Err | TrapSignal::Debug | TrapSignal::Exit)), + ) + .map(|()| builtins::ExitCode::Success); } Ok(exit_code) diff --git a/brush-core/src/builtins/trap.rs b/brush-core/src/builtins/trap.rs index 1a9cc71f..33cff639 100644 --- a/brush-core/src/builtins/trap.rs +++ b/brush-core/src/builtins/trap.rs @@ -1,7 +1,8 @@ use clap::Parser; use std::io::Write; -use crate::{builtins, commands, error, sys, traps}; +use crate::traps::TrapSignal; +use crate::{builtins, commands, error}; /// Manage signal traps. #[derive(Parser)] @@ -23,13 +24,12 @@ impl builtins::Command for TrapCommand { mut context: commands::ExecutionContext<'_>, ) -> Result { if self.list_signals { - Self::display_signals(&context)?; - Ok(builtins::ExitCode::Success) + crate::traps::format_signals(context.stdout(), TrapSignal::iterator()) + .map(|()| builtins::ExitCode::Success) } else if self.print_trap_commands || self.args.is_empty() { if !self.args.is_empty() { for signal_type in &self.args { - let signal_type = parse_signal(signal_type)?; - Self::display_handlers_for(&context, signal_type)?; + Self::display_handlers_for(&context, signal_type.parse()?)?; } } else { Self::display_all_handlers(&context)?; @@ -37,15 +37,14 @@ impl builtins::Command for TrapCommand { Ok(builtins::ExitCode::Success) } else if self.args.len() == 1 { let signal = self.args[0].as_str(); - let signal_type = parse_signal(signal)?; - Self::remove_all_handlers(&mut context, signal_type); + Self::remove_all_handlers(&mut context, signal.parse()?); Ok(builtins::ExitCode::Success) } else { let handler = &self.args[0]; let mut signal_types = vec![]; for signal in &self.args[1..] { - signal_types.push(parse_signal(signal)?); + signal_types.push(signal.parse()?); } Self::register_handler(&mut context, signal_types, handler.as_str()); @@ -56,16 +55,6 @@ impl builtins::Command for TrapCommand { #[allow(unused_variables)] impl TrapCommand { - #[allow(clippy::unnecessary_wraps)] - fn display_signals(context: &commands::ExecutionContext<'_>) -> Result<(), error::Error> { - #[cfg(unix)] - for signal in nix::sys::signal::Signal::iterator() { - writeln!(context.stdout(), "{}: {signal}", signal as i32)?; - } - - Ok(()) - } - fn display_all_handlers(context: &commands::ExecutionContext<'_>) -> Result<(), error::Error> { for signal in context.shell.traps.handlers.keys() { Self::display_handlers_for(context, *signal)?; @@ -75,7 +64,7 @@ impl TrapCommand { fn display_handlers_for( context: &commands::ExecutionContext<'_>, - signal_type: traps::TrapSignal, + signal_type: TrapSignal, ) -> Result<(), error::Error> { if let Some(handler) = context.shell.traps.handlers.get(&signal_type) { writeln!(context.stdout(), "trap -- '{handler}' {signal_type}")?; @@ -85,14 +74,14 @@ impl TrapCommand { fn remove_all_handlers( context: &mut crate::commands::ExecutionContext<'_>, - signal: traps::TrapSignal, + signal: TrapSignal, ) { context.shell.traps.remove_handlers(signal); } fn register_handler( context: &mut crate::commands::ExecutionContext<'_>, - signals: Vec, + signals: Vec, handler: &str, ) { for signal in signals { @@ -103,26 +92,3 @@ impl TrapCommand { } } } - -fn parse_signal(signal: &str) -> Result { - if signal.chars().all(|c| c.is_ascii_digit()) { - let digits = signal - .parse::() - .map_err(|_| error::Error::InvalidSignal)?; - - sys::signal::parse_numeric_signal(digits) - } else { - let mut signal_to_parse = signal.to_ascii_uppercase(); - - if !signal_to_parse.starts_with("SIG") { - signal_to_parse.insert_str(0, "SIG"); - } - - match signal_to_parse { - s if s == "SIGDEBUG" => Ok(traps::TrapSignal::Debug), - s if s == "SIGERR" => Ok(traps::TrapSignal::Err), - s if s == "SIGEXIT" => Ok(traps::TrapSignal::Exit), - s => sys::signal::parse_os_signal_name(s.as_str()), - } - } -} diff --git a/brush-core/src/completion.rs b/brush-core/src/completion.rs index 716ae3d1..46b2bb4f 100644 --- a/brush-core/src/completion.rs +++ b/brush-core/src/completion.rs @@ -464,7 +464,7 @@ impl Spec { } } CompleteAction::Signal => { - for signal in traps::TrapSignal::all_values() { + for signal in traps::TrapSignal::iterator() { candidates.insert(signal.to_string()); } } diff --git a/brush-core/src/error.rs b/brush-core/src/error.rs index d2e890dc..946e0821 100644 --- a/brush-core/src/error.rs +++ b/brush-core/src/error.rs @@ -148,8 +148,8 @@ pub enum Error { ThreadingError(#[from] tokio::task::JoinError), /// An invalid signal was referenced. - #[error("invalid signal")] - InvalidSignal, + #[error("{0}: invalid signal specification")] + InvalidSignal(String), /// A system error occurred. #[cfg(unix)] diff --git a/brush-core/src/sys/stubs/signal.rs b/brush-core/src/sys/stubs/signal.rs index 22c8932d..6b816cfe 100644 --- a/brush-core/src/sys/stubs/signal.rs +++ b/brush-core/src/sys/stubs/signal.rs @@ -1,13 +1,5 @@ use crate::{error, sys, traps}; -pub(crate) fn parse_numeric_signal(_signal: i32) -> Result { - Err(error::Error::InvalidSignal) -} - -pub(crate) fn parse_os_signal_name(_signal: &str) -> Result { - Err(error::Error::InvalidSignal) -} - pub(crate) fn continue_process(_pid: sys::process::ProcessId) -> Result<(), error::Error> { error::unimp("continue process") } diff --git a/brush-core/src/sys/unix/signal.rs b/brush-core/src/sys/unix/signal.rs index 281f2f50..67cb1a6d 100644 --- a/brush-core/src/sys/unix/signal.rs +++ b/brush-core/src/sys/unix/signal.rs @@ -1,18 +1,4 @@ -use std::str::FromStr; - -use crate::{error, sys, traps}; - -pub(crate) fn parse_numeric_signal(signal: i32) -> Result { - Ok(traps::TrapSignal::Signal( - nix::sys::signal::Signal::try_from(signal).map_err(|_| error::Error::InvalidSignal)?, - )) -} - -pub(crate) fn parse_os_signal_name(signal: &str) -> Result { - Ok(traps::TrapSignal::Signal( - nix::sys::signal::Signal::from_str(signal).map_err(|_| error::Error::InvalidSignal)?, - )) -} +use crate::{error, sys}; pub(crate) fn continue_process(pid: sys::process::ProcessId) -> Result<(), error::Error> { #[allow(clippy::cast_possible_wrap)] diff --git a/brush-core/src/traps.rs b/brush-core/src/traps.rs index f4004ca6..e0110a1a 100644 --- a/brush-core/src/traps.rs +++ b/brush-core/src/traps.rs @@ -1,5 +1,10 @@ +use std::str::FromStr; use std::{collections::HashMap, fmt::Display}; +use itertools::Itertools as _; + +use crate::error; + /// Type of signal that can be trapped in the shell. #[derive(Clone, Copy, Eq, Hash, PartialEq)] pub enum TrapSignal { @@ -16,28 +21,120 @@ pub enum TrapSignal { impl Display for TrapSignal { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.as_str()) + } +} + +impl TrapSignal { + /// Returns all possible values of [`TrapSignal`]. + pub fn iterator() -> impl Iterator { + const SIGNALS: &[TrapSignal] = &[TrapSignal::Debug, TrapSignal::Err, TrapSignal::Exit]; + let iter = SIGNALS.iter().copied(); + + #[cfg(unix)] + let iter = itertools::chain!( + iter, + nix::sys::signal::Signal::iterator().map(TrapSignal::Signal) + ); + + iter + } + + /// Converts [`TrapSignal`] into its corresponding signal name as a [`&'static str`](str) + pub const fn as_str(self) -> &'static str { match self { #[cfg(unix)] - TrapSignal::Signal(s) => s.fmt(f), - TrapSignal::Debug => write!(f, "DEBUG"), - TrapSignal::Err => write!(f, "ERR"), - TrapSignal::Exit => write!(f, "EXIT"), + TrapSignal::Signal(s) => s.as_str(), + TrapSignal::Debug => "DEBUG", + TrapSignal::Err => "ERR", + TrapSignal::Exit => "EXIT", } } } -impl TrapSignal { - /// Returns all possible values of `TrapSignal`. - #[allow(unused_mut)] - pub fn all_values() -> Vec { - let mut signals = vec![TrapSignal::Debug, TrapSignal::Err, TrapSignal::Exit]; +/// Formats [`Iterator`](TrapSignal) to the provided writer. +/// +/// # Arguments +/// +/// * `f` - Any type that implements [`std::io::Write`]. +/// * `it` - An iterator over the signals that will be formatted into the `f`. +pub fn format_signals( + mut f: impl std::io::Write, + it: impl Iterator, +) -> Result<(), error::Error> { + let it = it + .sorted_by(|a, b| Ord::cmp(&i32::from(*a), &i32::from(*b))) + .format_with("\n", |s, f| f(&format_args!("{}) {s}", i32::from(s)))); + write!(f, "{it}")?; + Ok(()) +} - #[cfg(unix)] - for signal in nix::sys::signal::Signal::iterator() { - signals.push(TrapSignal::Signal(signal)); +// implement s.parse::() +impl FromStr for TrapSignal { + type Err = error::Error; + fn from_str(s: &str) -> Result::Err> { + if let Ok(n) = s.parse::() { + TrapSignal::try_from(n) + } else { + TrapSignal::try_from(s) + } + } +} + +// from a signal number +impl TryFrom for TrapSignal { + type Error = error::Error; + fn try_from(value: i32) -> Result { + Ok(match value { + 32 => TrapSignal::Debug, + 33 => TrapSignal::Err, + 0 => TrapSignal::Exit, + #[cfg(unix)] + value => TrapSignal::Signal( + nix::sys::signal::Signal::try_from(value) + .map_err(|_| error::Error::InvalidSignal(value.to_string()))?, + ), + #[cfg(not(unix))] + _ => return Err(error::Error::InvalidSignal(value.to_string())), + }) + } +} + +// from a signal name +impl TryFrom<&str> for TrapSignal { + type Error = error::Error; + fn try_from(value: &str) -> Result { + let mut s = value.to_ascii_uppercase(); + // Bash compatibility: + // support for signal names without the `SIG` prefix, for example `HUP` -> `SIGHUP` + if !s.starts_with("SIG") { + s.insert_str(0, "SIG"); } - signals + Ok(match s.as_str() { + "SIGDEBUG" => TrapSignal::Debug, + "SIGERR" => TrapSignal::Err, + "SIGEXIT" => TrapSignal::Exit, + + #[cfg(unix)] + _ => nix::sys::signal::Signal::from_str(s.as_str()) + .map(TrapSignal::Signal) + .map_err(|_| error::Error::InvalidSignal(value.into()))?, + #[cfg(not(unix))] + _ => return Err(error::Error::InvalidSignal(value.into())), + }) + } +} + +impl From for i32 { + fn from(value: TrapSignal) -> Self { + match value { + #[cfg(unix)] + TrapSignal::Signal(s) => s as i32, + TrapSignal::Debug => 32, + TrapSignal::Err => 33, + TrapSignal::Exit => 0, + } } } diff --git a/brush-shell/tests/cases/builtins/kill.yaml b/brush-shell/tests/cases/builtins/kill.yaml index 61207e7f..2779f322 100644 --- a/brush-shell/tests/cases/builtins/kill.yaml +++ b/brush-shell/tests/cases/builtins/kill.yaml @@ -6,7 +6,6 @@ cases: # 1) .. 2) .. 3) .. etc. instead of just a simple list stdin: | # kill -l - # kill -l 0 # TODO: EXIT signal kill -l 1 kill -l 2 kill -l 3 @@ -36,8 +35,14 @@ cases: kill -l 28 kill -l 29 kill -l 30 + kill -l 31 # invalid option kill -l 9999 kill -l HUP + kill -l iNt + kill -l int kill -l SIGHUP + kill -l EXIT + kill -l ERR + kill -l DEBUG