Subject: bug#27368: closed (Re: bug#27368: Minor concern: Confusing tail warning)

From: [email-masked] (GNU bug Tracking System)
Date: Sat, 17 Jun 2017 08:33:02 +0000

Your bug report

#27368: Minor concern: Confusing tail warning

which was filed against the coreutils package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
27368: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=27368
GNU Bug Tracking System
Contact [email-masked] with problems

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

Subject: Minor concern: Confusing tail warning
From: Charlie Hagedorn <[email-masked]>
To: [email-masked]
Date: Wed, 14 Jun 2017 16:03:07 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

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.

(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).

System is Debian Jessie -- coreutils v8.23-4.

Thanks!

Charlie