Re: inotify back end for tail -f on linux

[Jim Meyering]

Giuseppe Scrivano wrote:

> Jim Meyering <address@hidden> writes:
>
>> If we add inotify support, I'd like it to work for
>> both tail-by-FD and tail-by-name.
>>
>> You can watch the parent directory for e.g., rename and creation events.
>> Log rotation is so common, that it'd be a shame not to be able to take
>> advantage of inotify when using "tail -F ...".
>>
>> For starters, you needn't worry about recovering from parent-dir-removal.
>> I think that parent removal is rare enough in practice that we won't
>> need to deal with it for some time.
>>
>> Eventually, the code could monitor the longest parent prefix that *does* 
>> exist.
>> When the missing subdir reappears, monitor $parent/$subdir, etc.
>> However, there's no need to do that now.
>
> Considering only the parent directory makes things easier but I don't
> think that in future it will be difficult to remove completely this
> limit.
>
> The version I attached is a bit more complex than the last one.  Since
> the watch descriptors are added at any time, I can't do any consideration
> on their range and I am using a hash table to access the files
> information in a costant time.
>
> I need to stress my patch better but it seems already to cover all cases
> you showed me.
>
>
>> I haven't looked closely at the code below or even tried it,
>> but here are a few superficial comments:
>
> Thank you.  I included them except this one:
>
>>> +      if (!len && errno == EINTR)
>>> +        continue;
>>> +
>>> +      if (!len)
>>> +        error (EXIT_FAILURE, errno, _("error reading inotify event"));
>>
>> This function must not exit upon a failure that is specific
>> to a single file.  Just diagnose it and continue with any other files.
>> Of course, if there are no other usable file descriptors, *then*
>> you will need to exit the loop -- since this is currently used only
>> when tailing-by-FD.
>>
>> You would not exit a similar loop when tailing by name.
>
> I added some code to recover from errors when possible.  I don't think
> there are other recoverable cases but if we want to force it to re-try
> read then we will need to avoid an end-less loop when there is an
> unrecoverable situation.  What do you think?

Thanks for persevering.
A few suggestions:
  - configure with --enable-gcc-warnings.
      That exposed two unused-variables in your patch.
  - always run "make syntax-check"

    That exposed two easily-fixed problems:
      - use of !strcmp that should be a use of STREQ
      - unnecessary inclusion of dirname.h

        src/tail.c:51:# include "dirname.h"
        maint.mk: the above are already included via system.h

        src/tail.c:1276: !strcmp (ev->name, f[i].name + f[i].basename_start))
        maint.mk: use STREQ in place of the above uses of strcmp
        make: *** [sc_prohibit_strcmp] Error 1

Note this is the second time that my attempt to apply
your patch with "git am FILE" failed.
That seems to be due to your using an out of date version
of tail.c, since your base version has only 2008 listed on
the Copyright line, while the latest has 2008-2009.

I've also adjusted indentation, spacing-around-operators, added curly
braces around a single-stmt (but three-line) loop body, and
changed a variable name (s/watched/found_watchable/) and types.
In general: avoid "int". i.e., prefer unsigned types for counters,
"bool" for boolean.

Please base your future changes on the patch below.
FYI, while we wait for savannah.gnu.org to recover from
its disk failures, you can use this up to date mirror:

  http://repo.or.cz/w/coreutils.git

First, save the patch below in FILE, and change the first line
so it starts with "From ", not ">From ".
Then get a clean, up to date clone:

    git clone git://repo.or.cz/coreutils.git
    cd coreutils

Create a private branch

    git checkout -b tail-inotify

And apply your modified patch:

    git am FILE

Finally, please think about adding some tests.
This change is invasive enough that we'll want
to exercise as much of it as possible before imposing
it on everyone.

I'll actually run/test your modified version of
tail later today or tomorrow.

Oops.  I see that this change induces at least one "make check"
test failure:

FAIL: tail-2/tail-n0f

+ pid=9087
+ sleep .5
+ tail --sleep=4 -n 0 -f empty
./tail-2/tail-n0f: line 44:  9087 Segmentation fault      tail --sleep=4 
-${c_or_n} 0 -f $file

----------------------------------------------------
Here are the changes I've made to your patch,
and which are included in the amended patch below.
(i.e., do *not* apply this patch.  rather just save the complete
patch below and apply *it* with "git am FILE")

diff --git a/src/tail.c b/src/tail.c
index b64b069..0acec2c 100644
--- a/src/tail.c
+++ b/src/tail.c
@@ -48,7 +48,6 @@

 #ifdef HAVE_INOTIFY
 # include "hash.h"
-# include "dirname.h"
 # include <sys/inotify.h>
 #endif

@@ -1156,23 +1155,18 @@ wd_comparator (const void *e1, const void *e2)
 static void
 tail_forever_inotify (int wd, struct File_spec *f, int nfiles)
 {
-  /* Create an index to access File_spec by its watch descriptor
-   * in costant time.  */
-  struct File_spec **wd_index;
-  int i;
-  int max_realloc = 3;
+  unsigned int i;
+  unsigned int max_realloc = 3;
   Hash_table *wd_table;

-  int watched = 0;
+  bool found_watchable = false;
   size_t last;
   size_t evlen = 0;
   char *evbuff;
   size_t evbuff_off = 0;
   ssize_t len = 0;

-  wd_table = hash_initialize (nfiles, NULL, wd_hasher,
-                              wd_comparator,
-                              NULL);
+  wd_table = hash_initialize (nfiles, NULL, wd_hasher, wd_comparator, NULL);
   if (! wd_table)
     xalloc_die ();

@@ -1194,10 +1188,10 @@ tail_forever_inotify (int wd, struct File_spec *f, int 
nfiles)

               f[i].name[dirlen] = '\0';

-              /* Do not care if the directory was already added, in this case 
the
-               * same watch descriptor will be returned.  */
-              f[i].parent_wd = inotify_add_watch (wd, f[i].name, IN_CREATE|
-                                                  IN_MOVED_TO);
+              /* Do not care if the directory was already added, in this
+                 case the same watch descriptor will be returned.  */
+              f[i].parent_wd = inotify_add_watch (wd, f[i].name,
+                                                  IN_CREATE | IN_MOVED_TO);

               f[i].name[dirlen] = prev;

@@ -1211,9 +1205,9 @@ tail_forever_inotify (int wd, struct File_spec *f, int 
nfiles)

           if (f[i].errnum != ENOENT)
             {
-              f[i].wd = inotify_add_watch (wd, f[i].name, IN_MODIFY|
-                                           IN_ATTRIB|IN_DELETE_SELF|
-                                           IN_MOVE_SELF);
+              f[i].wd = inotify_add_watch (wd, f[i].name,
+                                           (IN_MODIFY | IN_ATTRIB
+                                            | IN_DELETE_SELF | IN_MOVE_SELF));

               if (f[i].wd < 0)
                 {
@@ -1224,12 +1218,12 @@ tail_forever_inotify (int wd, struct File_spec *f, int 
nfiles)
               hash_insert (wd_table, &(f[i]));
             }

-            if (follow_mode == Follow_name || f[i].wd)
-                watched++;
+          if (follow_mode == Follow_name || f[i].wd)
+            found_watchable = true;
         }
     }

-  if (!watched)
+  if (!found_watchable)
     return;

   last = f[nfiles - 1].wd;
@@ -1239,7 +1233,6 @@ tail_forever_inotify (int wd, struct File_spec *f, int 
nfiles)

   while (1)
     {
-      char inotify_buffer;
       char const *name;
       struct File_spec *fspec;
       uintmax_t bytes_read;
@@ -1272,21 +1265,23 @@ tail_forever_inotify (int wd, struct File_spec *f, int 
nfiles)
             error (EXIT_FAILURE, errno, _("error reading inotify event"));
         }

-      ev = (struct inotify_event*)(evbuff + evbuff_off);
+      ev = (struct inotify_event *) (evbuff + evbuff_off);

-      if (ev->mask & (IN_CREATE|IN_MOVED_TO))
+      if (ev->mask & (IN_CREATE | IN_MOVED_TO))
         {
           for (i = 0; i < nfiles; i++)
-            if (f[i].parent_wd == ev->wd &&
-                !strcmp (ev->name, f[i].name + f[i].basename_start))
-              break;
+            {
+              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 == nfiles)
             continue;

-          f[i].wd = inotify_add_watch (wd, f[i].name, IN_MODIFY|IN_ATTRIB|
-                                       IN_DELETE_SELF);
+          f[i].wd = inotify_add_watch (wd, f[i].name,
+                                       IN_MODIFY | IN_ATTRIB | IN_DELETE_SELF);

           if (f[i].wd < 0)
             {


>From 7e03a13528d4574ccf43d14e869b7095557a6114 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.
* configure.ac: Check if inotify is present.
* src/tail.c (tail_forever_inotify): New function.
(main): Use the inotify-based function, if possible.
---
 NEWS         |    2 +
 configure.ac |    2 +
 src/tail.c   |  240 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 243 insertions(+), 1 deletions(-)

diff --git a/NEWS b/NEWS
index 29b09a0..c5a2ef5 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 if it is present.
+

 * Noteworthy changes in release 7.4 (2009-05-07) [stable]

diff --git a/configure.ac b/configure.ac
index 4eb640e..a63ac0c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -410,6 +410,8 @@ AM_GNU_GETTEXT_VERSION([0.15])
 # For a test of uniq: it uses the $LOCALE_FR envvar.
 gt_LOCALE_FR

+AC_CHECK_FUNCS([inotify_init], [AC_DEFINE([HAVE_INOTIFY], [1], [Check for 
libinotify])])
+
 AC_CONFIG_FILES(
   Makefile
   doc/Makefile
diff --git a/src/tail.c b/src/tail.c
index 9d416e1..0acec2c 100644
--- a/src/tail.c
+++ b/src/tail.c
@@ -46,6 +46,11 @@
 #include "xstrtol.h"
 #include "xstrtod.h"

+#ifdef 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 +130,17 @@ struct File_spec
   /* The value of errno seen last time we checked this file.  */
   int errnum;

+#ifdef 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
@@ -1117,6 +1133,221 @@ tail_forever (struct File_spec *f, int nfiles, double 
sleep_interval)
     }
 }

+#ifdef 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 NFILES files forever, or until killed.
+   Check modifications using the inotify events system.  */
+static void
+tail_forever_inotify (int wd, struct File_spec *f, int nfiles)
+{
+  unsigned int i;
+  unsigned int max_realloc = 3;
+  Hash_table *wd_table;
+
+  bool found_watchable = false;
+  size_t last;
+  size_t evlen = 0;
+  char *evbuff;
+  size_t evbuff_off = 0;
+  ssize_t len = 0;
+
+  wd_table = hash_initialize (nfiles, NULL, wd_hasher, wd_comparator, NULL);
+  if (! wd_table)
+    xalloc_die ();
+
+  for (i = 0; i < nfiles; i++)
+    {
+      if (!f[i].ignore)
+        {
+          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';
+
+              /* Do not care if the directory was already added, in this
+                 case the same watch descriptor will be returned.  */
+              f[i].parent_wd = inotify_add_watch (wd, 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;
+                }
+            }
+
+          if (f[i].errnum != ENOENT)
+            {
+              f[i].wd = inotify_add_watch (wd, f[i].name,
+                                           (IN_MODIFY | IN_ATTRIB
+                                            | IN_DELETE_SELF | IN_MOVE_SELF));
+
+              if (f[i].wd < 0)
+                {
+                  error (0, errno, _("cannot watch %s"), quote (f[i].name));
+                  continue;
+                }
+
+              hash_insert (wd_table, &(f[i]));
+            }
+
+          if (follow_mode == Follow_name || f[i].wd)
+            found_watchable = true;
+        }
+    }
+
+  if (!found_watchable)
+    return;
+
+  last = f[nfiles - 1].wd;
+
+  evlen += sizeof (struct inotify_event) + 1;
+  evbuff = xmalloc (evlen);
+
+  while (1)
+    {
+      char const *name;
+      struct File_spec *fspec;
+      uintmax_t bytes_read;
+      struct stat stats;
+
+      struct inotify_event *ev;
+
+      evbuff_off += sizeof (*ev) + ev->len;
+
+      if (len <= evbuff_off)
+        {
+          len = read (wd, evbuff, evlen);
+          evbuff_off = 0;
+
+          if (len <= 0 && errno == EINVAL && max_realloc--)
+            {
+              len = 0;
+              evlen *= 2;
+              evbuff = xrealloc (evbuff, evlen);
+              continue;
+            }
+
+          if (len <= 0 && errno == EINTR)
+            {
+              len = 0;
+              continue;
+            }
+
+          if (len <= 0)
+            error (EXIT_FAILURE, errno, _("error reading inotify event"));
+        }
+
+      ev = (struct inotify_event *) (evbuff + evbuff_off);
+
+      if (ev->mask & (IN_CREATE | IN_MOVED_TO))
+        {
+          for (i = 0; i < nfiles; 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 == nfiles)
+            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]);
+          hash_insert (wd_table, fspec);
+
+          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]));
+            }
+
+          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 +1922,14 @@ main (int argc, char **argv)
     ok &= tail_file (&F[i], n_units);

   if (forever)
-    tail_forever (F, n_files, sleep_interval);
+    {
+#ifdef HAVE_INOTIFY
+      int wd = inotify_init ();
+      if (wd > 0)
+        tail_forever_inotify (wd, F, n_files);
+#endif
+      tail_forever (F, n_files, sleep_interval);
+    }

   if (have_read_stdin && close (STDIN_FILENO) < 0)
     error (EXIT_FAILURE, errno, "-");
--
1.6.3.1.308.g426b5