bug#27368: Minor concern: Confusing tail warning

2017-06-17 Thread Jim Meyering
On Sat, Jun 17, 2017 at 3:57 PM, Pádraig Brady  wrote:
> On 17/06/17 14:30, Pádraig Brady wrote:
>> On 17/06/17 07:35, Jim Meyering wrote:
>>> In this new function, please move the declaration of "i" into the for-loop:
>>>
>>> +static bool
>>> +any_non_regular (const struct File_spec *f, size_t n_files)
>>> +{
>>> +  size_t i;
>>> +
>>> +  for (i = 0; i < n_files; i++)
>
> I did that in about 100 instances at:
> http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=1379bdc

Nice! Thank you.





bug#27368: Minor concern: Confusing tail warning

2017-06-17 Thread Pádraig Brady
On 17/06/17 14:30, Pádraig Brady wrote:
> On 17/06/17 07:35, Jim Meyering wrote:
>> In this new function, please move the declaration of "i" into the for-loop:
>>
>> +static bool
>> +any_non_regular (const struct File_spec *f, size_t n_files)
>> +{
>> +  size_t i;
>> +
>> +  for (i = 0; i < n_files; i++)

I did that in about 100 instances at:
http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commitdiff;h=1379bdc

cheers,
Pádraig





bug#27368: Minor concern: Confusing tail warning

2017-06-17 Thread Pádraig Brady
On 17/06/17 07:35, Jim Meyering wrote:
> On Sat, Jun 17, 2017 at 1:32 AM, Pádraig Brady  wrote:
> ...
>> Two proposed patches for this are attached.
> 
> Nice fixes. Thank you!
> 
> In the NEWS addition:
> 
>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.

> Please don't use "will" in [NEWS].
> I.e., use the present tense instead. Referencing the future makes
> sense only now, prior to the release.
> 
> --
> In this new function, please move the declaration of "i" into the for-loop:
> 
> +static bool
> +any_non_regular (const struct File_spec *f, size_t n_files)
> +{
> +  size_t i;
> +
> +  for (i = 0; i < n_files; i++)
> 

Cool.
Thanks for the review





bug#27368: Minor concern: Confusing tail warning

2017-06-17 Thread Jim Meyering
On Sat, Jun 17, 2017 at 1:32 AM, Pádraig Brady  wrote:
...
> Two proposed patches for this are attached.

Nice fixes. Thank you!

In the NEWS addition:

   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.

Please don't use "will" in these blurbs.
I.e., use the present tense instead. Referencing the future makes
sense only now, prior to the release.

   tail -f now exits immediately when the output is piped
   and the reader of the pipe terminates.

   tail -f no longer erroneously warns about being ineffective
   when following a single tty, as the simple blocking loop used
   is effective in this case.

   tail -f /dev/tty is now supported by avoiding inotify when any
   non regular file is specified, as inotify is ineffective with these.
   [bug introduced with inotify support added in coreutils-7.5]

--
In this new function, please move the declaration of "i" into the for-loop:

+static bool
+any_non_regular (const struct File_spec *f, size_t n_files)
+{
+  size_t i;
+
+  for (i = 0; i < n_files; i++)





bug#27368: Minor concern: Confusing tail warning

2017-06-17 Thread Pádraig Brady
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?= 
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.