tail: Big refactoring of uutail, tail_file, tail_stdin methods, Input…

… and error handling

* Replace Input::resolve with Input::open
* Platform independent way to determine and print a fifo with file seek optimizations
* Fifos are now printed only once and behave like pipes in regard to a Stdin handle
* Remove the need to canonicalize the path for Input to determine if it's a fifo or pipe. This
  didn't work on all platforms (equally), most notatibly on macos and windows.
* Remove the need to store the offset for fifos.
* Activate the previously failing fifo tests and enhance the TestRunner
* Error and Result handling in one designated place
* Concise code
* Remove the last unhandled unwraps in tail.rs

src/uu/tail/src/chunks.rs

@@ -46,21 +46,21 @@ pub struct ReverseChunks<'a> {
 
 impl<'a> ReverseChunks<'a> {
  // TODO: Return io::Result<...> ?
- pub fn new(file: &'a mut File) -> ReverseChunks<'a> {
+ pub fn new(file: &'a mut File) -> io::Result<ReverseChunks<'a>> {
  let current = if cfg!(unix) {
- file.seek(SeekFrom::Current(0)).unwrap()
+ file.seek(SeekFrom::Current(0))?
  } else {
  0
  };
- let size = file.seek(SeekFrom::End(0)).unwrap() - current;
+ let size = file.seek(SeekFrom::End(0))? - current;
  let max_blocks_to_read = (size as f64 / BLOCK_SIZE as f64).ceil() as usize;
  let block_idx = 0;
- ReverseChunks {
+ Ok(ReverseChunks {
  file,
  size,
  max_blocks_to_read,
  block_idx,
- }
+ })
  }
 }
 

src/uu/tail/src/follow/files.rs

@@ -175,6 +175,7 @@ impl FileHandling {
 
 /// Data structure to keep a handle on the BufReader, Metadata
 /// and the display_name (header_name) of files that are being followed.
+/// FIXME: store File instead of a Buffered Reader
 pub struct PathData {
  pub reader: Option<Box<dyn BufRead>>,
  pub metadata: Option<Metadata>,

src/uu/tail/src/paths.rs

@@ -6,12 +6,13 @@
 // spell-checker:ignore tailable seekable stdlib (stdlib)
 
 use crate::text;
+use same_file::Handle;
 use std::fs::{File, Metadata};
-use std::io::{Seek, SeekFrom};
+use std::io::{self, Seek, SeekFrom};
 #[cfg(unix)]
 use std::os::unix::fs::{FileTypeExt, MetadataExt};
 use std::path::{Path, PathBuf};
-use uucore::error::{UIoError, UResult};
+use uucore::error::UResult;
 
 #[derive(Debug, Clone)]
 pub enum InputKind {
@@ -25,11 +26,12 @@ pub struct Input {
  pub display_name: String,
 }
 
+// TODO: return from input.open()
+#[allow(dead_code)]
 pub enum Resolved {
- CanonicalPath(PathBuf),
- Fifo(PathBuf),
+ File(File),
+ Fifo(File),
  Pipe,
- Error(UIoError),
 }
 
 impl Input {
@@ -59,59 +61,35 @@ impl Input {
  }
  }
 
- /// FreeBSD, Macos: redirected directory `tail < dir` => Pipe
- /// (canonicalizing to /dev/fd/0).
- /// Linux: `tail < dir` => Fifo(path_to_dir)
- #[cfg(all(unix, not(target_os = "macos")))]
- pub fn resolve(&self) -> Resolved {
+ pub fn open(&self) -> io::Result<Option<File>> {
  match &self.kind {
- InputKind::File(path) if path != &PathBuf::from(text::DEV_STDIN) => {
- match path.canonicalize() {
- Ok(path) => Resolved::CanonicalPath(path),
- Err(error) => Resolved::Error(UIoError::from(error)),
- }
- }
- InputKind::File(_) | InputKind::Stdin => {
- match PathBuf::from(text::DEV_STDIN).canonicalize() {
- Ok(path) if path != PathBuf::from(text::FD0) => Resolved::Fifo(path),
- Ok(_) | Err(_) => Resolved::Pipe,
+ InputKind::File(path) => Ok(Some(File::open(path)?)),
+ InputKind::Stdin => {
+ let mut handle = Handle::stdin()?;
+ let file = handle.as_file_mut();
+ if file.is_seekable() {
+ // TODO: return Ok(None) if try_clone fails
+ Ok(Some(file.try_clone()?))
+ } else {
+ Ok(None)
  }
  }
  }
  }
 
- #[cfg(target_os = "macos")]
- pub fn resolve(&self) -> Resolved {
- use same_file::Handle;
+ #[cfg(unix)]
+ pub fn path(&self) -> PathBuf {
  match &self.kind {
- InputKind::File(path) if path != &PathBuf::from(text::DEV_STDIN) => {
- match path.canonicalize() {
- Ok(path) => Resolved::CanonicalPath(path),
- Err(error) => Resolved::Error(UIoError::from(error)),
- }
- }
- InputKind::File(_) | InputKind::Stdin => match Handle::stdin() {
- Ok(mut handle) => match handle.as_file_mut().is_seekable(0) {
- true => Resolved::Fifo(PathBuf::from(text::DEV_STDIN)),
- false => Resolved::Pipe,
- },
- Err(error) => Resolved::Error(UIoError::from(error)),
- },
+ InputKind::File(path) => path.to_owned(),
+ InputKind::Stdin => PathBuf::from(text::DEV_STDIN),
  }
  }
 
  #[cfg(windows)]
- pub fn resolve(&self) -> Resolved {
+ pub fn path(&self) -> PathBuf {
  match &self.kind {
- InputKind::File(path) => match path.canonicalize() {
- Ok(path) => Resolved::CanonicalPath(path),
- Err(error) => Resolved::Error(UIoError::from(error)),
- },
- // We treat all stdin on windows as pipe (use unbounded_tail)
- // Someone experienced with windows may find a way to resolve a
- // named pipe to a path and then return Fifo(path) so we can use the
- // file seek optimizations on fifo stdin.
- InputKind::Stdin => Resolved::Pipe,
+ InputKind::File(path) => path.to_owned(),
+ InputKind::Stdin => todo!(),
  }
  }
 }
@@ -156,16 +134,13 @@ impl HeaderPrinter {
 }
 pub trait FileExtTail {
  #[allow(clippy::wrong_self_convention)]
- fn is_seekable(&mut self, current_offset: u64) -> bool;
+ fn is_seekable(&mut self) -> bool;
 }
 
 impl FileExtTail for File {
  /// Test if File is seekable.
- /// Set the current position offset to `current_offset`.
- fn is_seekable(&mut self, current_offset: u64) -> bool {
+ fn is_seekable(&mut self) -> bool {
  self.seek(SeekFrom::Current(0)).is_ok()
- && self.seek(SeekFrom::End(0)).is_ok()
- && self.seek(SeekFrom::Start(current_offset)).is_ok()
  }
 }
 
@@ -181,7 +156,7 @@ impl MetadataExtTail for Metadata {
  let ft = self.file_type();
  #[cfg(unix)]
  {
- ft.is_file() || ft.is_char_device() || ft.is_fifo()
+ ft.is_file() || ft.is_char_device() || ft.is_fifo() || ft.is_block_device()
  }
  #[cfg(not(unix))]
  {

src/uu/tail/src/tail.rs

@@ -35,14 +35,12 @@ pub mod text;
 pub use args::uu_app;
 use args::{parse_args, FilterMode, Settings, Signum};
 use chunks::ReverseChunks;
-use error::{IOErrorHandler, IOErrorHandlerResult};
+use error::IOErrorHandler;
 use follow::Observer;
-use paths::{FileExtTail, HeaderPrinter, Input, InputKind, MetadataExtTail, Resolved};
-use same_file::Handle;
+use paths::{FileExtTail, HeaderPrinter, Input, MetadataExtTail};
 use std::cmp::Ordering;
 use std::fs::File;
 use std::io::{self, stdin, stdout, BufRead, BufReader, BufWriter, Read, Seek, SeekFrom, Write};
-use std::path::{Path, PathBuf};
 use uucore::display::Quotable;
 use uucore::error::{UResult, USimpleError};
 
@@ -74,13 +72,37 @@ fn uu_tail(settings: &Settings, mut observer: Observer) -> UResult<()> {
  // Do an initial tail print of each path's content.
  // Add `path` and `reader` to `files` map if `--follow` is selected.
  for input in &settings.inputs {
- match input.kind() {
- InputKind::File(path) if cfg!(not(unix)) || path != &PathBuf::from(text::DEV_STDIN) => {
- tail_file(settings, &mut printer, input, path, &mut observer, 0)?;
+ let result = match input.open() {
+ Ok(Some(file)) => tail_file(settings, &mut printer, input, file),
+ Ok(None) => tail_stdin(settings, &mut printer, input),
+ Err(error) => Err(error),
+ };
+ match result {
+ Ok(reader) if input.is_stdin() => {
+ observer.register_stdin(input.display_name.as_str(), Some(reader), true)?;
  }
- // File points to /dev/stdin here
- InputKind::File(_) | InputKind::Stdin => {
- tail_stdin(settings, &mut printer, input, &mut observer)?;
+ Ok(reader) => {
+ observer.register_file(
+ input.path().as_path(),
+ input.display_name.as_str(),
+ Some(reader),
+ true,
+ )?;
+ }
+ Err(error) => {
+ let mut handler = IOErrorHandler::from(input.clone(), &observer);
+ let path = input.path();
+ match path.metadata() {
+ // TODO: register directory as stdin if input is stdin ??
+ Ok(meta) if meta.is_dir() || IOErrorHandler::is_directory_error(&error) => {
+ observer.register_directory(&path, input.display_name.as_str(), false)?;
+ handler.handle_directory();
+ }
+ Ok(_) | Err(_) => {
+ observer.register_bad_file(&path, input.display_name.as_str(), false)?;
+ handler.handle(error);
+ }
+ }
  }
  }
  }
@@ -92,130 +114,35 @@ fn tail_file(
  settings: &Settings,
  header_printer: &mut HeaderPrinter,
  input: &Input,
- path: &Path,
- observer: &mut Observer,
- offset: u64,
-) -> UResult<()> {
- if input.is_stdin() && cfg!(target_os = "macos") {
- dbg!(settings);
- dbg!(input);
- dbg!(path);
- dbg!(offset);
- let result = path.metadata();
- dbg!(&result);
- }
- match File::open(path) {
- // On some OS other than linux the check for path.is_dir may return
- // false if it was a redirect: $ tail < dir, so we have to do some
- // additional error handling later
- Ok(mut file) if !path.is_dir() => {
- header_printer.print_input(input);
-
- // we were able to open the file, so it's safe to use unwrap here
- let metadata = path.metadata().unwrap();
- let mut reader;
- let result = if !settings.presume_input_pipe
- && file.is_seekable(if input.is_stdin() { offset } else { 0 })
- && metadata.get_block_size() > 0
- {
- let result = bounded_tail(&mut file, settings);
- reader = BufReader::new(file);
- result
- } else {
- reader = BufReader::new(file);
- unbounded_tail(&mut reader, settings)
- };
-
- match result {
- Ok(_) => observer.register_file(
- path,
- input.display_name.as_str(),
- Some(Box::new(reader)),
- true,
- )?,
- Err(error) => {
- // TODO: observer.register_bad_file() in other cases ?
- if let IOErrorHandlerResult::IsADirectory =
- IOErrorHandler::from(input.clone(), observer).handle(error)
- {
- // FIXME: This is a redirected directory. Register as stdin.
- observer.register_directory(path, input.display_name.as_str(), false)?;
- }
- }
- }
- }
- Ok(_) | Err(_) if path.is_dir() => {
- header_printer.print_input(input);
-
- let mut handler = IOErrorHandler::from(input.clone(), observer);
- handler.handle_directory();
-
- observer.register_directory(path, input.display_name.as_str(), false)?;
- }
- Err(error) => {
- observer.register_bad_file(path, input.display_name.as_str(), false)?;
-
- let mut handler = IOErrorHandler::from(input.clone(), observer);
- handler.handle(error);
- }
- _ => unreachable!(),
- }
+ mut file: File,
+) -> io::Result<Box<dyn BufRead>> {
+ header_printer.print_input(input);
+
+ let meta = input.path().metadata()?;
+ let reader = if !settings.presume_input_pipe && file.is_seekable() && meta.get_block_size() > 0
+ {
+ bounded_tail(&mut file, settings)?;
+ BufReader::new(file)
+ } else {
+ let mut reader = BufReader::new(file);
+ unbounded_tail(&mut reader, settings)?;
+ reader
+ };
 
- Ok(())
+ Ok(Box::new(reader))
 }
 
 fn tail_stdin(
  settings: &Settings,
  header_printer: &mut HeaderPrinter,
  input: &Input,
- observer: &mut Observer,
-) -> UResult<()> {
- if cfg!(target_os = "macos") {
- dbg!(settings);
- dbg!(input);
- }
- match input.resolve() {
- Resolved::Fifo(path) => {
- let mut stdin_offset = 0;
- if cfg!(unix) {
- // Save the current seek position/offset of a stdin redirected file.
- // This is needed to pass "gnu/tests/tail-2/start-middle.sh"
- if let Ok(mut stdin_handle) = Handle::stdin() {
- if let Ok(offset) = stdin_handle.as_file_mut().seek(SeekFrom::Current(0)) {
- stdin_offset = offset;
- }
- }
- }
- tail_file(
- settings,
- header_printer,
- input,
- &path,
- observer,
- stdin_offset,
- )?;
- }
- Resolved::Pipe => {
- header_printer.print_input(input);
- let mut reader = BufReader::new(stdin());
+) -> io::Result<Box<dyn BufRead>> {
+ header_printer.print_input(input);
 
- if let Err(error) = unbounded_tail(&mut reader, settings) {
- let mut handler = IOErrorHandler::from(input.clone(), observer);
- handler.handle(error);
+ let mut reader = BufReader::new(stdin());
+ unbounded_tail(&mut reader, settings).map(|_| ())?;
 
- // TODO: add bad stdin to observer??
- return Ok(());
- }
-
- observer.register_stdin(input.display_name.as_str(), Some(Box::new(reader)), true)?;
- }
- Resolved::CanonicalPath(_) => unreachable!(),
- Resolved::Error(error) => {
- dbg!(&error);
- }
- };
-
- Ok(())
+ Ok(Box::new(reader))
 }
 
 /// Find the index after the given number of instances of a given byte.
@@ -303,7 +230,7 @@ fn backwards_thru_file(file: &mut File, num_delimiters: u64, delimiter: u8) -> i
  // so far (reading from the end of the file toward the beginning).
  let mut counter = 0;
 
- for (block_idx, slice) in ReverseChunks::new(file).enumerate() {
+ for (block_idx, slice) in ReverseChunks::new(file)?.enumerate() {
  let slice = slice?;
 
  // Iterate over each byte in the slice in reverse order.