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 protected]>
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 protected]>
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
