From ef642b3834ed0f3db0cc11e2fbc8e899c2ee090d Mon Sep 17 00:00:00 2001 From: Tom D Date: Thu, 23 Mar 2023 13:51:22 +0100 Subject: [PATCH] Refactor signal handling in yes, tee, and timeout Yes, using libc while using nix was a bit redundant. Upon investigation, duplicated code was found and moved to uucore. --- Cargo.lock | 1 - src/uu/tee/src/tee.rs | 49 +++++++------------------- src/uu/timeout/src/timeout.rs | 27 +++++--------- src/uu/yes/Cargo.toml | 8 +++-- src/uu/yes/src/yes.rs | 25 +++---------- src/uucore/Cargo.toml | 2 +- src/uucore/src/lib/features/signals.rs | 20 ++++++++++- src/uucore/src/lib/mods/error.rs | 10 +++--- 8 files changed, 56 insertions(+), 86 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8b9a7fda807..1b9a1150a80 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3272,7 +3272,6 @@ name = "uu_yes" version = "0.0.17" dependencies = [ "clap", - "libc", "nix", "uucore", ] diff --git a/src/uu/tee/src/tee.rs b/src/uu/tee/src/tee.rs index 96c318afd40..f8e6ebe4193 100644 --- a/src/uu/tee/src/tee.rs +++ b/src/uu/tee/src/tee.rs @@ -16,7 +16,7 @@ use uucore::{format_usage, help_about, help_section, help_usage, show_error}; // spell-checker:ignore nopipe #[cfg(unix)] -use uucore::libc; +use uucore::signals::{enable_pipe_errors, ignore_interrupts}; const ABOUT: &str = help_about!("tee.md"); const USAGE: &str = help_usage!("tee.md"); @@ -135,44 +135,19 @@ pub fn uu_app() -> Command { ) } -#[cfg(unix)] -fn ignore_interrupts() -> Result<()> { - let ret = unsafe { libc::signal(libc::SIGINT, libc::SIG_IGN) }; - if ret == libc::SIG_ERR { - return Err(Error::new(ErrorKind::Other, "")); - } - Ok(()) -} - -#[cfg(not(unix))] -fn ignore_interrupts() -> Result<()> { - // Do nothing. - Ok(()) -} - -#[cfg(unix)] -fn enable_pipe_errors() -> Result<()> { - let ret = unsafe { libc::signal(libc::SIGPIPE, libc::SIG_DFL) }; - if ret == libc::SIG_ERR { - return Err(Error::new(ErrorKind::Other, "")); - } - Ok(()) -} - -#[cfg(not(unix))] -fn enable_pipe_errors() -> Result<()> { - // Do nothing. - Ok(()) -} - fn tee(options: &Options) -> Result<()> { - if options.ignore_interrupts { - ignore_interrupts()?; - } - if options.output_error.is_none() { - enable_pipe_errors()?; - } + #[cfg(unix)] + { + // ErrorKind::Other is raised by MultiWriter when all writers have exited. + // This is therefore just a clever way to stop all writers + if options.ignore_interrupts { + ignore_interrupts().map_err(|_| Error::from(ErrorKind::Other))?; + } + if options.output_error.is_none() { + enable_pipe_errors().map_err(|_| Error::from(ErrorKind::Other))?; + } + } let mut writers: Vec = options .files .clone() diff --git a/src/uu/timeout/src/timeout.rs b/src/uu/timeout/src/timeout.rs index fe342727ba5..153beea3d66 100644 --- a/src/uu/timeout/src/timeout.rs +++ b/src/uu/timeout/src/timeout.rs @@ -17,8 +17,14 @@ use std::time::Duration; use uucore::display::Quotable; use uucore::error::{UClapError, UResult, USimpleError, UUsageError}; use uucore::process::ChildExt; -use uucore::signals::{signal_by_name_or_value, signal_name_by_value}; -use uucore::{format_usage, show_error}; + +#[cfg(unix)] +use uucore::signals::enable_pipe_errors; + +use uucore::{ + format_usage, show_error, + signals::{signal_by_name_or_value, signal_name_by_value}, +}; static ABOUT: &str = "Start COMMAND, and kill it if still running after DURATION."; const USAGE: &str = "{} [OPTION] DURATION COMMAND..."; @@ -285,21 +291,6 @@ fn preserve_signal_info(signal: libc::c_int) -> libc::c_int { signal } -#[cfg(unix)] -fn enable_pipe_errors() -> std::io::Result<()> { - let ret = unsafe { libc::signal(libc::SIGPIPE, libc::SIG_DFL) }; - if ret == libc::SIG_ERR { - return Err(std::io::Error::new(std::io::ErrorKind::Other, "")); - } - Ok(()) -} - -#[cfg(not(unix))] -fn enable_pipe_errors() -> std::io::Result<()> { - // Do nothing. - Ok(()) -} - /// TODO: Improve exit codes, and make them consistent with the GNU Coreutils exit codes. fn timeout( @@ -314,7 +305,7 @@ fn timeout( if !foreground { unsafe { libc::setpgid(0, 0) }; } - + #[cfg(unix)] enable_pipe_errors()?; let process = &mut process::Command::new(&cmd[0]) diff --git a/src/uu/yes/Cargo.toml b/src/uu/yes/Cargo.toml index cf120f2e0ba..fd2aca5b498 100644 --- a/src/uu/yes/Cargo.toml +++ b/src/uu/yes/Cargo.toml @@ -16,12 +16,14 @@ path = "src/yes.rs" [dependencies] clap = { workspace=true } -libc = { workspace=true } -uucore = { workspace=true, features=["pipes"] } -[target.'cfg(any(target_os = "linux", target_os = "android"))'.dependencies] +[target.'cfg(unix)'.dependencies] +uucore = { workspace=true, features=["pipes", "signals"] } nix = { workspace=true } +[target.'cfg(not(unix))'.dependencies] +uucore = { workspace=true, features=["pipes"] } + [[bin]] name = "yes" path = "src/main.rs" diff --git a/src/uu/yes/src/yes.rs b/src/uu/yes/src/yes.rs index 714bdcac514..41bfeddca62 100644 --- a/src/uu/yes/src/yes.rs +++ b/src/uu/yes/src/yes.rs @@ -7,13 +7,13 @@ /* last synced with: yes (GNU coreutils) 8.13 */ -use std::borrow::Cow; -use std::io::{self, Result, Write}; - use clap::{Arg, ArgAction, Command}; +use std::borrow::Cow; +use std::io::{self, Write}; use uucore::error::{UResult, USimpleError}; +#[cfg(unix)] +use uucore::signals::enable_pipe_errors; use uucore::{format_usage, help_about, help_usage}; - #[cfg(any(target_os = "linux", target_os = "android"))] mod splice; @@ -69,25 +69,10 @@ fn prepare_buffer<'a>(input: &'a str, buffer: &'a mut [u8; BUF_SIZE]) -> &'a [u8 } } -#[cfg(unix)] -fn enable_pipe_errors() -> Result<()> { - let ret = unsafe { libc::signal(libc::SIGPIPE, libc::SIG_DFL) }; - if ret == libc::SIG_ERR { - return Err(io::Error::new(io::ErrorKind::Other, "")); - } - Ok(()) -} - -#[cfg(not(unix))] -fn enable_pipe_errors() -> Result<()> { - // Do nothing. - Ok(()) -} - pub fn exec(bytes: &[u8]) -> io::Result<()> { let stdout = io::stdout(); let mut stdout = stdout.lock(); - + #[cfg(unix)] enable_pipe_errors()?; #[cfg(any(target_os = "linux", target_os = "android"))] diff --git a/src/uucore/Cargo.toml b/src/uucore/Cargo.toml index f5bec905397..4ca3941db7d 100644 --- a/src/uucore/Cargo.toml +++ b/src/uucore/Cargo.toml @@ -49,7 +49,7 @@ sm3 = { workspace=true } [target.'cfg(unix)'.dependencies] walkdir = { workspace=true, optional=true } -nix = { workspace=true, features = ["fs", "uio", "zerocopy"] } +nix = { workspace=true, features = ["fs", "uio", "zerocopy", "signal"] } [dev-dependencies] clap = { workspace=true } diff --git a/src/uucore/src/lib/features/signals.rs b/src/uucore/src/lib/features/signals.rs index b92ecf764df..c9f7295ee30 100644 --- a/src/uucore/src/lib/features/signals.rs +++ b/src/uucore/src/lib/features/signals.rs @@ -7,7 +7,12 @@ // spell-checker:ignore (vars/api) fcntl setrlimit setitimer // spell-checker:ignore (vars/signals) ABRT ALRM CHLD SEGV SIGABRT SIGALRM SIGBUS SIGCHLD SIGCONT SIGEMT SIGFPE SIGHUP SIGILL SIGINFO SIGINT SIGIO SIGIOT SIGKILL SIGPIPE SIGPROF SIGPWR SIGQUIT SIGSEGV SIGSTOP SIGSYS SIGTERM SIGTRAP SIGTSTP SIGTHR SIGTTIN SIGTTOU SIGURG SIGUSR SIGVTALRM SIGWINCH SIGXCPU SIGXFSZ STKFLT PWR THR TSTP TTIN TTOU VTALRM XCPU XFSZ - +#[cfg(unix)] +use nix::errno::Errno; +#[cfg(unix)] +use nix::sys::signal::{ + signal, SigHandler::SigDfl, SigHandler::SigIgn, Signal::SIGINT, Signal::SIGPIPE, +}; pub static DEFAULT_SIGNAL: usize = 15; /* @@ -196,6 +201,19 @@ pub fn signal_name_by_value(signal_value: usize) -> Option<&'static str> { ALL_SIGNALS.get(signal_value).copied() } +#[cfg(unix)] +pub fn enable_pipe_errors() -> Result<(), Errno> { + // We pass the error as is, the return value would just be Ok(SigDfl), so we can safely ignore it. + // SAFETY: this function is safe as long as we do not use a custom SigHandler -- we use the default one. + unsafe { signal(SIGPIPE, SigDfl) }.map(|_| ()) +} +#[cfg(unix)] +pub fn ignore_interrupts() -> Result<(), Errno> { + // We pass the error as is, the return value would just be Ok(SigIgn), so we can safely ignore it. + // SAFETY: this function is safe as long as we do not use a custom SigHandler -- we use the default one. + unsafe { signal(SIGINT, SigIgn) }.map(|_| ()) +} + #[test] fn signal_by_value() { assert_eq!(signal_by_name_or_value("0"), Some(0)); diff --git a/src/uucore/src/lib/mods/error.rs b/src/uucore/src/lib/mods/error.rs index 5d4527d746c..e564c7bb5ec 100644 --- a/src/uucore/src/lib/mods/error.rs +++ b/src/uucore/src/lib/mods/error.rs @@ -522,7 +522,7 @@ impl From for Box { /// // prints "fix me please!: Permission denied" /// println!("{}", uio_result.unwrap_err()); /// ``` -#[cfg(any(target_os = "linux", target_os = "android"))] +#[cfg(unix)] impl FromIo> for Result { fn map_err_context(self, context: impl FnOnce() -> String) -> UResult { self.map_err(|e| { @@ -534,7 +534,7 @@ impl FromIo> for Result { } } -#[cfg(any(target_os = "linux", target_os = "android"))] +#[cfg(unix)] impl FromIo> for nix::Error { fn map_err_context(self, context: impl FnOnce() -> String) -> UResult { Err(Box::new(UIoError { @@ -544,7 +544,7 @@ impl FromIo> for nix::Error { } } -#[cfg(any(target_os = "linux", target_os = "android"))] +#[cfg(unix)] impl From for UIoError { fn from(f: nix::Error) -> Self { Self { @@ -554,7 +554,7 @@ impl From for UIoError { } } -#[cfg(any(target_os = "linux", target_os = "android"))] +#[cfg(unix)] impl From for Box { fn from(f: nix::Error) -> Self { let u_error: UIoError = f.into(); @@ -751,7 +751,7 @@ impl Display for ClapErrorWrapper { #[cfg(test)] mod tests { #[test] - #[cfg(any(target_os = "linux", target_os = "android"))] + #[cfg(unix)] fn test_nix_error_conversion() { use super::{FromIo, UIoError}; use nix::errno::Errno;