From 07231e6c6c0470678a797b156db8a2a6b8ae9d47 Mon Sep 17 00:00:00 2001 From: Jan Scheer Date: Wed, 18 May 2022 14:22:53 +0200 Subject: [PATCH] tail: fix handling of stdin redirects for macOS On macOS path.is_dir() can be false for directories if it was a redirect, e.g. ` tail < DIR` * fix some tests for macOS Cleanup: * fix clippy/spell-checker * fix build for windows by refactoring stdin_is_pipe_or_fifo() --- Cargo.lock | 4 +- src/uu/tail/src/platform/unix.rs | 1 + src/uu/tail/src/tail.rs | 86 ++++++++++++++++++++++++-------- tests/by-util/test_tail.rs | 23 +++++---- 4 files changed, 82 insertions(+), 32 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9f546acd3..413d5412b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -190,9 +190,9 @@ checksum = "14c189c53d098945499cdfa7ecc63567cf3886b3332b312a5b4585d8d3a6a610" [[package]] name = "cc" -version = "1.0.72" +version = "1.0.73" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "22a9137b95ea06864e018375b72adfb7db6e6f68cfc8df5a04d00288050485ee" +checksum = "2fff2a6927b3bb87f9595d67196a70493f627687a71d87a0d692242c33f58c11" [[package]] name = "cexpr" diff --git a/src/uu/tail/src/platform/unix.rs b/src/uu/tail/src/platform/unix.rs index 00d21a6ae..8d1af2a3d 100644 --- a/src/uu/tail/src/platform/unix.rs +++ b/src/uu/tail/src/platform/unix.rs @@ -9,6 +9,7 @@ */ // spell-checker:ignore (ToDO) errno EPERM ENOSYS +// spell-checker:ignore (options) GETFD use std::io::{stdin, Error}; diff --git a/src/uu/tail/src/tail.rs b/src/uu/tail/src/tail.rs index c491f60ce..ddbfe701a 100644 --- a/src/uu/tail/src/tail.rs +++ b/src/uu/tail/src/tail.rs @@ -7,7 +7,7 @@ // * For the full copyright and license information, please view the LICENSE // * file that was distributed with this source code. -// spell-checker:ignore (ToDO) seekable seek'd tail'ing ringbuffer ringbuf unwatch +// spell-checker:ignore (ToDO) seekable seek'd tail'ing ringbuffer ringbuf unwatch Uncategorized // spell-checker:ignore (libs) kqueue // spell-checker:ignore (acronyms) // spell-checker:ignore (env/flags) @@ -46,8 +46,6 @@ use uucore::lines::lines; use uucore::parse_size::{parse_size, ParseSizeError}; use uucore::ringbuffer::RingBuffer; -#[cfg(unix)] -use crate::platform::{stdin_is_bad_fd, stdin_is_pipe_or_fifo}; #[cfg(unix)] use std::fs::metadata; #[cfg(unix)] @@ -70,7 +68,6 @@ pub mod text { pub static STDIN_HEADER: &str = "standard input"; pub static NO_FILES_REMAINING: &str = "no files remaining"; pub static NO_SUCH_FILE: &str = "No such file or directory"; - #[cfg(unix)] pub static BAD_FD: &str = "Bad file descriptor"; #[cfg(target_os = "linux")] pub static BACKEND: &str = "inotify"; @@ -250,13 +247,11 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { // skip expansive fstat check if PRESUME_INPUT_PIPE is selected if !args.stdin_is_pipe_or_fifo { - // FIXME windows has GetFileType which can determine if the file is a pipe/FIFO - // so this check can also be performed - if cfg!(unix) { - args.stdin_is_pipe_or_fifo = stdin_is_pipe_or_fifo(); - } + args.stdin_is_pipe_or_fifo = stdin_is_pipe_or_fifo(); } + // dbg!(args.stdin_is_pipe_or_fifo); + uu_tail(args) } @@ -298,6 +293,9 @@ fn uu_tail(mut settings: Settings) -> UResult<()> { } } + let mut buf = [0; 0]; // empty buffer to check if stdin().read().is_err() + let stdin_read_possible = settings.stdin_is_pipe_or_fifo && stdin().read(&mut buf).is_ok(); + if path.is_stdin() && settings.follow == Some(FollowMode::Name) { // TODO: still needed? // Mimic GNU's tail; Exit immediately even though there might be other valid files. @@ -306,16 +304,30 @@ fn uu_tail(mut settings: Settings) -> UResult<()> { if settings.follow == Some(FollowMode::Descriptor) && settings.retry { show_warning!("--retry only effective for the initial open"); } - if !path.exists() { + if !path.exists() && !settings.stdin_is_pipe_or_fifo { settings.return_code = 1; show_error!( "cannot open {} for reading: {}", display_name.quote(), text::NO_SUCH_FILE ); - } else if path.is_dir() { + } else if path.is_dir() || display_name.is_stdin() && !stdin_read_possible { + let err_msg = "Is a directory".to_string(); + + // NOTE: On macOS path.is_dir() can be false for directories + // if it was a redirect, e.g. ` tail < DIR` + if !path.is_dir() { + // TODO: match against ErrorKind + // if unstable library feature "io_error_more" becomes stable + // if let Err(e) = stdin().read(&mut buf) { + // if e.kind() != std::io::ErrorKind::IsADirectory { + // err_msg = e.message.to_string(); + // } + // } + } + settings.return_code = 1; - show_error!("error reading {}: Is a directory", display_name.quote()); + show_error!("error reading {}: {}", display_name.quote(), err_msg); if settings.follow.is_some() { let msg = if !settings.retry { "; giving up on this name" @@ -333,14 +345,25 @@ fn uu_tail(mut settings: Settings) -> UResult<()> { continue; } } else { - // TODO: [2021-10; jhscheer] how to handle block device, socket, fifo? + // TODO: [2021-10; jhscheer] how to handle block device or socket? + // dbg!(path.is_tailable()); + // dbg!(path.is_stdin()); + // dbg!(path.is_dir()); + // dbg!(path.exists()); + // if let Ok(md) = path.metadata() { + // let ft = md.file_type(); + // dbg!(ft.is_fifo()); + // dbg!(ft.is_socket()); + // dbg!(ft.is_block_device()); + // dbg!(ft.is_char_device()); + // } todo!(); } } let md = path.metadata().ok(); - if display_name.is_stdin() { + if display_name.is_stdin() && path.is_tailable() { if settings.verbose { if !first_header { println!(); @@ -725,10 +748,10 @@ fn follow(files: &mut FileHandling, settings: &mut Settings) -> UResult<()> { paths.push(path.to_path_buf()); } } - for path in paths.iter_mut() { + for path in &mut paths { if let Ok(new_md) = path.metadata() { if let Some(old_md) = &files.map.get(path).unwrap().metadata { - // TODO: [2021-10; jhscheer] reduce dublicate code + // TODO: [2021-10; jhscheer] reduce duplicate code let display_name = files.map.get(path).unwrap().display_name.to_path_buf(); if new_md.len() <= old_md.len() && new_md.modified().unwrap() != old_md.modified().unwrap() @@ -1407,6 +1430,26 @@ fn get_block_size(md: &Metadata) -> u64 { } } +pub fn stdin_is_pipe_or_fifo() -> bool { + #[cfg(unix)] + { + platform::stdin_is_pipe_or_fifo() + } + // FIXME windows has GetFileType which can determine if the file is a pipe/FIFO + // so this check can also be performed + #[cfg(not(unix))] + false +} + +pub fn stdin_is_bad_fd() -> bool { + #[cfg(unix)] + { + platform::stdin_is_bad_fd() + } + #[cfg(not(unix))] + false +} + trait PathExt { fn is_stdin(&self) -> bool; fn print_header(&self); @@ -1416,9 +1459,9 @@ trait PathExt { impl PathExt for Path { fn is_stdin(&self) -> bool { - self.eq(Path::new(text::DASH)) - || self.eq(Path::new(text::DEV_STDIN)) - || self.eq(Path::new(text::STDIN_HEADER)) + self.eq(Self::new(text::DASH)) + || self.eq(Self::new(text::DEV_STDIN)) + || self.eq(Self::new(text::STDIN_HEADER)) } fn print_header(&self) { println!("==> {} <==", self.display()); @@ -1431,7 +1474,10 @@ impl PathExt for Path { { // TODO: [2021-10; jhscheer] what about fifos? self.is_file() - || (self.exists() && metadata(self).unwrap().file_type().is_char_device()) + || self.exists() && { + let ft = metadata(self).unwrap().file_type(); + ft.is_char_device() || ft.is_fifo() + } } #[cfg(not(unix))] { diff --git a/tests/by-util/test_tail.rs b/tests/by-util/test_tail.rs index abeb7878d..695b15f99 100644 --- a/tests/by-util/test_tail.rs +++ b/tests/by-util/test_tail.rs @@ -3,7 +3,7 @@ // * For the full copyright and license information, please view the LICENSE // * file that was distributed with this source code. -// spell-checker:ignore (ToDO) abcdefghijklmnopqrstuvwxyz efghijklmnopqrstuvwxyz vwxyz emptyfile logfile bogusfile siette ocho nueve diez +// spell-checker:ignore (ToDO) abcdefghijklmnopqrstuvwxyz efghijklmnopqrstuvwxyz vwxyz emptyfile logfile file siette ocho nueve diez // spell-checker:ignore (libs) kqueue // spell-checker:ignore (jargon) tailable untailable @@ -89,7 +89,6 @@ fn test_stdin_redirect_file() { p.kill().unwrap(); let (buf_stdout, buf_stderr) = take_stdout_stderr(&mut p); - dbg!(&buf_stdout); assert!(buf_stdout.eq("foo")); assert!(buf_stderr.is_empty()); } @@ -200,7 +199,6 @@ fn test_follow_stdin_explicit_indefinitely() { p.kill().unwrap(); let (buf_stdout, buf_stderr) = take_stdout_stderr(&mut p); - dbg!(&buf_stdout, &buf_stderr); assert!(buf_stdout.eq("==> standard input <==")); assert!(buf_stderr.eq("tail: warning: following standard input indefinitely is ineffective")); @@ -495,9 +493,9 @@ fn test_bytes_single() { #[test] fn test_bytes_stdin() { new_ucmd!() + .pipe_in_fixture(FOOBAR_TXT) .arg("-c") .arg("13") - .pipe_in_fixture(FOOBAR_TXT) .run() .stdout_is_fixture("foobar_bytes_stdin.expected") .no_stderr(); @@ -945,7 +943,12 @@ fn test_retry2() { let ts = TestScenario::new(util_name!()); let missing = "missing"; - let result = ts.ucmd().arg(missing).arg("--retry").run(); + let result = ts + .ucmd() + .set_stdin(Stdio::null()) + .arg(missing) + .arg("--retry") + .run(); result .stderr_is( "tail: warning: --retry ignored; --retry is useful only when following\n\ @@ -1300,7 +1303,6 @@ fn test_retry9() { p.kill().unwrap(); let (buf_stdout, buf_stderr) = take_stdout_stderr(&mut p); - // println!("stdout:\n{}\nstderr:\n{}", buf_stdout, buf_stderr); // dbg assert_eq!(buf_stdout, expected_stdout); assert_eq!(buf_stderr, expected_stderr); } @@ -1672,10 +1674,12 @@ fn take_stdout_stderr(p: &mut std::process::Child) -> (String, String) { #[test] fn test_no_such_file() { new_ucmd!() - .arg("bogusfile") + .set_stdin(Stdio::null()) + .arg("missing") .fails() + .stderr_is("tail: cannot open 'missing' for reading: No such file or directory") .no_stdout() - .stderr_contains("cannot open 'bogusfile' for reading: No such file or directory"); + .code_is(1); } #[test] @@ -1708,8 +1712,7 @@ fn test_presume_input_pipe_default() { } #[test] -#[cfg(target_os = "linux")] -#[cfg(disable_until_fixed)] +#[cfg(unix)] fn test_fifo() { let ts = TestScenario::new(util_name!()); let at = &ts.fixtures;