Re: Bug#545422: coreutils: "tail -f -" fails

[Giuseppe Scrivano]

Jim Meyering <address@hidden> writes:

> I considered that and discussed the
> trade-off in the comment I committed.
> There have been systems and configurations with
> nonexistent and unusable /dev/stdin files.

sorry, I didn't read you comment.

This patch changes `tail' to handle stdin separately from inotify
events, similar to what we are already doing when a --pid is specified.

Regards,
Giuseppe


>From f3010bebf9e25be9a83868b4ad9db2cc6cb6613f Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <address@hidden>
Date: Mon, 7 Sep 2009 16:35:16 +0200
Subject: [PATCH] tail: handle "-" properly

* src/tail.c (tail_forever_inotify): Handle stdin (i.e., "-", but not
/dev/stdin) separately from inotify.
* tests/tail-2/wait: Ensure that when a stdin is watched, tail does not
raise errors.
---
 src/tail.c        |  176 ++++++++++++++++++++++++++++++++++-------------------
 tests/tail-2/wait |    6 ++
 2 files changed, 119 insertions(+), 63 deletions(-)

diff --git a/src/tail.c b/src/tail.c
index e3b9529..b817ecb 100644
--- a/src/tail.c
+++ b/src/tail.c
@@ -134,7 +134,7 @@ struct File_spec
   int errnum;
 
 #if HAVE_INOTIFY
-  /* The watch descriptor used by inotify.  */
+  /* The watch descriptor used by inotify, -1 on error, -2 if stdin.  */
   int wd;
 
   /* The parent directory watch descriptor.  It is used only
@@ -1184,6 +1184,7 @@ tail_forever_inotify (int wd, struct File_spec *f, size_t 
n_files,
   char *evbuf;
   size_t evbuf_off = 0;
   size_t len = 0;
+  struct File_spec *stdin_spec = NULL;
 
   wd_table = hash_initialize (n_files, NULL, wd_hasher, wd_comparator, NULL);
   if (! wd_table)
@@ -1196,6 +1197,34 @@ tail_forever_inotify (int wd, struct File_spec *f, 
size_t n_files,
     {
       if (!f[i].ignore)
         {
+          if (STREQ (f[i].name, "-"))
+            {
+              int old_flags = fcntl (f[i].fd, F_GETFL);
+              int new_flags = old_flags | O_NONBLOCK;
+
+              stdin_spec = &f[i];
+              found_watchable = true;
+
+              if (old_flags < 0
+                  || (new_flags != old_flags
+                      && fcntl (f[i].fd, F_SETFL, new_flags) == -1))
+                {
+                  /* Don't update f[i].blocking if fcntl fails.  */
+                  if (S_ISREG (f[i].mode) && errno == EPERM)
+                    {
+                      /* This happens when using tail -f on a file with
+                         the append-only attribute.  */
+                    }
+                  else
+                    error (EXIT_FAILURE, errno,
+                           _("%s: cannot change stdin nonblocking mode"));
+                }
+              f[i].blocking = false;
+              f[i].wd = -2;
+              prev_wd = f[i].wd;
+              continue;
+            }
+
           size_t fnlen = strlen (f[i].name);
           if (evlen < fnlen)
             evlen = fnlen;
@@ -1235,6 +1264,8 @@ tail_forever_inotify (int wd, struct File_spec *f, size_t 
n_files,
               continue;
             }
 
+          prev_wd = f[i].wd;
+
           if (hash_insert (wd_table, &(f[i])) == NULL)
             xalloc_die ();
 
@@ -1245,8 +1276,6 @@ tail_forever_inotify (int wd, struct File_spec *f, size_t 
n_files,
   if (follow_mode == Follow_descriptor && !found_watchable)
     return;
 
-  prev_wd = f[n_files - 1].wd;
-
   evlen += sizeof (struct inotify_event) + 1;
   evbuf = xmalloc (evlen);
 
@@ -1259,12 +1288,12 @@ tail_forever_inotify (int wd, struct File_spec *f, 
size_t n_files,
       struct File_spec *fspec;
       uintmax_t bytes_read;
       struct stat stats;
-
+      bool check_stdin = false;
       struct inotify_event *ev;
 
-      /* When watching a PID, ensure that a read from WD will not block
-         indefinetely.  */
-      if (pid)
+      /* When watching a PID or stdin, ensure that a read from WD will not 
block
+         indefinitely.  */
+      if (pid || stdin_spec)
         {
           fd_set rfd;
           struct timeval select_timeout;
@@ -1284,78 +1313,92 @@ tail_forever_inotify (int wd, struct File_spec *f, 
size_t n_files,
 
           if (n_descriptors == 0)
             {
-              /* See if the process we are monitoring is still alive.  */
-              if (kill (pid, 0) != 0 && errno != EPERM)
-                exit (EXIT_SUCCESS);
+              if (stdin_spec)
+                check_stdin = true;
+              if (pid)
+                {
+                  /* See if the process we are monitoring is still alive.  */
+                  if (kill (pid, 0) != 0 && errno != EPERM)
+                    exit (EXIT_SUCCESS);
 
-              continue;
+                  if (!check_stdin)
+                    continue;
+                }
             }
         }
 
-      if (len <= evbuf_off)
+      if (check_stdin)
         {
-          len = safe_read (wd, evbuf, evlen);
-          evbuf_off = 0;
-
-          /* For kernels prior to 2.6.21, read returns 0 when the buffer
-             is too small.  */
-          if ((len == 0 || (len == SAFE_READ_ERROR && errno == EINVAL))
-              && max_realloc--)
+          ev = NULL;
+          fspec = stdin_spec;
+          check_stdin = false;
+        }
+      else
+        {
+          if (len <= evbuf_off)
             {
-              len = 0;
-              evlen *= 2;
-              evbuf = xrealloc (evbuf, evlen);
-              continue;
-            }
+              len = safe_read (wd, evbuf, evlen);
+              evbuf_off = 0;
 
-          if (len == 0 || len == SAFE_READ_ERROR)
-            error (EXIT_FAILURE, errno, _("error reading inotify event"));
-        }
+              /* For kernels prior to 2.6.21, read returns 0 when the buffer
+                 is too small.  */
+              if ((len == 0 || (len == SAFE_READ_ERROR && errno == EINVAL))
+                  && max_realloc--)
+                {
+                  len = 0;
+                  evlen *= 2;
+                  evbuf = xrealloc (evbuf, evlen);
+                  continue;
+                }
 
-      ev = (struct inotify_event *) (evbuf + evbuf_off);
-      evbuf_off += sizeof (*ev) + ev->len;
+              if (len == 0 || len == SAFE_READ_ERROR)
+                error (EXIT_FAILURE, errno, _("error reading inotify event"));
+            }
 
-      if (ev->len)
-        {
-          for (i = 0; i < n_files; i++)
+          ev = (struct inotify_event *) (evbuf + evbuf_off);
+          evbuf_off += sizeof (*ev) + ev->len;
+
+          if (ev->len)
             {
-              /* With N=hundreds of frequently-changing files, this O(N^2)
-                 process might be a problem.  FIXME: use a hash table?  */
-              if (f[i].parent_wd == ev->wd
-                  && STREQ (ev->name, f[i].name + f[i].basename_start))
-                break;
-            }
+              for (i = 0; i < n_files; i++)
+                {
+                  /* With N=hundreds of frequently-changing files, this O(N^2)
+                     process might be a problem.  FIXME: use a hash table?  */
+                  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;
+              /* It is not a watched file.  */
+              if (i == n_files)
+                continue;
 
-          f[i].wd = inotify_add_watch (wd, f[i].name, inotify_wd_mask);
+              f[i].wd = inotify_add_watch (wd, f[i].name, inotify_wd_mask);
 
-          if (f[i].wd < 0)
-            {
-              error (0, errno, _("cannot watch %s"), quote (f[i].name));
-              continue;
-            }
+              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 ();
+              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 (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 && ev->mask & (IN_ATTRIB | IN_DELETE_SELF | IN_MOVE_SELF))
         {
           if (ev->mask & (IN_DELETE_SELF | IN_MOVE_SELF))
             {
@@ -1364,7 +1407,6 @@ tail_forever_inotify (int wd, struct File_spec *f, size_t 
n_files,
             }
           if (follow_mode == Follow_name)
             recheck (fspec, false);
-
           continue;
         }
 
@@ -1386,11 +1428,19 @@ tail_forever_inotify (int wd, struct File_spec *f, 
size_t n_files,
           fspec->size = stats.st_size;
         }
 
-      if (ev->wd != prev_wd)
+      if (fspec->mode == stats.st_mode
+          && (! S_ISREG (stats.st_mode) || fspec->size == stats.st_size)
+          && timespec_cmp (fspec->mtime, get_stat_mtime (&stats)) == 0)
+        continue;
+
+      fspec->mtime = get_stat_mtime (&stats);
+      fspec->mode = stats.st_mode;
+
+      if (fspec->wd != prev_wd)
         {
           if (print_headers)
             write_header (name);
-          prev_wd = ev->wd;
+          prev_wd = fspec->wd;
         }
 
       bytes_read = dump_remainder (name, fspec->fd, COPY_TO_EOF);
diff --git a/tests/tail-2/wait b/tests/tail-2/wait
index 62498d5..aa6e2dd 100755
--- a/tests/tail-2/wait
+++ b/tests/tail-2/wait
@@ -42,6 +42,12 @@ for inotify in ---disable-inotify ''; do
   timeout 1 tail -s0.1 -f $inotify here 2>tail.err
   test $? = 124 || fail=1
 
+  timeout 1 tail -s0.1 -f $inotify < here 2>tail.err
+  test $? = 124 || fail=1
+
+  echo hello | timeout 1 tail -s0.1 -f $inotify 2>tail.err
+  test $? = 124 || fail=1
+
   # `tail -F' must wait in any case.
 
   timeout 1 tail -s0.1 -F $inotify here 2>>tail.err
-- 
1.6.3.3