Re: inotify back end for tail -f on linux

[Giuseppe Scrivano]

Hello,

Thanks for your fast review.

Jim Meyering <address@hidden> writes:

> Please rename to n_files, to be consistent with other variables
> in this file.  Besides, it's slightly more readable.

The same name is used in the tail_forever function, to keep consistency
between tail_forever and tail_forever_inotify, I changed nfiles in
tail_forever too.


> Typically we abbreviate with "buf", not "buff".
> How about evbuf and evbuf_off?

Ok, done.


> Whoops.
> You forgot to detect insertion failure.
> I'm adding __attribute__ ((__warn_unused_result__))
> to that function and a few others in gnulib's hash.h,
> so the compiler will detect this if it happens again.

Added.


> I have a slight preference for the shorter #if ...:
>
>   #if HAVE_INOTIFY

I replaced all #ifdef HAVE_INOTIFY with the shorter form.


> For readability, I usually prefer not to use a single-line else block
> unless the then-block is also a one-liner.  Please do this instead:
>
>            if (wd < 0)
>              error (0, errno, _("inotify cannot be used, reverting to 
> polling"));
>            else
>              {
>                ...
>              }

Done.


> How about just this (like below)?
>
>     touch here || framework_failure
>
> Each test is guaranteed to start in an empty directory.
>

I removed all the unuseful additional checks to don't modify existing
files, I didn't know that tests start in a clean temporary directory.


> Bonus points if you can test effectively (i.e., still avoid race conditions)
> without sleeping for long.  I consider 4-5 seconds "long".

I couldn't remove `sleep's completely but I reduced their time.  I also
fixed some mistakes I previously did.


> No need to remove files.
> The entire temporary directory is removed automatically.
> Same below.

As above, I didn't know it was a temporary directory.  I removed these
`rm's too.


> With these two new uses, the above test has been repeated enough times
> now that it deserves to be factored into a require_proc_pid_status_
> function in test-lib.sh.
> [....]
> The above 3 lines are repeated far too many times.
> Please define a function in test-lib.sh, and then do e.g.,

I refactored the code as you suggested, also I changed tail-2/tail-n0f
to use this new function.


> - insufficient quoting; that will evoke a syntax error if $state is empty
>     (btw, below, no quotes in "case $state in..." is fine)
> - use -n in place of "! -z";
> - I prefer to use "test" in place of "["..."]"

I changed it.


>> +test  "$(wc -w < tail.err) eq 0"  || fail=1
>
> This can be written more simply:
>
>   test -s tail.err && fail=1

Done.


The new version includes all these changes.

Cheers,
Giuseppe




>From 53bf397a3d34ccd2186197926c63b089477e5a62 Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <address@hidden>
Date: Tue, 2 Jun 2009 08:28:23 +0200
Subject: [PATCH] tail: Use inotify if it is available.

* NEWS: Document the new feature.
* m4/jm-macros.m4: Check if inotify is present.
* src/tail.c (tail_forever_inotify): New function.
(main): Use the inotify-based function, if possible.
* tests/Makefile.am: Added new tests for tail.
* tests/test-lib.sh: The require_proc_pid_status_ and
get_process_status_ functions were added.
* tests/tail-2/pid: New file.
* tests/tail-2/wait: New file.
* tests/tail-2/tail-n0f: Refactored code into the
test-lib.sh require_proc_pid_status_ function.
---
 NEWS                  |    2 +
 m4/jm-macros.m4       |    5 +
 src/tail.c            |  265 +++++++++++++++++++++++++++++++++++++++++++++++--
 tests/Makefile.am     |    2 +
 tests/tail-2/pid      |   68 +++++++++++++
 tests/tail-2/tail-n0f |    9 +--
 tests/tail-2/wait     |  118 ++++++++++++++++++++++
 tests/test-lib.sh     |   18 ++++
 8 files changed, 472 insertions(+), 15 deletions(-)
 create mode 100755 tests/tail-2/pid
 create mode 100755 tests/tail-2/wait

diff --git a/NEWS b/NEWS
index 29b09a0..cc61bdc 100644
--- a/NEWS
+++ b/NEWS
@@ -14,6 +14,8 @@ GNU coreutils NEWS                                    -*- 
outline -*-
   sort accepts a new option, --human-numeric-sort (-h): sort numbers
   while honoring human readable suffixes like KiB and MB etc.
 
+  tail uses inotify when possible.
+
 
 * Noteworthy changes in release 7.4 (2009-05-07) [stable]
 
diff --git a/m4/jm-macros.m4 b/m4/jm-macros.m4
index 90c55bf..f14d6a3 100644
--- a/m4/jm-macros.m4
+++ b/m4/jm-macros.m4
@@ -52,6 +52,11 @@ AC_DEFUN([coreutils_MACROS],
   # Used by sort.c.
   AC_CHECK_FUNCS_ONCE([nl_langinfo])
 
+  # Used by tail.c.
+  AC_CHECK_FUNCS([inotify_init],
+    [AC_DEFINE([HAVE_INOTIFY], [1],
+     [Define to 1 if you have usable inotify support.])])
+
   AC_CHECK_FUNCS_ONCE( \
     endgrent \
     endpwent \
diff --git a/src/tail.c b/src/tail.c
index 9d416e1..1de76b1 100644
--- a/src/tail.c
+++ b/src/tail.c
@@ -21,7 +21,8 @@
 
    Original version by Paul Rubin <address@hidden>.
    Extensions by David MacKenzie <address@hidden>.
-   tail -f for multiple files by Ian Lance Taylor <address@hidden>.  */
+   tail -f for multiple files by Ian Lance Taylor <address@hidden>.
+   inotify back-end by Giuseppe Scrivano <address@hidden>.  */
 
 #include <config.h>
 
@@ -46,6 +47,11 @@
 #include "xstrtol.h"
 #include "xstrtod.h"
 
+#if HAVE_INOTIFY
+# include "hash.h"
+# include <sys/inotify.h>
+#endif
+
 /* The official name of this program (e.g., no `g' prefix).  */
 #define PROGRAM_NAME "tail"
 
@@ -125,6 +131,17 @@ struct File_spec
   /* The value of errno seen last time we checked this file.  */
   int errnum;
 
+#if HAVE_INOTIFY
+  /* The watch descriptor used by inotify.  */
+  int wd;
+
+  /* The parent directory watch descriptor.  It is used only
+   * when Follow_name is used.  */
+  int parent_wd;
+
+  /* Offset in NAME of the basename part.  */
+  size_t basename_start;
+#endif
 };
 
 /* Keep trying to open a file even if it is inaccessible when tail starts
@@ -964,7 +981,7 @@ any_live_files (const struct File_spec *f, int n_files)
   return false;
 }
 
-/* Tail NFILES files forever, or until killed.
+/* Tail N_FILES files forever, or until killed.
    The pertinent information for each file is stored in an entry of F.
    Loop over each of them, doing an fstat to see if they have changed size,
    and an occasional open/fstat to see if any dev/ino pair has changed.
@@ -972,22 +989,22 @@ any_live_files (const struct File_spec *f, int n_files)
    while and try again.  Continue until the user interrupts us.  */
 
 static void
-tail_forever (struct File_spec *f, int nfiles, double sleep_interval)
+tail_forever (struct File_spec *f, int n_files, double sleep_interval)
 {
   /* Use blocking I/O as an optimization, when it's easy.  */
   bool blocking = (pid == 0 && follow_mode == Follow_descriptor
-                  && nfiles == 1 && ! S_ISREG (f[0].mode));
+                  && n_files == 1 && ! S_ISREG (f[0].mode));
   int last;
   bool writer_is_dead = false;
 
-  last = nfiles - 1;
+  last = n_files - 1;
 
   while (1)
     {
       int i;
       bool any_input = false;
 
-      for (i = 0; i < nfiles; i++)
+      for (i = 0; i < n_files; i++)
        {
          int fd;
          char const *name;
@@ -1087,7 +1104,7 @@ tail_forever (struct File_spec *f, int nfiles, double 
sleep_interval)
          f[i].size += bytes_read;
        }
 
-      if (! any_live_files (f, nfiles) && ! reopen_inaccessible_files)
+      if (! any_live_files (f, n_files) && ! reopen_inaccessible_files)
        {
          error (0, 0, _("no files remaining"));
          break;
@@ -1117,6 +1134,221 @@ tail_forever (struct File_spec *f, int nfiles, double 
sleep_interval)
     }
 }
 
+#if HAVE_INOTIFY
+
+static size_t
+wd_hasher (const void *entry, size_t tabsize)
+{
+  const struct File_spec *spec = entry;
+  return spec->wd % tabsize;
+}
+
+static bool
+wd_comparator (const void *e1, const void *e2)
+{
+  const struct File_spec *spec1 = e1;
+  const struct File_spec *spec2 = e2;
+  return spec1->wd == spec2->wd;
+}
+
+/* Tail N_FILES files forever, or until killed.
+   Check modifications using the inotify events system.  */
+static void
+tail_forever_inotify (int wd, struct File_spec *f, int n_files)
+{
+  unsigned int i;
+  unsigned int max_realloc = 3;
+  Hash_table *wd_table;
+
+  bool found_watchable = false;
+  size_t last;
+  size_t evlen = 0;
+  char *evbuf;
+  size_t evbuf_off = 0;
+  ssize_t len = 0;
+
+  wd_table = hash_initialize (n_files, NULL, wd_hasher, wd_comparator, NULL);
+  if (! wd_table)
+    xalloc_die ();
+
+  for (i = 0; i < n_files; i++)
+    {
+      if (!f[i].ignore)
+        {
+          int old_wd = f[i].wd;
+          size_t fnlen = strlen (f[i].name);
+          if (evlen < fnlen)
+            evlen = fnlen;
+
+          f[i].wd = 0;
+
+          if (follow_mode == Follow_name)
+            {
+              size_t dirlen = dir_len (f[i].name);
+              char prev = f[i].name[dirlen];
+              f[i].basename_start = last_component (f[i].name) - f[i].name;
+
+              f[i].name[dirlen] = '\0';
+
+               /* It's fine to add the same directory more than once.
+                  In that case the same watch descriptor is returned.  */
+              f[i].parent_wd = inotify_add_watch (wd, dirlen ? f[i].name : ".",
+                                                  IN_CREATE | IN_MOVED_TO);
+
+              f[i].name[dirlen] = prev;
+
+              if (f[i].parent_wd < 0)
+                {
+                  error (0, errno, _("cannot watch parent directory of %s"),
+                         quote (f[i].name));
+                  continue;
+                }
+            }
+
+          old_wd = f[i].wd;
+          f[i].wd = inotify_add_watch (wd, f[i].name,
+                                       (IN_MODIFY | IN_ATTRIB
+                                        | IN_DELETE_SELF | IN_MOVE_SELF));
+
+          if (last == old_wd)
+            last = f[i].wd;
+
+          if (f[i].wd < 0)
+            {
+              if (errno != f[i].errnum)
+                error (0, errno, _("cannot watch %s"), quote (f[i].name));
+              continue;
+            }
+
+          if (hash_insert (wd_table, &(f[i])) == NULL)
+            xalloc_die ();
+
+          if (follow_mode == Follow_name || f[i].wd)
+            found_watchable = true;
+        }
+    }
+
+  if (follow_mode == Follow_descriptor && !found_watchable)
+    return;
+
+  last = f[n_files - 1].wd;
+
+  evlen += sizeof (struct inotify_event) + 1;
+  evbuf = xmalloc (evlen);
+
+  while (1)
+    {
+      char const *name;
+      struct File_spec *fspec;
+      uintmax_t bytes_read;
+      struct stat stats;
+
+      struct inotify_event *ev;
+
+      if (len <= evbuf_off)
+        {
+          len = safe_read (wd, evbuf, evlen);
+          evbuf_off = 0;
+
+          if (len == SAFE_READ_ERROR && errno == EINVAL && max_realloc--)
+            {
+              len = 0;
+              evlen *= 2;
+              evbuf = xrealloc (evbuf, evlen);
+              continue;
+            }
+
+          if (len == SAFE_READ_ERROR)
+            error (EXIT_FAILURE, errno, _("error reading inotify event"));
+        }
+
+      ev = (struct inotify_event *) (evbuf + evbuf_off);
+      evbuf_off += sizeof (*ev) + ev->len;
+
+      if (ev->mask & (IN_CREATE | IN_MOVED_TO))
+        {
+          for (i = 0; i < n_files; i++)
+            {
+              if (f[i].parent_wd == ev->wd &&
+                  STREQ (ev->name, f[i].name + f[i].basename_start))
+                break;
+            }
+
+          /* It is not a watched file.  */
+          if (i == n_files)
+            continue;
+
+          f[i].wd = inotify_add_watch (wd, f[i].name,
+                                       IN_MODIFY | IN_ATTRIB | IN_DELETE_SELF);
+
+          if (f[i].wd < 0)
+            {
+              error (0, errno, _("cannot watch %s"), quote (f[i].name));
+              continue;
+            }
+
+          fspec = &(f[i]);
+          if (hash_insert (wd_table, fspec) == NULL)
+            xalloc_die ();
+
+          if (follow_mode == Follow_name)
+            recheck (&(f[i]), false);
+        }
+      else
+        {
+          struct File_spec key;
+          key.wd = ev->wd;
+          fspec = hash_lookup (wd_table, &key);
+        }
+
+      if (! fspec)
+        continue;
+
+      if (ev->mask & (IN_ATTRIB | IN_DELETE_SELF | IN_MOVE_SELF))
+        {
+          if (ev->mask & (IN_DELETE_SELF | IN_MOVE_SELF))
+            {
+              inotify_rm_watch (wd, f[i].wd);
+              hash_delete (wd_table, &(f[i]));
+            }
+          if (follow_mode == Follow_name)
+            recheck (fspec, false);
+
+          continue;
+        }
+
+      name = pretty_name (fspec);
+
+      if (fstat (fspec->fd, &stats) != 0)
+        {
+          close_fd (fspec->fd, name);
+          fspec->fd = -1;
+          fspec->errnum = errno;
+          continue;
+        }
+
+      if (S_ISREG (fspec->mode) && stats.st_size < fspec->size)
+        {
+          error (0, 0, _("%s: file truncated"), name);
+          last = ev->wd;
+          xlseek (fspec->fd, stats.st_size, SEEK_SET, name);
+          fspec->size = stats.st_size;
+        }
+
+      if (ev->wd != last)
+        {
+          if (print_headers)
+            write_header (name);
+          last = ev->wd;
+        }
+
+      bytes_read = dump_remainder (name, fspec->fd, COPY_TO_EOF);
+      fspec->size += bytes_read;
+    }
+
+}
+#endif
+
 /* Output the last N_BYTES bytes of file FILENAME open for reading in FD.
    Return true if successful.  */
 
@@ -1691,7 +1923,24 @@ main (int argc, char **argv)
     ok &= tail_file (&F[i], n_units);
 
   if (forever)
-    tail_forever (F, n_files, sleep_interval);
+    {
+#if HAVE_INOTIFY
+      if (pid == 0)
+        {
+          int wd = inotify_init ();
+          if (wd < 0)
+            error (0, errno, _("inotify cannot be used, reverting to 
polling"));
+          else
+            {
+              tail_forever_inotify (wd, F, n_files);
+
+              /* The only way the above returns is upon failure.  */
+              exit (EXIT_FAILURE);
+            }
+        }
+#endif
+      tail_forever (F, n_files, sleep_interval);
+    }
 
   if (have_read_stdin && close (STDIN_FILENO) < 0)
     error (EXIT_FAILURE, errno, "-");
diff --git a/tests/Makefile.am b/tests/Makefile.am
index a0ed986..c108356 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -419,6 +419,8 @@ TESTS =                                             \
   tail-2/big-4gb                               \
   tail-2/proc-ksyms                            \
   tail-2/start-middle                          \
+  tail-2/pid                                   \
+  tail-2/wait                                  \
   touch/dangling-symlink                       \
   touch/dir-1                                  \
   touch/fail-diag                              \
diff --git a/tests/tail-2/pid b/tests/tail-2/pid
new file mode 100755
index 0000000..ff2f099
--- /dev/null
+++ b/tests/tail-2/pid
@@ -0,0 +1,68 @@
+#!/bin/sh
+# Test the --pid option of tail.
+
+# Copyright (C) 2003, 2006-2009 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if test "$VERBOSE" = yes; then
+  set -x
+  tail --version
+fi
+
+. $srcdir/test-lib.sh
+
+require_proc_pid_status_
+
+touch here || framework_failure
+
+
+fail=0
+
+# Use tail itself to create a background process.
+
+tail -f here &
+bg_pid=$!
+
+tail -s0.1 -f here --pid=$bg_pid &
+
+pid=$!
+
+sleep 0.5
+
+state=$(get_process_status_ $pid)
+
+if test -n "$state"; then
+  case $state in
+    S*) ;;
+    *) echo $0: process dead 1>&2; fail=1 ;;
+  esac
+  kill $pid
+fi
+
+kill $bg_pid
+
+sleep 0.5
+
+state=$(get_process_status_ $pid)
+
+if test -n "$state"; then
+  case $state in
+    S*) echo $0: process still active 1>&2; fail=1 ;;
+    *) ;;
+  esac
+  kill $pid
+fi
+
+Exit $fail
diff --git a/tests/tail-2/tail-n0f b/tests/tail-2/tail-n0f
index bf19cf5..322fddc 100755
--- a/tests/tail-2/tail-n0f
+++ b/tests/tail-2/tail-n0f
@@ -2,7 +2,7 @@
 # Make sure that `tail -n0 -f' and `tail -c0 -f' sleep
 # rather than doing what amounted to a busy-wait.
 
-# Copyright (C) 2003, 2006-2008 Free Software Foundation, Inc.
+# Copyright (C) 2003, 2006-2009 Free Software Foundation, Inc.
 
 # This program is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -28,12 +28,7 @@ fi
 
 . $srcdir/test-lib.sh
 
-sleep 2 &
-pid=$!
-sleep .5
-grep '^State:[  ]*[S]' /proc/$pid/status > /dev/null 2>&1 ||
-  skip_test_ "/proc/$pid/status: missing or 'different'"
-kill $pid
+require_proc_pid_status_
 
 touch empty || framework_failure
 echo anything > nonempty || framework_failure
diff --git a/tests/tail-2/wait b/tests/tail-2/wait
new file mode 100755
index 0000000..17c07dd
--- /dev/null
+++ b/tests/tail-2/wait
@@ -0,0 +1,118 @@
+#!/bin/sh
+# Make sure that `tail -f' returns immediately if a file doesn't exist
+# while `tail -F' waits for it to appear.
+
+# Copyright (C) 2003, 2006-2009 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if test "$VERBOSE" = yes; then
+  set -x
+  tail --version
+fi
+
+. $srcdir/test-lib.sh
+
+require_proc_pid_status_
+
+touch here || framework_failure
+(touch not_accessible && chmod 0 not_accessible) || framework_failure
+
+fail=0
+
+tail -s0.1 -f not_here &
+pid=$!
+sleep .5
+state=$(get_process_status_ $pid)
+
+if test -n "$state"; then
+  case $state in
+    S*) echo $0: process still active 1>&2; fail=1 ;;
+    *) ;;
+  esac
+  kill $pid
+fi
+
+tail -s0.1 -f not_accessible &
+pid=$!
+sleep .5
+state=$(get_process_status_ $pid)
+
+if test -n "$state"; then
+  case $state in
+    S*) echo $0: process still active 1>&2; fail=1 ;;
+    *) ;;
+  esac
+  kill $pid
+fi
+
+(tail -s0.1 -f here 2>tail.err) &
+pid=$!
+sleep .5
+state=$(get_process_status_ $pid)
+
+if test -n "$state"; then
+  case $state in
+    S*) ;;
+    *)  echo $0: process died 1>&2; fail=1 ;;
+  esac
+  kill $pid
+fi
+
+
+
+# `tail -F' must wait in any case.
+
+(tail -s0.1 -F here 2>>tail.err) &
+pid=$!
+sleep .5
+state=$(get_process_status_ $pid)
+
+if test -n "$state"; then
+  case $state in
+    S*) ;;
+    *) echo $0: process died 1>&2; fail=1 ;;
+  esac
+  kill $pid
+fi
+
+tail -s0.1 -F not_accessible &
+pid=$!
+sleep .5
+state=$(get_process_status_ $pid)
+
+if test -n "$state"; then
+  case $state in
+    S*) ;;
+    *) echo $0: process died 1>&2; fail=1 ;;
+  esac
+  kill $pid
+fi
+
+tail -s0.1 -F not_here &
+pid=$!
+sleep .5
+state=$(get_process_status_ $pid)
+
+if test -n "$state"; then
+  case $state in
+    S*) ;;
+    *) echo $0: process died 1>&2; fail=1 ;;
+  esac
+  kill $pid
+fi
+
+test -s tail.err && fail=1
+
+Exit $fail
diff --git a/tests/test-lib.sh b/tests/test-lib.sh
index a765bd6..8296ed5 100644
--- a/tests/test-lib.sh
+++ b/tests/test-lib.sh
@@ -122,6 +122,13 @@ uid_is_privileged_()
   esac
 }
 
+get_process_status_()
+{
+  set _ `sed -n '/^State:[      ]*\([^  ]\)/s//\1/p' /proc/$1/status`
+  shift # Remove the leading `_'.
+  return $1
+}
+
 # Convert an ls-style permission string, like drwxr----x and -rw-r-x-wx
 # to the equivalent chmod --mode (-m) argument, (=,u=rwx,g=r,o=x and
 # =,u=rw,g=rx,o=wx).  Ignore ACLs.
@@ -234,6 +241,17 @@ of group names or numbers.  E.g.,
   esac
 }
 
+# Is /proc/$PID/status supported?
+require_proc_pid_status_()
+{
+    sleep 2 &
+    pid=$!
+    sleep .5
+    grep '^State:[      ]*[S]' /proc/$pid/status > /dev/null 2>&1 ||
+    skip_test_ "/proc/$pid/status: missing or 'different'"
+    kill $pid
+}
+
 # Does the current (working-dir) file system support sparse files?
 require_sparse_support_()
 {
-- 
1.6.3.1