Subject: Re: bug#27368: Minor concern: Confusing tail warning
From: Pádraig Brady <[email-masked]>
To: Charlie Hagedorn <[email-masked]>, [email-masked]
Date: Sat, 17 Jun 2017 01:32:34 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

On 15/06/17 02:40, Pádraig Brady wrote:
> On 14/06/17 16:03, Charlie Hagedorn wrote:
>> Hi!
>>
>> Thank you for maintaining such useful and reliable tools.
>>
>> Today I came across an unexpected warning in tail. The warning is intended
>> to handle this case:
>>
>> [:~]$ tail -f
>> tail: warning: following standard input indefinitely is ineffective
>>
>> which is both important and fun.
>>
>> Today, however, I was surprised to see it appear in this context:
>>
>> [:~]$ tail -f < /dev/ttyUSB0  > data.dat
>> tail: warning: following standard input indefinitely is ineffective
>>
>> The warning was confusing and perhaps inappropriate, as this call actually
>> *does* something, and is very effective at doing what I want; streaming the
>> port's output into data.dat.
> 
> Right. The above command will go into non inotify blocking mode
> and will thus work as you expect.  Note tail will not output
> anything until the first time read() returns nothing.
> The following patch should suppress the warning if we would be using this mode.
> 
> diff --git a/src/tail.c b/src/tail.c
> index 3918373..2f9b981 100644
> --- a/src/tail.c
> +++ b/src/tail.c
> @@ -2365,12 +2365,22 @@ main (int argc, char **argv)
>      if (found_hyphen && follow_mode == Follow_name)
>        die (EXIT_FAILURE, 0, _("cannot follow %s by name"), quoteaf ("-"));
> 
> -    /* When following forever, warn if any file is '-'.
> +    /* When following forever, and not using simple blocking, warn if
> +       any file is '-' as the stats() used to check for input are ineffective.
>         This is only a warning, since tail's output (before a failing seek,
>         and that from any non-stdin files) might still be useful.  */
> -    if (forever && found_hyphen && isatty (STDIN_FILENO))
> -      error (0, 0, _("warning: following standard input"
> -                     " indefinitely is ineffective"));
> +    if (forever && found_hyphen)
> +      {
> +        struct stat in_stat;
> +        bool blocking_stdin;
> +        blocking_stdin = (pid == 0 && follow_mode == Follow_descriptor
> +                          && n_files == 1 && ! fstat (STDIN_FILENO, &in_stat)
> +                          && ! S_ISREG (in_stat.st_mode));
> +
> +        if (! blocking_stdin && isatty (STDIN_FILENO))
> +          error (0, 0, _("warning: following standard input"
> +                         " indefinitely is ineffective"));
> +      }
>    }
> 
>> (Importantly, for reasons I don't yet understand, tail -f /dev/ttyUSB0 >
>> data.dat does not reliably tail the port; it redirects only one line of
>> output instead of a continuous stream).
> 
> That looks like another case where we should be disabling inotify.
> I.E. you can see this waits forever for inotify events if you hit ctrl-d a few times:
> 
>   strace tail -f /dev/tty
> 
> Ah yes that was discussed at http://bugs.gnu.org/21265
> I suppose we should improve things here with a couple of patches.
> 
> 1. Disable inotify if any non regular files (except fifo/pipe)
> (the kernel should really disallow this, but best handle I think)
> 
> 2. Generalise the warning in the above patch to be:
> if (n_files > 1 && any_device_or_tty)
>   printf ("warning: following a stdin/device in combination with other inputs is ineffective")

Two proposed patches for this are attached.

cheers,
Pádraig.

From 2a49ab8eae92859c02c0db7a14231b391287c5ee Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Thu, 15 Jun 2017 02:41:14 -0700
Subject: [PATCH 1/2] tail: with -f don't warn if doing a blocking read of a
 tty

* src/tail.c: (main): Only issue the warning about -f being
ineffective when we're not going into simple blocking mode.
* tests/tail-2/follow-stdin.sh: Ensure the warning is output correctly.
Fixes http://bugs.gnu.org/27368
---
 NEWS                         |  4 ++++
 src/tail.c                   | 18 ++++++++++++++----
 tests/tail-2/follow-stdin.sh | 21 ++++++++++++++++++++-
 3 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/NEWS b/NEWS
index d2672e8..22805c8 100644
--- a/NEWS
+++ b/NEWS
@@ -56,6 +56,10 @@ GNU coreutils NEWS                                    -*- outline -*-
   tail -f will now exit immediately if the output is piped
   and the reader of the pipe terminates.
 
+  tail -f will no longer erroneously warn about being ineffective
+  when following a single tty, as the simple blocking loop used
+  is effective in this case.
+
 
 * Noteworthy changes in release 8.27 (2017-03-08) [stable]
 
diff --git a/src/tail.c b/src/tail.c
index 3918373..2f9b981 100644
--- a/src/tail.c
+++ b/src/tail.c
@@ -2365,12 +2365,22 @@ main (int argc, char **argv)
     if (found_hyphen && follow_mode == Follow_name)
       die (EXIT_FAILURE, 0, _("cannot follow %s by name"), quoteaf ("-"));
 
-    /* When following forever, warn if any file is '-'.
+    /* When following forever, and not using simple blocking, warn if
+       any file is '-' as the stats() used to check for input are ineffective.
        This is only a warning, since tail's output (before a failing seek,
        and that from any non-stdin files) might still be useful.  */
-    if (forever && found_hyphen && isatty (STDIN_FILENO))
-      error (0, 0, _("warning: following standard input"
-                     " indefinitely is ineffective"));
+    if (forever && found_hyphen)
+      {
+        struct stat in_stat;
+        bool blocking_stdin;
+        blocking_stdin = (pid == 0 && follow_mode == Follow_descriptor
+                          && n_files == 1 && ! fstat (STDIN_FILENO, &in_stat)
+                          && ! S_ISREG (in_stat.st_mode));
+
+        if (! blocking_stdin && isatty (STDIN_FILENO))
+          error (0, 0, _("warning: following standard input"
+                         " indefinitely is ineffective"));
+      }
   }
 
   /* Don't read anything if we'll never output anything.  */
diff --git a/tests/tail-2/follow-stdin.sh b/tests/tail-2/follow-stdin.sh
index 7e82835..c922ea1 100755
--- a/tests/tail-2/follow-stdin.sh
+++ b/tests/tail-2/follow-stdin.sh
@@ -1,5 +1,5 @@
 #!/bin/sh
-# tail -f - would fail with the initial inotify implementation
+# Test tail -f -
 
 # Copyright (C) 2009-2017 Free Software Foundation, Inc.
 
@@ -19,6 +19,9 @@
 . "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
 print_ver_ tail
 
+#################
+# tail -f - would fail with the initial inotify implementation
+
 check_tail_output()
 {
   local delay="$1"
@@ -51,6 +54,7 @@ for mode in '' '---disable-inotify'; do
 done

+#################
 # Before coreutils-8.26 this would induce an UMR under UBSAN
 returns_ 1 timeout 10 tail -f - <&- 2>errt || fail=1
 cat <<\EOF >exp || framework_failure_
@@ -63,4 +67,19 @@ sed 's/\(tail:.*\):.*/\1/' errt > err || framework_failure_
 compare exp err || fail=1

+#################
+# Before coreutils-8.28 this would erroneously issue a warning
+if tty -s </dev/tty && test -t 0; then
+  for input in '' '-' '/dev/tty'; do
+    returns_ 124 timeout 0.1 tail -f $input 2>err || fail=1
+    compare /dev/null err || fail=1
+  done
+
+  tail -f - /dev/null </dev/tty 2> out & pid=$!
+  # Wait up to 12.7s for output to appear:
+  tail_re='ineffective' retry_delay_ check_tail_output .1 7 ||
+    { cat out; fail=1; }
+  cleanup_
+fi
+
 Exit $fail
-- 
2.9.3

From 9066e57d421935747a4e99460478ec71b3191452 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Fri, 16 Jun 2017 23:50:47 -0700
Subject: [PATCH 2/2] tail: only use inotify with regular files

* src/tail.c (any_non_regular): A new function to check passed files.
(main): Use the above to skip inotify if any non regular files passed
like /dev/tty or /dev/ttyUSB0 etc.
* tests/tail-2/inotify-only-regular.sh: A new test.
* tests/local.mk: Reference the new test.
* NEWS: Mention the bug fix.
Fixes http://bugs.gnu.org/21265 and http://bugs.gnu.org/27368
---
 NEWS                                 |  4 ++++
 src/tail.c                           | 17 +++++++++++++++++
 tests/local.mk                       |  1 +
 tests/tail-2/inotify-only-regular.sh | 32 ++++++++++++++++++++++++++++++++
 4 files changed, 54 insertions(+)
 create mode 100755 tests/tail-2/inotify-only-regular.sh

diff --git a/NEWS b/NEWS
index 22805c8..86e6bc7 100644
--- a/NEWS
+++ b/NEWS
@@ -36,6 +36,10 @@ GNU coreutils NEWS                                    -*- outline -*-
   or ignored until future events on the monitored files.
   [bug introduced with inotify support added in coreutils-7.5]
 
+  tail -f /dev/tty is now supported by not using inotify when any
+  non regular files are specified, as inotify is ineffective with these.
+  [bug introduced with inotify support added in coreutils-7.5]
+
   uptime no longer outputs the AM/PM component of the current time,
   as that's inconsistent with the 24 hour time format used.
   [bug introduced in coreutils-7.0]
diff --git a/src/tail.c b/src/tail.c
index 2f9b981..b8be1d2 100644
--- a/src/tail.c
+++ b/src/tail.c
@@ -1339,6 +1339,22 @@ any_symlinks (const struct File_spec *f, size_t n_files)
   return false;
 }
 
+/* Return true if any of the N_FILES files in F is not
+   a regular file.  This is used to avoid adding inotify
+   watches on a device file for example, which inotify
+   will accept, but not give any events for.  */
+
+static bool
+any_non_regular (const struct File_spec *f, size_t n_files)
+{
+  size_t i;
+
+  for (i = 0; i < n_files; i++)
+    if (0 <= f[i].fd && ! S_ISREG (f[i].mode))
+      return true;
+  return false;
+}
+
 /* Return true if any of the N_FILES files in F represents
    stdin and is tailable.  */
 
@@ -2457,6 +2473,7 @@ main (int argc, char **argv)
                                || any_remote_file (F, n_files)
                                || ! any_non_remote_file (F, n_files)
                                || any_symlinks (F, n_files)
+                               || any_non_regular (F, n_files)
                                || (!ok && follow_mode == Follow_descriptor)))
         disable_inotify = true;
 
diff --git a/tests/local.mk b/tests/local.mk
index fdf3edf..6112e88 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -177,6 +177,7 @@ all_tests =					\
   tests/tail-2/inotify-rotate.sh		\
   tests/tail-2/inotify-rotate-resources.sh	\
   tests/tail-2/inotify-dir-recreate.sh		\
+  tests/tail-2/inotify-only-regular.sh		\
   tests/chmod/no-x.sh				\
   tests/chgrp/basic.sh				\
   tests/rm/dangling-symlink.sh			\
diff --git a/tests/tail-2/inotify-only-regular.sh b/tests/tail-2/inotify-only-regular.sh
new file mode 100755
index 0000000..4d106fb
--- /dev/null
+++ b/tests/tail-2/inotify-only-regular.sh
@@ -0,0 +1,32 @@
+#!/bin/sh
+# ensure that tail -f only uses inotify for regular files
+
+# Copyright (C) 2017 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/>.
+
+. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
+print_ver_ tail
+
+grep '^#define HAVE_INOTIFY 1' "$CONFIG_HEADER" >/dev/null \
+  || skip_ 'inotify support required'
+
+require_strace_ 'inotify_add_watch'
+
+returns_ 124 timeout .1 strace -e inotify_add_watch -o strace.out \
+  tail -f /dev/null || fail=1
+
+grep 'inotify' strace.out && fail=1
+
+Exit $fail
-- 
2.9.3