From e0efd6cc905361b9fa597fb189fd2e6c7116731f Mon Sep 17 00:00:00 2001 From: Jan Scheer Date: Thu, 2 Jun 2022 17:22:57 +0200 Subject: [PATCH] tail: update readme * add clippy fixes * add cicd fixes --- src/uu/tail/README.md | 104 +++++++++++++++++-------------------- src/uu/tail/src/parse.rs | 2 +- src/uu/tail/src/tail.rs | 10 ++-- tests/by-util/test_tail.rs | 52 +++++++++++-------- 4 files changed, 86 insertions(+), 82 deletions(-) diff --git a/src/uu/tail/README.md b/src/uu/tail/README.md index 2dbff4b164f..d3227c96173 100644 --- a/src/uu/tail/README.md +++ b/src/uu/tail/README.md @@ -1,91 +1,83 @@ - + # Notes / ToDO ## Missing features * `--max-unchanged-stats` -* The current implementation doesn't follow stdin on non-unix platforms * check whether process p is alive at least every number of seconds (relevant for `--pid`) Note: -There's a stub for `--max-unchanged-stats` so GNU tests using it can run, however this flag has no functionality yet. +There's a stub for `--max-unchanged-stats` so GNU test-suite checks using it can run, however this flag has no functionality yet. -### Platform support for `--follow` +### Platform support for `--follow` and `--retry` The `--follow=descriptor`, `--follow=name` and `--retry` flags have very good support on Linux (inotify backend). -They work good enough on macOS/BSD (kqueue backend) with some tests failing due to differences how kqueue works compared to inotify. -Windows support is there in theory due to support by the notify-crate, however it's completely untested. - -### Flags with features - -- [x] fast poll := `-s.1 --max-unchanged-stats=1` - - [x] sub-second sleep interval e.g. `-s.1` - - [ ] `--max-unchanged-stats` (only meaningful with `--follow=name` `---disable-inotify`) -- [x] `--follow=name` -- [x] `--retry` -- [x] `-F` (same as `--follow=name` `--retry`) -- [x] `---disable-inotify` (three hyphens is correct) -- [x] `---presume-input-pipe` (three hyphens is correct) +They work good enough on macOS/BSD (kqueue backend) with some tests failing due to differences of how kqueue works compared to inotify. +Windows support is there in theory due to ReadDirectoryChanges support by the notify-crate, however these flags are completely untested on Windows. Note: -`---disable-inotify` means to use polling instead of inotify, -however inotify is a Linux only backend and polling is now supported also for the other backends. +The undocumented `---disable-inotify` flag is used to disable the inotify backend to test polling. +However inotify is a Linux only backend and polling is now supported also for the other backends. Because of this, `disable-inotify` is now an alias to the new and more versatile flag name: `--use-polling`. ## Possible optimizations * Don't read the whole file if not using `-f` and input is regular file. Read in chunks from the end going backwards, reading each individual chunk forward. * Reduce number of system calls to e.g. `fstat` +* Improve resource management by adding more system calls to `inotify_rm_watch` when appropriate. -# GNU tests results +# GNU test-suite results The functionality for the test "gnu/tests/tail-2/follow-stdin.sh" is implemented. It fails because it is provoking closing a file descriptor with `tail -f <&-` and as part of a workaround, Rust's stdlib reopens closed FDs as `/dev/null` which means uu_tail cannot detect this. See also, e.g. the discussion at: https://github.com/uutils/coreutils/issues/2873 -The functionality for the test "gnu/tests/tail-2/inotify-rotate-resourced.sh" is implemented. -It fails with an error because it is using `strace` to look for calls to 'inotify_add_watch' and 'inotify_rm_watch', -however in uu_tail these system calls are invoked from a seperate thread. If the GNU test would use `strace -f` this issue could be resolved. +The functionality for the test "gnu/tests/tail-2/inotify-rotate-resources.sh" is implemented. +It fails with an error because it is using `strace` to look for calls to `inotify_add_watch` and `inotify_rm_watch`, +however in uu_tail these system calls are invoked from a separate thread. +If the GNU test would follow threads, i.e. use `strace -f`, this issue could be resolved. ## Testsuite summary for GNU coreutils 9.1.8-e08752: ### PASS: -tail-2/F-headers.sh -tail-2/F-vs-missing.sh -tail-2/append-only.sh # skipped test: must be run as root -tail-2/assert-2.sh -tail-2/assert.sh -tail-2/big-4gb.sh -tail-2/descriptor-vs-rename.sh -tail-2/end-of-device.sh # skipped test: must be run as root -tail-2/flush-initial.sh -tail-2/follow-name.sh -tail-2/inotify-dir-recreate.sh -tail-2/inotify-hash-abuse.sh -tail-2/inotify-hash-abuse2.sh -tail-2/inotify-only-regular.sh -tail-2/inotify-rotate.sh -tail-2/overlay-headers.sh -tail-2/pid.sh -tail-2/pipe-f2.sh -tail-2/proc-ksyms.sh -tail-2/start-middle.sh -tail-2/tail-c.sh -tail-2/tail-n0f.sh -tail-2/truncate.sh +- [x] `tail-2/F-headers.sh` +- [x] `tail-2/F-vs-missing.sh` +- [x] `tail-2/F-vs-rename.sh` +- [x] `tail-2/append-only.sh # skipped test: must be run as root` +- [x] `tail-2/assert-2.sh` +- [x] `tail-2/assert.sh` +- [x] `tail-2/big-4gb.sh` +- [x] `tail-2/descriptor-vs-rename.sh` +- [x] `tail-2/end-of-device.sh # skipped test: must be run as root` +- [x] `tail-2/flush-initial.sh` +- [x] `tail-2/follow-name.sh` +- [x] `tail-2/inotify-dir-recreate.sh` +- [x] `tail-2/inotify-hash-abuse.sh` +- [x] `tail-2/inotify-hash-abuse2.sh` +- [x] `tail-2/inotify-only-regular.sh` +- [x] `tail-2/inotify-rotate.sh` +- [x] `tail-2/overlay-headers.sh` +- [x] `tail-2/pid.sh` +- [x] `tail-2/pipe-f2.sh` +- [x] `tail-2/proc-ksyms.sh` +- [x] `tail-2/retry.sh` +- [x] `tail-2/start-middle.sh` +- [x] `tail-2/tail-c.sh` +- [x] `tail-2/tail-n0f.sh` +- [x] `tail-2/truncate.sh` + ### SKIP: -tail-2/inotify-race.sh # skipped test: breakpoint not hit -tail-2/inotify-race2.sh # skipped test: breakpoint not hit -tail-2/pipe-f.sh # skipped test: trapping SIGPIPE is not supported +- [ ] `tail-2/inotify-race.sh # skipped test: breakpoint not hit` +- [ ] `tail-2/inotify-race2.sh # skipped test: breakpoint not hit` +- [ ] `tail-2/pipe-f.sh # skipped test: trapping SIGPIPE is not supported` ### FAIL: -misc/tail.pl -tail-2/F-vs-rename.sh -tail-2/follow-stdin.sh -tail-2/retry.sh -tail-2/symlink.sh -tail-2/wait.sh +- [ ] `misc/tail.pl` +- [ ] `tail-2/follow-stdin.sh` +- [ ] `tail-2/symlink.sh` +- [ ] `tail-2/wait.sh` + ### ERROR: -tail-2/inotify-rotate-resources.sh +- [ ] `tail-2/inotify-rotate-resources.sh` diff --git a/src/uu/tail/src/parse.rs b/src/uu/tail/src/parse.rs index d524adbc107..7511f2405e4 100644 --- a/src/uu/tail/src/parse.rs +++ b/src/uu/tail/src/parse.rs @@ -5,7 +5,7 @@ use std::ffi::OsString; -#[derive(PartialEq, Debug)] +#[derive(PartialEq, Eq, Debug)] pub enum ParseError { Syntax, Overflow, diff --git a/src/uu/tail/src/tail.rs b/src/uu/tail/src/tail.rs index df0dff349d1..daa0c09d5f8 100644 --- a/src/uu/tail/src/tail.rs +++ b/src/uu/tail/src/tail.rs @@ -605,7 +605,7 @@ pub fn uu_app<'a>() -> Command<'a> { ) .arg( Arg::new(options::USE_POLLING) - .visible_alias(options::DISABLE_INOTIFY_TERM) // NOTE: Used by GNU's test suite + .alias(options::DISABLE_INOTIFY_TERM) // NOTE: Used by GNU's test suite .alias("dis") // NOTE: Used by GNU's test suite .long(options::USE_POLLING) .help(POLLING_HELP), @@ -761,6 +761,7 @@ fn follow(files: &mut FileHandling, settings: &mut Settings) -> UResult<()> { // e.g. `echo "X1" > missing ; sleep 0.1 ; echo "X" > missing ;` should trigger a // truncation event, but PollWatcher doesn't recognize it. // This is relevant to pass, e.g.: "gnu/tests/tail-2/truncate.sh" + // TODO: [2022-06; jhscheer] maybe use `--max-unchanged-stats` here to reduce fstat calls? if settings.use_polling && settings.follow.is_some() { for path in &files .map @@ -1059,9 +1060,8 @@ impl FileHandling { /// Return true if there is only stdin remaining fn only_stdin_remaining(&self) -> bool { - self.map.len() == 1 - && (self.map.contains_key(Path::new(text::DASH))) - // || self.map.contains_key(Path::new(text::DEV_STDIN))) // TODO: still needed? + self.map.len() == 1 && (self.map.contains_key(Path::new(text::DASH))) + // || self.map.contains_key(Path::new(text::DEV_STDIN))) // TODO: still needed? } /// Return true if there is at least one "tailable" path (or stdin) remaining @@ -1436,6 +1436,8 @@ pub fn stdin_is_bad_fd() -> bool { } trait FileExtTail { + // clippy complains, but it looks like a false positive + #[allow(clippy::wrong_self_convention)] fn is_seekable(&mut self) -> bool; } diff --git a/tests/by-util/test_tail.rs b/tests/by-util/test_tail.rs index 234a05981fa..1b792ac7374 100644 --- a/tests/by-util/test_tail.rs +++ b/tests/by-util/test_tail.rs @@ -195,7 +195,7 @@ fn test_permission_denied_multiple() { fn test_follow_redirect_stdin_name_retry() { // $ touch f && tail -F - < f // tail: cannot follow '-' by name - // NOTE: Note sure why GNU's tail doesn't just follow `f` in this case. + // NOTE: Not sure why GNU's tail doesn't just follow `f` in this case. let ts = TestScenario::new(util_name!()); let at = &ts.fixtures; @@ -1276,9 +1276,6 @@ fn test_retry7() { let at = &ts.fixtures; let untailable = "untailable"; - // tail: 'untailable' has appeared; following new file\n\ - // tail: 'untailable' has become inaccessible: No such file or directory\n\ - // tail: 'untailable' has appeared; following new file\n"; let expected_stderr = "tail: error reading 'untailable': Is a directory\n\ tail: untailable: cannot follow end of this type of file\n\ tail: 'untailable' has become accessible\n\ @@ -1336,7 +1333,7 @@ fn test_retry7() { } #[test] -#[cfg(all(unix, not(target_os = "android")))] // FIXME: fix this test for Android +#[cfg(all(unix, not(any(target_os = "android", target_vendor = "apple"))))] // FIXME: make this work not just on Linux fn test_retry8() { // Ensure that inotify will switch to polling mode if directory // of the watched file was initially missing and later created. @@ -1466,6 +1463,7 @@ fn test_retry9() { sleep(Duration::from_millis(delay)); p.kill().unwrap(); + sleep(Duration::from_millis(delay)); let (buf_stdout, buf_stderr) = take_stdout_stderr(&mut p); assert_eq!(buf_stdout, expected_stdout); @@ -1576,7 +1574,7 @@ fn test_follow_descriptor_vs_rename2() { } #[test] -#[cfg(unix)] +#[cfg(all(unix, not(target_os = "android")))] // FIXME: make this work not just on Linux fn test_follow_name_remove() { // This test triggers a remove event while `tail --follow=name file` is running. // ((sleep 2 && rm file &)>/dev/null 2>&1 &) ; tail --follow=name file @@ -1696,7 +1694,8 @@ fn test_follow_name_truncate2() { #[test] #[cfg(target_os = "linux")] // FIXME: fix this test for BSD/macOS fn test_follow_name_truncate3() { - // Opening an empty file in truncate mode should not trigger a truncate event while. + // Opening an empty file in truncate mode should not trigger a truncate event while + // `tail --follow=name file` is running. // $ rm -f file && touch file // $ ((sleep 1 && echo -n "x\n" > file &)>/dev/null 2>&1 &) ; tail --follow=name file @@ -1724,6 +1723,7 @@ fn test_follow_name_truncate3() { } #[test] +#[cfg(unix)] fn test_follow_name_truncate4() { // Truncating a file with the same content it already has should not trigger a truncate event @@ -1755,6 +1755,7 @@ fn test_follow_name_truncate4() { } #[test] +#[cfg(unix)] fn test_follow_truncate_fast() { // inspired by: "gnu/tests/tail-2/truncate.sh" // Ensure all logs are output upon file truncation @@ -1765,12 +1766,7 @@ fn test_follow_truncate_fast() { let ts = TestScenario::new(util_name!()); let at = &ts.fixtures; - let mut args = vec![ - "-s.1", - "--max-unchanged-stats=1", - "f", - "---disable-inotify", - ]; + let mut args = vec!["-s.1", "--max-unchanged-stats=1", "f", "---disable-inotify"]; let follow = vec!["-f", "-F"]; let delay = 100; @@ -1847,6 +1843,7 @@ fn test_follow_name_move_create() { } #[test] +#[cfg(all(unix, not(any(target_os = "android", target_vendor = "apple"))))] // FIXME: make this work not just on Linux fn test_follow_name_move_create2() { // inspired by: "gnu/tests/tail-2/inotify-hash-abuse.sh" // Exercise an abort-inducing flaw in inotify-enabled tail -F @@ -1859,12 +1856,22 @@ fn test_follow_name_move_create2() { } let mut args = vec![ - "-s.1", "--max-unchanged-stats=1", - "-q", "-F", - "1", "2", "3", "4", "5", "6", "7", "8", "9", + "-s.1", + "--max-unchanged-stats=1", + "-q", + "-F", + "1", + "2", + "3", + "4", + "5", + "6", + "7", + "8", + "9", ]; - let delay = 100; + let delay = 300; for _ in 0..2 { let mut p = ts.ucmd().set_stdin(Stdio::null()).args(&args).run_no_wait(); sleep(Duration::from_millis(100)); @@ -1882,11 +1889,14 @@ fn test_follow_name_move_create2() { sleep(Duration::from_millis(delay)); let (buf_stdout, buf_stderr) = take_stdout_stderr(&mut p); - assert_eq!(buf_stderr, "tail: '1' has become inaccessible: No such file or directory\n\ - tail: '1' has appeared; following new file\n"); + assert_eq!( + buf_stderr, + "tail: '1' has become inaccessible: No such file or directory\n\ + tail: '1' has appeared; following new file\n" + ); - // NOTE: Because "gnu/tests/tail-2/inotify-hash-abuse.sh" forgets to clear the files used - // during the first loop iteration, we also won't clear them to get the same side-effects. + // NOTE: Because "gnu/tests/tail-2/inotify-hash-abuse.sh" 'forgets' to clear the files used + // during the first loop iteration, we also don't clear them to get the same side-effects. // Side-effects are truncating a file with the same content, see: test_follow_name_truncate4 // at.remove("1"); // at.touch("1");