Skip to content

Commit

Permalink
Refactor signal handling in yes, tee, and timeout
Browse files Browse the repository at this point in the history
Yes, using libc while using nix was a bit redundant.
Upon investigation, duplicated code was found and moved to uucore.
  • Loading branch information
anastygnome committed Mar 24, 2023
1 parent cc77a95 commit 980b2b0
Show file tree
Hide file tree
Showing 7 changed files with 44 additions and 54 deletions.
1 change: 0 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 3 additions & 16 deletions src/uu/tee/src/tee.rs
Expand Up @@ -11,7 +11,9 @@ use std::io::{copy, sink, stdin, stdout, Error, ErrorKind, Read, Result, Write};
use std::path::PathBuf;
use uucore::display::Quotable;
use uucore::error::UResult;
use uucore::{format_usage, help_about, help_section, help_usage, show_error};
use uucore::{
format_usage, help_about, help_section, help_usage, show_error, signals::enable_pipe_errors,
};

// spell-checker:ignore nopipe

Expand Down Expand Up @@ -150,21 +152,6 @@ fn ignore_interrupts() -> Result<()> {
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()?;
Expand Down
17 changes: 1 addition & 16 deletions src/uu/timeout/src/timeout.rs
Expand Up @@ -17,7 +17,7 @@ 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::signals::{enable_pipe_errors, signal_by_name_or_value, signal_name_by_value};
use uucore::{format_usage, show_error};

static ABOUT: &str = "Start COMMAND, and kill it if still running after DURATION.";
Expand Down Expand Up @@ -285,21 +285,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(
Expand Down
5 changes: 2 additions & 3 deletions src/uu/yes/Cargo.toml
Expand Up @@ -16,11 +16,10 @@ 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]
nix = { workspace=true }
[target.'cfg(unix)'.dependencies]
nix = { workspace=true, features = ["signal"] }

[[bin]]
name = "yes"
Expand Down
20 changes: 3 additions & 17 deletions src/uu/yes/src/yes.rs
Expand Up @@ -8,11 +8,12 @@
/* last synced with: yes (GNU coreutils) 8.13 */

use std::borrow::Cow;
use std::io::{self, Result, Write};
use std::io::{self, Write};

use clap::{Arg, ArgAction, Command};
#[cfg(unix)]
use uucore::error::{UResult, USimpleError};
use uucore::{format_usage, help_about, help_usage};
use uucore::{format_usage, help_about, help_usage, signals::enable_pipe_errors};

#[cfg(any(target_os = "linux", target_os = "android"))]
mod splice;
Expand Down Expand Up @@ -69,21 +70,6 @@ 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();
Expand Down
2 changes: 1 addition & 1 deletion src/uucore/Cargo.toml
Expand Up @@ -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 }
Expand Down
34 changes: 34 additions & 0 deletions src/uucore/src/lib/features/signals.rs
Expand Up @@ -7,6 +7,10 @@

// 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
use nix::sys::signal::{
signal, SigHandler::SigDfl, SigHandler::SigIgn, Signal::SIGINT, Signal::SIGPIPE,
};
use std::io::{Error, ErrorKind, Result};

pub static DEFAULT_SIGNAL: usize = 15;

Expand Down Expand Up @@ -196,6 +200,36 @@ 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<()> {
// SAFETY: this function is safe as long as we do not use a custom SigHandler -- we use the default one.
match unsafe { signal(SIGPIPE, SigDfl) } {
Ok(_) => Ok(()),
_ => Err(Error::from(ErrorKind::Other)),
}
}

#[cfg(not(unix))]
pub fn enable_pipe_errors() -> Result<()> {
// Do nothing.
Ok(())
}

#[cfg(unix)]
pub fn ignore_interrupts() -> Result<()> {
// SAFETY: this function is safe as long as we do not use a custom SigHandler -- we use the default one.
match unsafe { signal(SIGINT, SigIgn) } {
Ok(_) => Ok(()),
_ => Err(Error::from(ErrorKind::Other)),
}
}

#[cfg(not(unix))]
pub fn ignore_interrupts() -> Result<()> {
// Do nothing.
Ok(())
}

#[test]
fn signal_by_value() {
assert_eq!(signal_by_name_or_value("0"), Some(0));
Expand Down

0 comments on commit 980b2b0

Please sign in to comment.