Merge pull request #2695 from jhscheer/tail_notify

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

Cargo.lock

@@ -836,6 +836,15 @@ version = "1.2.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "2022715d62ab30faffd124d40b76f4134a550a87792276512b18d63272333394"
 
+[[package]]
+name = "fsevent-sys"
+version = "4.1.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "76ee7a02da4d231650c7cea31349b889be2f45ddb3ef3032d2ec8185f6313fd2"
+dependencies = [
+ "libc",
+]
+
 [[package]]
 name = "fts-sys"
 version = "0.2.1"
@@ -957,6 +966,26 @@ dependencies = [
  "hashbrown 0.11.2",
 ]
 
+[[package]]
+name = "inotify"
+version = "0.9.6"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "f8069d3ec154eb856955c1c0fbffefbf5f3c40a104ec912d4797314c1801abff"
+dependencies = [
+ "bitflags",
+ "inotify-sys",
+ "libc",
+]
+
+[[package]]
+name = "inotify-sys"
+version = "0.1.5"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "e05c02b5e89bff3b946cedeca278abc628fe811e604f027c45a8aa3cf793d0eb"
+dependencies = [
+ "libc",
+]
+
 [[package]]
 name = "instant"
 version = "0.1.12"
@@ -1003,6 +1032,26 @@ dependencies = [
  "winapi-build",
 ]
 
+[[package]]
+name = "kqueue"
+version = "1.0.5"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "97caf428b83f7c86809b7450722cd1f2b1fc7fb23aa7b9dee7e72ed14d048352"
+dependencies = [
+ "kqueue-sys",
+ "libc",
+]
+
+[[package]]
+name = "kqueue-sys"
+version = "1.0.3"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "8367585489f01bc55dd27404dcf56b95e6da061a256a666ab23be9ba96a2e587"
+dependencies = [
+ "bitflags",
+ "libc",
+]
+
 [[package]]
 name = "lazy_static"
 version = "1.4.0"
@@ -1156,6 +1205,24 @@ dependencies = [
  "minimal-lexical",
 ]
 
+[[package]]
+name = "notify"
+version = "5.0.0-pre.15"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "553f9844ad0b0824605c20fb55a661679782680410abfb1a8144c2e7e437e7a7"
+dependencies = [
+ "bitflags",
+ "crossbeam-channel",
+ "filetime",
+ "fsevent-sys",
+ "inotify",
+ "kqueue",
+ "libc",
+ "mio",
+ "walkdir",
+ "winapi 0.3.9",
+]
+
 [[package]]
 name = "num-bigint"
 version = "0.4.3"
@@ -2846,8 +2913,10 @@ dependencies = [
  "clap 3.1.18",
  "libc",
  "nix",
+ "notify",
  "uucore",
  "winapi 0.3.9",
+ "winapi-util",
 ]
 
 [[package]]

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      |        |
+| dircolors | test      |        |
-| dirname   | test      |        |
+| dirname   | dir       |        |
-| du        | dir       |        |
+| du        | vdir      |        |
-| echo      | 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.
+Note:
-- [ ] `--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`
+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
+    // GNU tail checks fd >= 0
-                            && match fstat(fd) {
+    fd >= 0
-                                Ok(stat) => {
+        && match fstat(fd) {
-                                    let mode = stat.st_mode as libc::mode_t;
+            Ok(stat) => {
-                                    // NOTE: This is probably not the most correct way to check this
+                let mode = stat.st_mode as libc::mode_t;
-                                    (mode & S_IFIFO != 0) || (mode & S_IFSOCK != 0)
+                // NOTE: This is probably not the most correct way to check this
-                                }
+                (mode & S_IFIFO != 0) || (mode & S_IFSOCK != 0)
-                                Err(err) => panic!("{}", err),
+            }
-                            }
+            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 }
 }

tests/fixtures/tail/follow_name.expected

@@ -0,0 +1,35 @@
+CHUNK(10)
+vier
+fuenf
+sechs
+sieben
+acht
+neun
+zehn
+elf
+END(25)
+START(0)
+uno
+dos
+tres
+quattro
+cinco
+seis
+siette
+ocho
+nueve
+diez
+once
+eins
+zwei
+drei
+CHUNK(10)
+vier
+fuenf
+sechs
+sieben
+acht
+neun
+zehn
+elf
+END(25)

tests/fixtures/tail/follow_name.txt

@@ -0,0 +1,25 @@
+START(0)
+uno
+dos
+tres
+quattro
+cinco
+seis
+siette
+ocho
+nueve
+diez
+once
+eins
+zwei
+drei
+CHUNK(10)
+vier
+fuenf
+sechs
+sieben
+acht
+neun
+zehn
+elf
+END(25)

tests/fixtures/tail/follow_name_short.expected

@@ -0,0 +1,10 @@
+CHUNK(10)
+vier
+fuenf
+sechs
+sieben
+acht
+neun
+zehn
+elf
+END(25)

util/build-gnu.sh

@@ -142,6 +142,7 @@ sed -i 's|touch |/usr/bin/touch |' tests/cp/preserve-link.sh tests/cp/reflink-pe
 sed -i 's|ln -|/usr/bin/ln -|' tests/cp/link-deref.sh
 sed -i 's|cp |/usr/bin/cp |' tests/mv/hard-2.sh
 sed -i 's|paste |/usr/bin/paste |' tests/misc/od-endian.sh
+sed -i 's|timeout |/usr/bin/timeout |' tests/tail-2/follow-stdin.sh
 
 # Add specific timeout to tests that currently hang to limit time spent waiting
 sed -i 's|\(^\s*\)seq \$|\1/usr/bin/timeout 0.1 seq \$|' tests/misc/seq-precision.sh tests/misc/seq-long-double.sh