From a3605045742c721d72bb1a9b6b397382cc7bcd51 Mon Sep 17 00:00:00 2001 From: Ed Smith Date: Mon, 20 Jun 2022 16:48:20 +0100 Subject: [PATCH 1/4] Implement tee -p and --output-error This has the following behaviours. On Unix: - The default is to exit on pipe errors, and warn on other errors. - "--output-error=warn" means to warn on all errors - "--output-error", "--output-error=warn-nopipe" and "-p" all mean that pipe errors are suppressed, all other errors warn. - "--output-error=exit" means to warn and exit on all errors. - "--output-error=exit-nopipe" means to suppress pipe errors, and to warn and exit on all other errors. On non-Unix platforms, all pipe behaviours are ignored, so the default is effectively "--output-error=warn" and "warn-nopipe" is identical. The only meaningful option is "--output-error=exit" which is identical to "--output-error=exit-nopipe" on these platforms. Note that warnings give a non-zero exit code, but do not halt writing to non-erroring targets. --- src/uu/tee/src/tee.rs | 183 ++++++++++++++++++-- tests/by-util/test_tee.rs | 355 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 521 insertions(+), 17 deletions(-) diff --git a/src/uu/tee/src/tee.rs b/src/uu/tee/src/tee.rs index 7b6b66208..319fab9e9 100644 --- a/src/uu/tee/src/tee.rs +++ b/src/uu/tee/src/tee.rs @@ -8,7 +8,7 @@ #[macro_use] extern crate uucore; -use clap::{crate_version, Arg, Command}; +use clap::{crate_version, Arg, Command, PossibleValue}; use retain_mut::RetainMut; use std::fs::OpenOptions; use std::io::{copy, sink, stdin, stdout, Error, ErrorKind, Read, Result, Write}; @@ -17,6 +17,8 @@ use uucore::display::Quotable; use uucore::error::UResult; use uucore::format_usage; +// spell-checker:ignore nopipe + #[cfg(unix)] use uucore::libc; @@ -27,6 +29,8 @@ mod options { pub const APPEND: &str = "append"; pub const IGNORE_INTERRUPTS: &str = "ignore-interrupts"; pub const FILE: &str = "file"; + pub const IGNORE_PIPE_ERRORS: &str = "ignore-pipe-errors"; + pub const OUTPUT_ERROR: &str = "output-error"; } #[allow(dead_code)] @@ -34,6 +38,15 @@ struct Options { append: bool, ignore_interrupts: bool, files: Vec, + output_error: Option, +} + +#[derive(Clone, Debug)] +enum OutputErrorMode { + Warn, + WarnNoPipe, + Exit, + ExitNoPipe, } #[uucore::main] @@ -47,6 +60,25 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { .values_of(options::FILE) .map(|v| v.map(ToString::to_string).collect()) .unwrap_or_default(), + output_error: { + if matches.is_present(options::IGNORE_PIPE_ERRORS) { + Some(OutputErrorMode::WarnNoPipe) + } else if matches.is_present(options::OUTPUT_ERROR) { + if let Some(v) = matches.value_of(options::OUTPUT_ERROR) { + match v { + "warn" => Some(OutputErrorMode::Warn), + "warn-nopipe" => Some(OutputErrorMode::WarnNoPipe), + "exit" => Some(OutputErrorMode::Exit), + "exit-nopipe" => Some(OutputErrorMode::ExitNoPipe), + _ => unreachable!(), + } + } else { + Some(OutputErrorMode::WarnNoPipe) + } + } else { + None + } + }, }; match tee(&options) { @@ -79,6 +111,29 @@ pub fn uu_app<'a>() -> Command<'a> { .multiple_occurrences(true) .value_hint(clap::ValueHint::FilePath), ) + .arg( + Arg::new(options::IGNORE_PIPE_ERRORS) + .short('p') + .help("set write error behavior (ignored on non-Unix platforms)"), + ) + .arg( + Arg::new(options::OUTPUT_ERROR) + .long(options::OUTPUT_ERROR) + .require_equals(true) + .min_values(0) + .max_values(1) + .possible_values([ + PossibleValue::new("warn") + .help("produce warnings for errors writing to any output"), + PossibleValue::new("warn-nopipe") + .help("produce warnings for errors that are not pipe errors (ignored on non-unix platforms)"), + PossibleValue::new("exit").help("exit on write errors to any output"), + PossibleValue::new("exit-nopipe") + .help("exit on write errors to any output that are not pipe errors (equivalent to exit on non-unix platforms)"), + ]) + .help("set write error behavior") + .conflicts_with(options::IGNORE_PIPE_ERRORS), + ) } #[cfg(unix)] @@ -96,19 +151,40 @@ 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()?; } + if options.output_error.is_none() { + enable_pipe_errors()?; + } + let mut writers: Vec = options .files .clone() .into_iter() - .map(|file| NamedWriter { - name: file.clone(), - inner: open(file, options.append), + .map(|file| { + Ok(NamedWriter { + name: file.clone(), + inner: open(file, options.append, options.output_error.as_ref())?, + }) }) - .collect(); + .collect::>>()?; writers.insert( 0, @@ -118,7 +194,7 @@ fn tee(options: &Options) -> Result<()> { }, ); - let mut output = MultiWriter::new(writers); + let mut output = MultiWriter::new(writers, options.output_error.clone()); let input = &mut NamedReader { inner: Box::new(stdin()) as Box, }; @@ -132,7 +208,11 @@ fn tee(options: &Options) -> Result<()> { } } -fn open(name: String, append: bool) -> Box { +fn open( + name: String, + append: bool, + output_error: Option<&OutputErrorMode>, +) -> Result> { let path = PathBuf::from(name.clone()); let inner: Box = { let mut options = OpenOptions::new(); @@ -143,56 +223,125 @@ fn open(name: String, append: bool) -> Box { }; match mode.write(true).create(true).open(path.as_path()) { Ok(file) => Box::new(file), - Err(_) => Box::new(sink()), + Err(f) => { + show_error!("{}: {}", name.maybe_quote(), f); + match output_error { + Some(OutputErrorMode::Exit) | Some(OutputErrorMode::ExitNoPipe) => { + return Err(f) + } + _ => Box::new(sink()), + } + } } }; - Box::new(NamedWriter { inner, name }) as Box + Ok(Box::new(NamedWriter { inner, name }) as Box) } struct MultiWriter { writers: Vec, - initial_len: usize, + output_error_mode: Option, + ignored_errors: usize, } impl MultiWriter { - fn new(writers: Vec) -> Self { + fn new(writers: Vec, output_error_mode: Option) -> Self { Self { - initial_len: writers.len(), writers, + output_error_mode, + ignored_errors: 0, } } + fn error_occurred(&self) -> bool { - self.writers.len() != self.initial_len + self.ignored_errors != 0 + } +} + +fn process_error( + mode: Option<&OutputErrorMode>, + f: Error, + writer: &NamedWriter, + ignored_errors: &mut usize, +) -> Result<()> { + match mode { + Some(OutputErrorMode::Warn) => { + show_error!("{}: {}", writer.name.maybe_quote(), f); + *ignored_errors += 1; + Ok(()) + } + Some(OutputErrorMode::WarnNoPipe) | None => { + if f.kind() != ErrorKind::BrokenPipe { + show_error!("{}: {}", writer.name.maybe_quote(), f); + *ignored_errors += 1; + } + Ok(()) + } + Some(OutputErrorMode::Exit) => { + show_error!("{}: {}", writer.name.maybe_quote(), f); + Err(f) + } + Some(OutputErrorMode::ExitNoPipe) => { + if f.kind() != ErrorKind::BrokenPipe { + show_error!("{}: {}", writer.name.maybe_quote(), f); + Err(f) + } else { + Ok(()) + } + } } } impl Write for MultiWriter { fn write(&mut self, buf: &[u8]) -> Result { + let mut aborted = None; + let mode = self.output_error_mode.clone(); + let mut errors = 0; RetainMut::retain_mut(&mut self.writers, |writer| { let result = writer.write_all(buf); match result { Err(f) => { - show_error!("{}: {}", writer.name.maybe_quote(), f); + if let Err(e) = process_error(mode.as_ref(), f, writer, &mut errors) { + if aborted.is_none() { + aborted = Some(e); + } + } false } _ => true, } }); - Ok(buf.len()) + self.ignored_errors += errors; + if let Some(e) = aborted { + Err(e) + } else { + Ok(buf.len()) + } } fn flush(&mut self) -> Result<()> { + let mut aborted = None; + let mode = self.output_error_mode.clone(); + let mut errors = 0; RetainMut::retain_mut(&mut self.writers, |writer| { let result = writer.flush(); match result { Err(f) => { - show_error!("{}: {}", writer.name.maybe_quote(), f); + if let Err(e) = process_error(mode.as_ref(), f, writer, &mut errors) { + if aborted.is_none() { + aborted = Some(e); + } + } false } _ => true, } }); - Ok(()) + self.ignored_errors += errors; + if let Some(e) = aborted { + Err(e) + } else { + Ok(()) + } } } diff --git a/tests/by-util/test_tee.rs b/tests/by-util/test_tee.rs index e89b9d438..7fd4d77b0 100644 --- a/tests/by-util/test_tee.rs +++ b/tests/by-util/test_tee.rs @@ -4,6 +4,8 @@ use crate::common::util::*; // inspired by: // https://github.com/coreutils/coreutils/tests/misc/tee.sh +// spell-checker:ignore nopipe + #[test] fn test_tee_processing_multiple_operands() { // POSIX says: "Processing of at least 13 file operands shall be supported." @@ -98,3 +100,356 @@ fn test_tee_no_more_writeable_2() { // assert_eq!(at.read(file_out_b), content); // assert!(result.stderr.contains("No space left on device")); } + +#[cfg(target_os = "linux")] +mod linux_only { + use crate::common::util::*; + + use std::fs::File; + use std::io::Write; + use std::process::Output; + use std::thread; + + fn make_broken_pipe() -> File { + use libc::c_int; + use std::os::unix::io::FromRawFd; + + let mut fds: [c_int; 2] = [0, 0]; + if unsafe { libc::pipe(&mut fds as *mut c_int) } != 0 { + panic!("Failed to create pipe"); + } + + // Drop the read end of the pipe + let _ = unsafe { File::from_raw_fd(fds[0]) }; + + // Make the write end of the pipe into a Rust File + unsafe { File::from_raw_fd(fds[1]) } + } + + fn run_tee(proc: &mut UCommand) -> (String, Output) { + let content = (1..=100000).map(|x| format!("{}\n", x)).collect::(); + + let mut prog = proc.run_no_wait(); + + let mut stdin = prog + .stdin + .take() + .unwrap_or_else(|| panic!("Could not take child process stdin")); + + let c = content.clone(); + let thread = thread::spawn(move || { + let _ = stdin.write_all(c.as_bytes()); + }); + + let output = prog.wait_with_output().unwrap(); + + thread.join().unwrap(); + + (content, output) + } + + fn expect_success(output: &Output) { + assert!( + output.status.success(), + "Command was expected to succeed.\nstdout = {}\n stderr = {}", + std::str::from_utf8(&output.stdout).unwrap(), + std::str::from_utf8(&output.stderr).unwrap(), + ); + assert!( + output.stderr.is_empty(), + "Unexpected data on stderr.\n stderr = {}", + std::str::from_utf8(&output.stderr).unwrap(), + ); + } + + fn expect_failure(output: &Output, message: &str) { + assert!( + !output.status.success(), + "Command was expected to fail.\nstdout = {}\n stderr = {}", + std::str::from_utf8(&output.stdout).unwrap(), + std::str::from_utf8(&output.stderr).unwrap(), + ); + assert!( + std::str::from_utf8(&output.stderr) + .unwrap() + .contains(message), + "Expected to see error message fragment {} in stderr, but did not.\n stderr = {}", + message, + std::str::from_utf8(&output.stderr).unwrap(), + ); + } + + fn expect_silent_failure(output: &Output) { + assert!( + !output.status.success(), + "Command was expected to fail.\nstdout = {}\n stderr = {}", + std::str::from_utf8(&output.stdout).unwrap(), + std::str::from_utf8(&output.stderr).unwrap(), + ); + assert!( + output.stderr.is_empty(), + "Unexpected data on stderr.\n stderr = {}", + std::str::from_utf8(&output.stderr).unwrap(), + ); + } + + fn expect_correct(name: &str, at: &AtPath, contents: &str) { + assert!(at.file_exists(name)); + let compare = at.read(name); + assert_eq!(compare, contents); + } + + fn expect_short(name: &str, at: &AtPath, contents: &str) { + assert!(at.file_exists(name)); + let compare = at.read(name); + assert!( + compare.len() < contents.len(), + "Too many bytes ({}) written to {} (should be a short count from {})", + compare.len(), + name, + contents.len() + ); + assert!(contents.starts_with(&compare), + "Expected truncated output to be a prefix of the correct output, but it isn't.\n Correct: {}\n Compare: {}", + contents, + compare); + } + + #[test] + fn test_pipe_error_default() { + let (at, mut ucmd) = at_and_ucmd!(); + + let file_out_a = "tee_file_out_a"; + + let proc = ucmd.arg(file_out_a).set_stdout(make_broken_pipe()); + + let (content, output) = run_tee(proc); + + expect_silent_failure(&output); + expect_short(file_out_a, &at, content.as_str()); + } + + #[test] + fn test_pipe_error_warn_nopipe_1() { + let (at, mut ucmd) = at_and_ucmd!(); + + let file_out_a = "tee_file_out_a"; + + let proc = ucmd + .arg("-p") + .arg(file_out_a) + .set_stdout(make_broken_pipe()); + + let (content, output) = run_tee(proc); + + expect_success(&output); + expect_correct(file_out_a, &at, content.as_str()); + } + + #[test] + fn test_pipe_error_warn_nopipe_2() { + let (at, mut ucmd) = at_and_ucmd!(); + + let file_out_a = "tee_file_out_a"; + + let proc = ucmd + .arg("--output-error") + .arg(file_out_a) + .set_stdout(make_broken_pipe()); + + let (content, output) = run_tee(proc); + + expect_success(&output); + expect_correct(file_out_a, &at, content.as_str()); + } + + #[test] + fn test_pipe_error_warn_nopipe_3() { + let (at, mut ucmd) = at_and_ucmd!(); + + let file_out_a = "tee_file_out_a"; + + let proc = ucmd + .arg("--output-error=warn-nopipe") + .arg(file_out_a) + .set_stdout(make_broken_pipe()); + + let (content, output) = run_tee(proc); + + expect_success(&output); + expect_correct(file_out_a, &at, content.as_str()); + } + + #[test] + fn test_pipe_error_warn() { + let (at, mut ucmd) = at_and_ucmd!(); + + let file_out_a = "tee_file_out_a"; + + let proc = ucmd + .arg("--output-error=warn") + .arg(file_out_a) + .set_stdout(make_broken_pipe()); + + let (content, output) = run_tee(proc); + + expect_failure(&output, "Broken pipe"); + expect_correct(file_out_a, &at, content.as_str()); + } + + #[test] + fn test_pipe_error_exit() { + let (at, mut ucmd) = at_and_ucmd!(); + + let file_out_a = "tee_file_out_a"; + + let proc = ucmd + .arg("--output-error=exit") + .arg(file_out_a) + .set_stdout(make_broken_pipe()); + + let (content, output) = run_tee(proc); + + expect_failure(&output, "Broken pipe"); + expect_short(file_out_a, &at, content.as_str()); + } + + #[test] + fn test_pipe_error_exit_nopipe() { + let (at, mut ucmd) = at_and_ucmd!(); + + let file_out_a = "tee_file_out_a"; + + let proc = ucmd + .arg("--output-error=exit-nopipe") + .arg(file_out_a) + .set_stdout(make_broken_pipe()); + + let (content, output) = run_tee(proc); + + expect_success(&output); + expect_correct(file_out_a, &at, content.as_str()); + } + + #[test] + fn test_space_error_default() { + let (at, mut ucmd) = at_and_ucmd!(); + + let file_out_a = "tee_file_out_a"; + + let proc = ucmd.arg(file_out_a).arg("/dev/full"); + + let (content, output) = run_tee(proc); + + expect_failure(&output, "No space left"); + expect_correct(file_out_a, &at, content.as_str()); + } + + #[test] + fn test_space_error_warn_nopipe_1() { + let (at, mut ucmd) = at_and_ucmd!(); + + let file_out_a = "tee_file_out_a"; + + let proc = ucmd + .arg("-p") + .arg(file_out_a) + .arg("/dev/full") + .set_stdout(make_broken_pipe()); + + let (content, output) = run_tee(proc); + + expect_failure(&output, "No space left"); + expect_correct(file_out_a, &at, content.as_str()); + } + + #[test] + fn test_space_error_warn_nopipe_2() { + let (at, mut ucmd) = at_and_ucmd!(); + + let file_out_a = "tee_file_out_a"; + + let proc = ucmd + .arg("--output-error") + .arg(file_out_a) + .arg("/dev/full") + .set_stdout(make_broken_pipe()); + + let (content, output) = run_tee(proc); + + expect_failure(&output, "No space left"); + expect_correct(file_out_a, &at, content.as_str()); + } + + #[test] + fn test_space_error_warn_nopipe_3() { + let (at, mut ucmd) = at_and_ucmd!(); + + let file_out_a = "tee_file_out_a"; + + let proc = ucmd + .arg("--output-error=warn-nopipe") + .arg(file_out_a) + .arg("/dev/full") + .set_stdout(make_broken_pipe()); + + let (content, output) = run_tee(proc); + + expect_failure(&output, "No space left"); + expect_correct(file_out_a, &at, content.as_str()); + } + + #[test] + fn test_space_error_warn() { + let (at, mut ucmd) = at_and_ucmd!(); + + let file_out_a = "tee_file_out_a"; + + let proc = ucmd + .arg("--output-error=warn") + .arg(file_out_a) + .arg("/dev/full") + .set_stdout(make_broken_pipe()); + + let (content, output) = run_tee(proc); + + expect_failure(&output, "No space left"); + expect_correct(file_out_a, &at, content.as_str()); + } + + #[test] + fn test_space_error_exit() { + let (at, mut ucmd) = at_and_ucmd!(); + + let file_out_a = "tee_file_out_a"; + + let proc = ucmd + .arg("--output-error=exit") + .arg(file_out_a) + .arg("/dev/full") + .set_stdout(make_broken_pipe()); + + let (content, output) = run_tee(proc); + + expect_failure(&output, "No space left"); + expect_short(file_out_a, &at, content.as_str()); + } + + #[test] + fn test_space_error_exit_nopipe() { + let (at, mut ucmd) = at_and_ucmd!(); + + let file_out_a = "tee_file_out_a"; + + let proc = ucmd + .arg("--output-error=exit-nopipe") + .arg(file_out_a) + .arg("/dev/full") + .set_stdout(make_broken_pipe()); + + let (content, output) = run_tee(proc); + + expect_failure(&output, "No space left"); + expect_short(file_out_a, &at, content.as_str()); + } +} From 7a961a94a579a24eb287f01aeca90d700fcccacb Mon Sep 17 00:00:00 2001 From: Ed Smith Date: Sat, 25 Jun 2022 23:14:17 +0100 Subject: [PATCH 2/4] Preserve signal exit statuses in timeout When the monitored process exits, the GNU version of timeout will preserve its exit status, including the signal state. This is a partial fix for timeout to enable the tee tests to pass. It removes the default Rust trap for SIGPIPE, and kill itself with the same signal as its child exited with to preserve the signal state. --- src/uu/timeout/src/timeout.rs | 48 +++++++++++++++++++++++++++++++++-- 1 file changed, 46 insertions(+), 2 deletions(-) diff --git a/src/uu/timeout/src/timeout.rs b/src/uu/timeout/src/timeout.rs index baafce4df..a077459c9 100644 --- a/src/uu/timeout/src/timeout.rs +++ b/src/uu/timeout/src/timeout.rs @@ -5,7 +5,7 @@ // * For the full copyright and license information, please view the LICENSE // * file that was distributed with this source code. -// spell-checker:ignore (ToDO) tstr sigstr cmdname setpgid sigchld +// spell-checker:ignore (ToDO) tstr sigstr cmdname setpgid sigchld getpid mod status; #[macro_use] @@ -241,6 +241,47 @@ fn wait_or_kill_process( } } +#[cfg(unix)] +fn preserve_signal_info(signal: libc::c_int) -> libc::c_int { + // This is needed because timeout is expected to preserve the exit + // status of its child. It is not the case that utilities have a + // single simple exit code, that's an illusion some shells + // provide. Instead exit status is really two numbers: + // + // - An exit code if the program ran to completion + // + // - A signal number if the program was terminated by a signal + // + // The easiest way to preserve the latter seems to be to kill + // ourselves with whatever signal our child exited with, which is + // what the following is intended to accomplish. + unsafe { + libc::kill(libc::getpid(), signal); + } + signal +} + +#[cfg(not(unix))] +fn preserve_signal_info(signal: libc::c_int) -> libc::c_int { + // Do nothing + 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( @@ -255,6 +296,9 @@ fn timeout( if !foreground { unsafe { libc::setpgid(0, 0) }; } + + enable_pipe_errors()?; + let mut process = process::Command::new(&cmd[0]) .args(&cmd[1..]) .stdin(Stdio::inherit()) @@ -286,7 +330,7 @@ fn timeout( match process.wait_or_timeout(duration) { Ok(Some(status)) => Err(status .code() - .unwrap_or_else(|| status.signal().unwrap()) + .unwrap_or_else(|| preserve_signal_info(status.signal().unwrap())) .into()), Ok(None) => { report_if_verbose(signal, &cmd[0], verbose); From 5c13e88f8bdd065e5e455ab786ce82d07efa5f78 Mon Sep 17 00:00:00 2001 From: Ed Smith Date: Sat, 25 Jun 2022 23:16:49 +0100 Subject: [PATCH 3/4] Do not trap pipe errors in yes This is part of fixing the tee tests. 'yes' is used by the GNU test suite to identify what the SIGPIPE exit code is on the target platform. By trapping SIGPIPE, it creates a requirement that other utilities also trap SIGPIPE (and exit 0 after SIGPIPE). This is sometimes at odds with their desired behaviour. --- Cargo.lock | 1 + src/uu/yes/Cargo.toml | 1 + src/uu/yes/src/yes.rs | 19 ++++++++++++++++++- tests/by-util/test_yes.rs | 16 +++++++++++++++- 4 files changed, 35 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 367360ea6..97f5c7eff 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3096,6 +3096,7 @@ name = "uu_yes" version = "0.0.14" dependencies = [ "clap 3.1.18", + "libc", "nix", "uucore", ] diff --git a/src/uu/yes/Cargo.toml b/src/uu/yes/Cargo.toml index e2c6d8450..6aaa83371 100644 --- a/src/uu/yes/Cargo.toml +++ b/src/uu/yes/Cargo.toml @@ -16,6 +16,7 @@ path = "src/yes.rs" [dependencies] clap = { version = "3.1", features = ["wrap_help", "cargo"] } +libc = "0.2.126" uucore = { version=">=0.0.11", package="uucore", path="../../uucore", features=["pipes"] } [target.'cfg(any(target_os = "linux", target_os = "android"))'.dependencies] diff --git a/src/uu/yes/src/yes.rs b/src/uu/yes/src/yes.rs index 879f60579..f78ddbfb6 100644 --- a/src/uu/yes/src/yes.rs +++ b/src/uu/yes/src/yes.rs @@ -8,7 +8,7 @@ /* last synced with: yes (GNU coreutils) 8.13 */ use std::borrow::Cow; -use std::io::{self, Write}; +use std::io::{self, Result, Write}; #[macro_use] extern crate clap; @@ -70,10 +70,27 @@ 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(); + enable_pipe_errors()?; + #[cfg(any(target_os = "linux", target_os = "android"))] { match splice::splice_data(bytes, &stdout) { diff --git a/tests/by-util/test_yes.rs b/tests/by-util/test_yes.rs index 7325cb4bc..20561ced6 100644 --- a/tests/by-util/test_yes.rs +++ b/tests/by-util/test_yes.rs @@ -1,7 +1,21 @@ use std::io::Read; +use std::process::ExitStatus; + +#[cfg(unix)] +use std::os::unix::process::ExitStatusExt; use crate::common::util::*; +#[cfg(unix)] +fn check_termination(result: &ExitStatus) { + assert_eq!(result.signal(), Some(libc::SIGPIPE as i32)); +} + +#[cfg(not(unix))] +fn check_termination(result: &ExitStatus) { + assert!(result.success(), "yes did not exit successfully"); +} + /// Run `yes`, capture some of the output, close the pipe, and verify it. fn run(args: &[&str], expected: &[u8]) { let mut cmd = new_ucmd!(); @@ -10,7 +24,7 @@ fn run(args: &[&str], expected: &[u8]) { let mut buf = vec![0; expected.len()]; stdout.read_exact(&mut buf).unwrap(); drop(stdout); - assert!(child.wait().unwrap().success()); + check_termination(&child.wait().unwrap()); assert_eq!(buf.as_slice(), expected); } From 607bf3ca4d34db62f4ecb655ce51c310928fb834 Mon Sep 17 00:00:00 2001 From: Ed Smith Date: Sat, 25 Jun 2022 23:19:30 +0100 Subject: [PATCH 4/4] Terminate on elimination of all writers in tee tee is supposed to exit when there is nothing left to write to. For finite inputs, it can be hard to determine whether this functions correctly, but for tee of infinite streams, it is very important to exit when there is nothing more to write to. --- src/uu/tee/src/tee.rs | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/uu/tee/src/tee.rs b/src/uu/tee/src/tee.rs index 319fab9e9..5096e47f0 100644 --- a/src/uu/tee/src/tee.rs +++ b/src/uu/tee/src/tee.rs @@ -199,10 +199,18 @@ fn tee(options: &Options) -> Result<()> { inner: Box::new(stdin()) as Box, }; - // TODO: replaced generic 'copy' call to be able to stop copying - // if all outputs are closed (due to errors) - if copy(input, &mut output).is_err() || output.flush().is_err() || output.error_occurred() { - Err(Error::new(ErrorKind::Other, "")) + let res = match copy(input, &mut output) { + // ErrorKind::Other is raised by MultiWriter when all writers + // have exited, so that copy will abort. It's equivalent to + // success of this part (if there was an error that should + // cause a failure from any writer, that error would have been + // returned instead). + Err(e) if e.kind() != ErrorKind::Other => Err(e), + _ => Ok(()), + }; + + if res.is_err() || output.flush().is_err() || output.error_occurred() { + Err(Error::from(ErrorKind::Other)) } else { Ok(()) } @@ -313,6 +321,11 @@ impl Write for MultiWriter { self.ignored_errors += errors; if let Some(e) = aborted { Err(e) + } else if self.writers.is_empty() { + // This error kind will never be raised by the standard + // library, so we can use it for early termination of + // `copy` + Err(Error::from(ErrorKind::Other)) } else { Ok(buf.len()) }