Merge pull request #2695 from jhscheer/tail_notify

`tail` overhaul (--follow=name, etc.)

README.md

@@ -422,10 +422,10 @@ See https://github.com/uutils/coreutils/issues/3336 for the main meta bugs
 | comm      | sort      |        |
 | csplit    | split     |        |
 | cut       | tac       |        |
-| dircolors | tail      |        |
-| dirname   | test      |        |
-| du        | dir       |        |
-| echo      | vdir      |        |
+| dircolors | test      |        |
+| dirname   | dir       |        |
+| du        | vdir      |        |
+| echo      |           |        |
 | env       |           |        |
 | expand    |           |        |
 | factor    |           |        |
@@ -478,6 +478,7 @@ See https://github.com/uutils/coreutils/issues/3336 for the main meta bugs
 | stdbuf    |           |        |
 | sum       |           |        |
 | sync      |           |        |
+| tail      |           |        |
 | tee       |           |        |
 | timeout   |           |        |
 | touch     |           |        |

src/uu/tail/Cargo.toml

@@ -1,3 +1,4 @@
+# spell-checker:ignore (libs) kqueue
 [package]
 name = "uu_tail"
 version = "0.0.14"
@@ -17,10 +18,12 @@ path = "src/tail.rs"
 [dependencies]
 clap = { version = "3.1", features = ["wrap_help", "cargo"] }
 libc = "0.2.126"
+notify = { version = "5.0.0-pre.15", features=["macos_kqueue"]}
 uucore = { version=">=0.0.11", package="uucore", path="../../uucore", features=["ringbuffer", "lines"] }
 
 [target.'cfg(windows)'.dependencies]
 winapi = { version="0.3", features=["fileapi", "handleapi", "processthreadsapi", "synchapi", "winbase"] }
+winapi-util = { version="0.1.5" }
 
 [target.'cfg(unix)'.dependencies]
 nix = { version = "0.24.1", features = ["fs"] }

src/uu/tail/README.md

@@ -1,18 +1,47 @@
-# Notes / ToDO
+<!-- spell-checker:ignore markdownlint ; (misc) backends kqueue Testsuite ksyms stdlib -->
 
-- Rudimentary tail implementation.
+# Notes / ToDO
 
 ## Missing features
 
-### Flags with features
+* `--max-unchanged-stats`
+* check whether process p is alive at least every number of seconds (relevant for `--pid`)
 
-- [ ] `--max-unchanged-stats` : with `--follow=name`, reopen a FILE which has not changed size after N (default 5) iterations  to see if it has been unlinked or renamed (this is the usual case of rotated log files).  With `inotify`, this option is rarely useful.
-- [ ] `--retry` : keep trying to open a file even when it is or becomes inaccessible; useful when follow‐ing by name, i.e., with `--follow=name`
+Note:
+There's a stub for `--max-unchanged-stats` so GNU test-suite checks using it can run, however this flag has no functionality yet.
 
-### Others
+### 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 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.
 
-- [ ] The current implementation doesn't follow stdin in non-unix platforms
+Note:
+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.
+* 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 test-suite results (9.1.8-e08752)
+
+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-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.
+
+There are 5 tests which are fixed but do not (always) pass the test suite if it's run inside the CI.
+The reason for this is probably related to load/scheduling on the CI test VM.
+The tests in question are:
+- [x] `tail-2/F-vs-rename.sh`
+- [x] `tail-2/follow-name.sh`
+- [x] `tail-2/inotify-rotate.sh`
+- [x] `tail-2/overlay-headers.sh`
+- [x] `tail-2/retry.sh`

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,

src/uu/tail/src/platform/mod.rs

@@ -9,7 +9,9 @@
  */
 
 #[cfg(unix)]
-pub use self::unix::{stdin_is_pipe_or_fifo, supports_pid_checks, Pid, ProcessChecker};
+pub use self::unix::{
+    stdin_is_bad_fd, stdin_is_pipe_or_fifo, supports_pid_checks, Pid, ProcessChecker,
+};
 
 #[cfg(windows)]
 pub use self::windows::{supports_pid_checks, Pid, ProcessChecker};

src/uu/tail/src/platform/unix.rs

@@ -8,7 +8,8 @@
  * file that was distributed with this source code.
  */
 
-// spell-checker:ignore (ToDO) errno EPERM ENOSYS
+// spell-checker:ignore (ToDO) stdlib
+// spell-checker:ignore (options) GETFD EPERM ENOSYS
 
 use std::io::{stdin, Error};
 
@@ -51,13 +52,23 @@ fn get_errno() -> i32 {
 
 pub fn stdin_is_pipe_or_fifo() -> bool {
     let fd = stdin().lock().as_raw_fd();
-    fd >= 0 // GNU tail checks fd >= 0
-                            && match fstat(fd) {
-                                Ok(stat) => {
-                                    let mode = stat.st_mode as libc::mode_t;
-                                    // NOTE: This is probably not the most correct way to check this
-                                    (mode & S_IFIFO != 0) || (mode & S_IFSOCK != 0)
-                                }
-                                Err(err) => panic!("{}", err),
-                            }
+    // GNU tail checks fd >= 0
+    fd >= 0
+        && match fstat(fd) {
+            Ok(stat) => {
+                let mode = stat.st_mode as libc::mode_t;
+                // NOTE: This is probably not the most correct way to check this
+                (mode & S_IFIFO != 0) || (mode & S_IFSOCK != 0)
+            }
+            Err(err) => panic!("{}", err),
+        }
+}
+
+// FIXME: Detect a closed file descriptor, e.g.: `tail <&-`
+pub fn stdin_is_bad_fd() -> bool {
+    let fd = stdin().as_raw_fd();
+    // this is never `true`, even with `<&-` because Rust's stdlib is reopening fds as /dev/null
+    // see also: https://github.com/uutils/coreutils/issues/2873
+    // (gnu/tests/tail-2/follow-stdin.sh fails because of this)
+    unsafe { libc::fcntl(fd, libc::F_GETFD) == -1 && get_errno() == libc::EBADF }
 }