From 5c36ed92b8d49001af59a6ab3b5f7f45825f423a Mon Sep 17 00:00:00 2001 From: Ben Wiederhake Date: Sun, 31 Mar 2024 16:47:35 +0200 Subject: [PATCH 1/3] tee: remove unnecessary memory indirection This removes two layers of Box>. The indirection appears to be unintentional, and makes it harder to understand and change the code. --- src/uu/tee/src/tee.rs | 50 +++++++++++++++++++------------------------ 1 file changed, 22 insertions(+), 28 deletions(-) diff --git a/src/uu/tee/src/tee.rs b/src/uu/tee/src/tee.rs index ca6e8a7c611..dea95ca959c 100644 --- a/src/uu/tee/src/tee.rs +++ b/src/uu/tee/src/tee.rs @@ -150,12 +150,7 @@ fn tee(options: &Options) -> Result<()> { .files .clone() .into_iter() - .map(|file| { - Ok(NamedWriter { - name: file.clone(), - inner: open(file, options.append, options.output_error.as_ref())?, - }) - }) + .map(|file| open(file, options.append, options.output_error.as_ref())) .collect::>>()?; writers.insert( @@ -188,31 +183,30 @@ fn tee(options: &Options) -> Result<()> { } } -fn open( - name: String, - append: bool, - output_error: Option<&OutputErrorMode>, -) -> Result> { +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(); - let mode = if append { - options.append(true) - } else { - options.truncate(true) - }; - match mode.write(true).create(true).open(path.as_path()) { - Ok(file) => Box::new(file), - Err(f) => { - show_error!("{}: {}", name.maybe_quote(), f); - match output_error { - Some(OutputErrorMode::Exit | OutputErrorMode::ExitNoPipe) => return Err(f), - _ => Box::new(sink()), - } + let mut options = OpenOptions::new(); + let mode = if append { + options.append(true) + } else { + options.truncate(true) + }; + match mode.write(true).create(true).open(path.as_path()) { + Ok(file) => Ok(NamedWriter { + inner: Box::new(file), + name, + }), + Err(f) => { + show_error!("{}: {}", name.maybe_quote(), f); + match output_error { + Some(OutputErrorMode::Exit | OutputErrorMode::ExitNoPipe) => Err(f), + _ => Ok(NamedWriter { + inner: Box::new(sink()), + name, + }), } } - }; - Ok(Box::new(NamedWriter { inner, name }) as Box) + } } struct MultiWriter { From 8ab825c49f218da7bc64a16083de66ce4bf50959 Mon Sep 17 00:00:00 2001 From: Ben Wiederhake Date: Sun, 31 Mar 2024 16:56:48 +0200 Subject: [PATCH 2/3] tee: correctly handle writing to read-only files --- src/uu/tee/src/tee.rs | 27 ++++++++++++++++----------- tests/by-util/test_tee.rs | 22 ++++++++++++++++++++++ 2 files changed, 38 insertions(+), 11 deletions(-) diff --git a/src/uu/tee/src/tee.rs b/src/uu/tee/src/tee.rs index dea95ca959c..0278cb40470 100644 --- a/src/uu/tee/src/tee.rs +++ b/src/uu/tee/src/tee.rs @@ -5,7 +5,7 @@ use clap::{builder::PossibleValue, crate_version, Arg, ArgAction, Command}; use std::fs::OpenOptions; -use std::io::{copy, sink, stdin, stdout, Error, ErrorKind, Read, Result, Write}; +use std::io::{copy, stdin, stdout, Error, ErrorKind, Read, Result, Write}; use std::path::PathBuf; use uucore::display::Quotable; use uucore::error::UResult; @@ -150,8 +150,9 @@ fn tee(options: &Options) -> Result<()> { .files .clone() .into_iter() - .map(|file| open(file, options.append, options.output_error.as_ref())) + .filter_map(|file| open(file, options.append, options.output_error.as_ref())) .collect::>>()?; + let had_open_errors = writers.len() != options.files.len(); writers.insert( 0, @@ -176,14 +177,21 @@ fn tee(options: &Options) -> Result<()> { _ => Ok(()), }; - if res.is_err() || output.flush().is_err() || output.error_occurred() { + if had_open_errors || res.is_err() || output.flush().is_err() || output.error_occurred() { Err(Error::from(ErrorKind::Other)) } else { Ok(()) } } -fn open(name: String, append: bool, output_error: Option<&OutputErrorMode>) -> Result { +/// Tries to open the indicated file and return it. Reports an error if that's not possible. +/// If that error should lead to program termination, this function returns Some(Err()), +/// otherwise it returns None. +fn open( + name: String, + append: bool, + output_error: Option<&OutputErrorMode>, +) -> Option> { let path = PathBuf::from(name.clone()); let mut options = OpenOptions::new(); let mode = if append { @@ -192,18 +200,15 @@ fn open(name: String, append: bool, output_error: Option<&OutputErrorMode>) -> R options.truncate(true) }; match mode.write(true).create(true).open(path.as_path()) { - Ok(file) => Ok(NamedWriter { + Ok(file) => Some(Ok(NamedWriter { inner: Box::new(file), name, - }), + })), Err(f) => { show_error!("{}: {}", name.maybe_quote(), f); match output_error { - Some(OutputErrorMode::Exit | OutputErrorMode::ExitNoPipe) => Err(f), - _ => Ok(NamedWriter { - inner: Box::new(sink()), - name, - }), + Some(OutputErrorMode::Exit | OutputErrorMode::ExitNoPipe) => Some(Err(f)), + _ => None, } } } diff --git a/tests/by-util/test_tee.rs b/tests/by-util/test_tee.rs index c186682785a..9ff4ea7dcf8 100644 --- a/tests/by-util/test_tee.rs +++ b/tests/by-util/test_tee.rs @@ -3,6 +3,7 @@ // For the full copyright and license information, please view the LICENSE // file that was distributed with this source code. use crate::common::util::TestScenario; +use regex::Regex; #[cfg(target_os = "linux")] use std::fmt::Write; @@ -92,6 +93,27 @@ fn test_tee_no_more_writeable_1() { assert_eq!(at.read(file_out), content); } +#[test] +fn test_readonly() { + let (at, mut ucmd) = at_and_ucmd!(); + let content_tee = "hello"; + let content_file = "world"; + let file_out = "tee_file_out"; + let writable_file = "tee_file_out2"; + at.write(file_out, content_file); + at.set_readonly(file_out); + ucmd.arg(file_out) + .arg(writable_file) + .pipe_in(content_tee) + .ignore_stdin_write_error() + .fails() + .stdout_is(content_tee) + // Windows says "Access is denied" for some reason. + .stderr_matches(&Regex::new("(Permission|Access is) denied").unwrap()); + assert_eq!(at.read(file_out), content_file); + assert_eq!(at.read(writable_file), content_tee); +} + #[test] #[cfg(target_os = "linux")] fn test_tee_no_more_writeable_2() { From 675dd9404a9482c445a414c18b369a62b1aeec65 Mon Sep 17 00:00:00 2001 From: Ben Wiederhake Date: Sun, 31 Mar 2024 16:59:34 +0200 Subject: [PATCH 3/3] tee: avoid unnecessarily cloning argument list --- src/uu/tee/src/tee.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/uu/tee/src/tee.rs b/src/uu/tee/src/tee.rs index 0278cb40470..b1443dbb95b 100644 --- a/src/uu/tee/src/tee.rs +++ b/src/uu/tee/src/tee.rs @@ -148,8 +148,7 @@ fn tee(options: &Options) -> Result<()> { } let mut writers: Vec = options .files - .clone() - .into_iter() + .iter() .filter_map(|file| open(file, options.append, options.output_error.as_ref())) .collect::>>()?; let had_open_errors = writers.len() != options.files.len(); @@ -188,11 +187,11 @@ fn tee(options: &Options) -> Result<()> { /// If that error should lead to program termination, this function returns Some(Err()), /// otherwise it returns None. fn open( - name: String, + name: &str, append: bool, output_error: Option<&OutputErrorMode>, ) -> Option> { - let path = PathBuf::from(name.clone()); + let path = PathBuf::from(name); let mut options = OpenOptions::new(); let mode = if append { options.append(true) @@ -202,7 +201,7 @@ fn open( match mode.write(true).create(true).open(path.as_path()) { Ok(file) => Some(Ok(NamedWriter { inner: Box::new(file), - name, + name: name.to_owned(), })), Err(f) => { show_error!("{}: {}", name.maybe_quote(), f);