On 04/04/2013 03:42 AM, Pádraig Brady wrote:
> I've nothing against simplicity though since the
> inotify code already deals with missing files when
> following by name, by watching the parent dir,
> it might be easy to adjust to handle this case.
Hi Padraig,
sorry for the late reply.
Somehow, I don't have a good feeling for using inotify in the
--follow=descriptor case because inotify works with file names.
Once tail opens the file successfully after inotify says it's
there, tail would have to go back to polling, because all further
inotify events are only relevant for the file name, not the
descriptor.
>From playing with the inotify code, I even had the impression that
there are not all events delivered to tail reliably. Is this true?
> If not I'm fine for reverting to polling.
>
>> @ -2189,7 +2192,8 @@ main (int argc, char **argv)
>> is recreated, then we don't recheck any new file when
>> follow_mode == Follow_name */
>> if (!disable_inotify && (tailable_stdin (F, n_files)
>> - || any_remote_file (F, n_files)))
>> + || any_remote_file (F, n_files)
>
>> + || !ok))
>
> I think that needs to be:
> || (!ok && follow_mode == Follow_descriptor)))
>
>> disable_inotify = true;
>>
>> if (!disable_inotify)
Good point, thanks!
>>>> Furthermore, I find this warning irritating
>>>> "warning: --retry is useful mainly when following by name"
>>>
>>> I agree. It's not imparting much info, about why that's supported.
>>> What we should do is print what tail will do in this case.
>>> I propose we do:
>>>
>>> if (retry)
>>> if (!follow_mode)
>>> warn ("--retry ignored");
>>> else if (follow_mode == DESC)
>>> warn ("--retry only effective for the initial open");
>>
>> I'd go even one step further, and also remove the latter warning.
>
> I'd be 60:40 for keeping the warning as users could
> easily specify 'tail -f --retry' not realizing it
> wouldn't handle rotated log files for example.
I agree.
>> +# Ensure that "tail --retry --follow=descriptor" waits for the file to
>> appear.
>> +# tail-8.21 failed at this (since the implementation of the inotify
>> support).
>> +tail -s.1 --follow=descriptor --retry missing >out 2>&1 & pid=$!
>> +sleep .2 # Wait a bit ...
>> +echo "X" > missing # ... for the file to appear.
>> +sleep .2 # Then give tail enough time to recognize it
>> + # ... and to output the content to out.
>> +kill $pid # Then kill tail ...
>> +wait $pid # ... and wait for it to complete.
>> +rm -f missing || fail=1
>> +# Expect 3 lines in the output file ("cannot open" + "has appeared" + "X").
>> +[ $( wc -l < out ) = 3 ] || { fail=1; cat out; }
>> +grep -F 'cannot open' out || { fail=1; cat out; }
>> +grep -F 'has appeared' out || { fail=1; cat out; }
>> +grep '^X$' out || { fail=1; cat out; }
>
> This looks racy. There is nothing I see that guarantees that
> tail will be scheduled before it's killed.
Good spot.
> I'd maybe try something like:
>
> timeout 10 tail .....
> until [ $(wc -l < out) = 3 ]; do sleep .1; done
> kill $pid
> wait $pid
Additionally to the above, I noticed that tail now would no longer
exit in the case the file appears untailable, e.g. when it appears
as directory. I fixed this, too.
After all, I think tail.c is not in a very good, maintainable shape.
There are so many knobs, partially with overlapping meaning like
'follow_mode' vs. 'forever', or 'reopen_inaccessible_files' vs.
f[i].ignore etc., that it's rather hard to do the right change
without breaking something else.
WDYT?
Have a nice day,
Berny
>From 1f1237ec3201cda16c98611e3c9ebc4541748cac Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <[email protected]>
Date: Wed, 17 Apr 2013 15:04:48 +0200
Subject: [PATCH 1/2] tail: let -f --retry wait for inaccessible files
The --retry option is indeed useful for both following modes
by name and by file descriptor. The difference is that in the
latter case, it is effective only during the initial open.
As a regression of the implementation of the inotify support,
tail -f --retry would immediately exit if the given file is
inaccessible.
* src/tail.c (usage): Change the description of the --retry option:
remove the note that this option would mainly be useful when
following by name.
(main): Change diagnosing dubios uses of --retry option:
when the --retry option is used without following, then issue
a warning that this option is ignored; when it is used together
with --follow=descriptor, then issue a warning that it is only
effective for the initial open.
Disable inotify also in the case when the initial open in tail_file()
failed (which is the actual bug fix).
* tests/tail-2/retry.sh: Add new tests.
* tests/local.mk (all_tests): Mention it.
* doc/coreutils.texi (tail invocation): Enhance the documentation
of the --retry option. Clarify the difference in tail's behavior
regarding the --retry option when combined with the following modes
name versus descriptor.
* NEWS (Bug fixes): Mention the fix.
Reported by Noel Morrison in:
http://lists.gnu.org/archive/html/coreutils/2013-04/msg00003.html
---
NEWS | 5 +++
doc/coreutils.texi | 14 ++++++--
src/tail.c | 21 +++++++----
tests/local.mk | 1 +
tests/tail-2/retry.sh | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++
5 files changed, 128 insertions(+), 9 deletions(-)
create mode 100644 tests/tail-2/retry.sh
diff --git a/NEWS b/NEWS
index 0db53d7..fc05b8a 100644
--- a/NEWS
+++ b/NEWS
@@ -13,6 +13,11 @@ GNU coreutils NEWS -*- outline -*-
the relative link on the dereferenced path of an existing link.
[This bug was introduced when --relative was added in coreutils-8.16.]
+ tail --retry -f now waits for the files specified to appear. Before, tail
+ would immediately exit when such a file is inaccessible during the initial
+ open.
+ [This bug was introduced when inotify support was added in coreutils-7.5]
+
** New features
join accepts a new option: --zero-terminated (-z). As with the sort,uniq
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 92917f1..f6f2eb4 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -3155,9 +3155,17 @@ will keep trying until it becomes accessible again.
@item --retry
@opindex --retry
-This option is useful mainly when following by name (i.e., with
-@option{--follow=name}).
-Without this option, when tail encounters a file that doesn't
+Indefinitely try to open the specified file.
+This option is useful mainly when following (and otherwise issues a warning).
+
+When following by file descriptor (i.e., with @option{--follow=descriptor}),
+this option only affects the initial open of the file, as after a successful
+open, @command{tail} will start following the file descriptor.
+
+When following by name (i.e., with @option{--follow=name}), @command{tail}
+infinitely retries to re-open the given files until killed.
+
+Without this option, when @command{tail} encounters a file that doesn't
exist or is otherwise inaccessible, it reports that fact and
never checks it again.
diff --git a/src/tail.c b/src/tail.c
index cdaecdd..3735757 100644
--- a/src/tail.c
+++ b/src/tail.c
@@ -294,9 +294,7 @@ With no FILE, or when FILE is -, read standard input.\n\
fputs (_("\
--pid=PID with -f, terminate after process ID, PID dies\n\
-q, --quiet, --silent never output headers giving file names\n\
- --retry keep trying to open a file even when it is or\n\
- becomes inaccessible; useful when following by\n\
- name, i.e., with --follow=name\n\
+ --retry keep trying to open a file if it is inaccessible\n\
"), stdout);
fputs (_("\
-s, --sleep-interval=N with -f, sleep for approximately N seconds\n\
@@ -2030,8 +2028,14 @@ parse_options (int argc, char **argv,
}
}
- if (reopen_inaccessible_files && follow_mode != Follow_name)
- error (0, 0, _("warning: --retry is useful mainly when following by name"));
+ if (reopen_inaccessible_files)
+ {
+ if (!forever)
+ error (0, 0, _("warning: --retry ignored; --retry is useful"
+ " only when following"));
+ else if (follow_mode == Follow_descriptor)
+ error (0, 0, _("warning: --retry only effective for the initial open"));
+ }
if (pid && !forever)
error (0, 0,
@@ -2182,6 +2186,10 @@ main (int argc, char **argv)
in this case because it would miss any updates to the file
that were not initiated from the local system.
+ ok is false when one of the files specified could not be opened for
+ reading. In this case and when following by descriptor,
+ tail_forever_inotify() cannot be used (in its current implementation).
+
FIXME: inotify doesn't give any notification when a new
(remote) file or directory is mounted on top a watched file.
When follow_mode == Follow_name we would ideally like to detect that.
@@ -2193,7 +2201,8 @@ main (int argc, char **argv)
is recreated, then we don't recheck any new file when
follow_mode == Follow_name */
if (!disable_inotify && (tailable_stdin (F, n_files)
- || any_remote_file (F, n_files)))
+ || any_remote_file (F, n_files)
+ || (!ok && follow_mode == Follow_descriptor)))
disable_inotify = true;
if (!disable_inotify)
diff --git a/tests/local.mk b/tests/local.mk
index 0b019d9..f47da8d 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -390,6 +390,7 @@ all_tests = \
tests/misc/uniq-perf.sh \
tests/misc/xattr.sh \
tests/tail-2/wait.sh \
+ tests/tail-2/retry.sh \
tests/chmod/c-option.sh \
tests/chmod/equal-x.sh \
tests/chmod/equals.sh \
diff --git a/tests/tail-2/retry.sh b/tests/tail-2/retry.sh
new file mode 100644
index 0000000..dc5218f
--- /dev/null
+++ b/tests/tail-2/retry.sh
@@ -0,0 +1,96 @@
+#!/bin/sh
+# Exercise tail's behavior regarding missing files with/without --retry.
+
+# Copyright (C) 2013 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
+
+# Function to wait until a given number of lines appears in 'out',
+# but timeout after 5 seconds.
+wait4lines_ ()
+{
+ local lc="$1" # expected line count
+ for i in $( seq 50 ) ; do
+ [ $( wc -l < out ) -ge "$lc" ] && return 0
+ sleep .1
+ done
+ return 1
+}
+
+# === Test:
+# Retry without --follow results in a warning.
+touch file
+tail --retry file > out 2>&1 || fail=1
+[ $( wc -l < out ) = 1 ] || fail=1
+grep -F 'tail: warning: --retry ignored' out || fail=1
+
+# === Test:
+# The same with a missing file: expect error message and exit 1.
+tail --retry missing > out 2>&1 && fail=1
+[ $( wc -l < out ) = 2 ] || fail=1
+grep -F 'tail: warning: --retry ignored' out || fail=1
+
+# === Test:
+# Ensure that "tail --retry --follow=name" waits for the file to appear.
+timeout 10 tail -s.1 --follow=name --retry missing >out 2>&1 & pid=$!
+wait4lines_ 1 || fail=1 # Wait for "cannot open" error.
+echo "X" > missing # Create the 'missing' file with some content.
+wait4lines_ 3 || fail=1 # Wait for the expected output.
+kill $pid
+wait $pid
+# Expect 3 lines in the output file.
+[ $( wc -l < out ) = 3 ] || { fail=1; cat out; }
+grep -F 'cannot open' out || { fail=1; cat out; }
+grep -F 'has become accessible' out || { fail=1; cat out; }
+grep '^X$' out || { fail=1; cat out; }
+rm -f missing out || fail=1
+
+# === Test:
+# Ensure that "tail --retry --follow=descriptor" waits for the file to appear.
+# tail-8.21 failed at this (since the implementation of the inotify support).
+timeout 10 tail -s.1 --follow=descriptor --retry missing >out 2>&1 & pid=$!
+wait4lines_ 2 || fail=1 # Wait for "cannot open" error.
+echo "X" > missing # Create the 'missing' file with some content.
+wait4lines_ 4 || fail=1 # Wait for the expected output.
+kill $pid
+wait $pid
+# Expect 4 lines in the output file.
+[ $( wc -l < out ) = 4 ] || { fail=1; cat out; }
+grep -F 'retry only effective for the initial open' out \
+ || { fail=1; cat out; }
+grep -F 'cannot open' out || { fail=1; cat out; }
+grep -F 'has appeared' out || { fail=1; cat out; }
+grep '^X$' out || { fail=1; cat out; }
+rm -f missing out || fail=1
+
+# === Test:
+# Ensure that --follow=descriptor (without --retry) does *not wait* for the
+# file to appear. Expect 2 lines in the output file ("cannot open" +
+# "no files remaining") and exit status 1.
+tail --follow=descriptor missing >out 2>&1 && fail=1
+[ $( wc -l < out ) = 2 ] || { fail=1; cat out; }
+grep -F 'cannot open' out || { fail=1; cat out; }
+grep -F 'no files remaining' out || { fail=1; cat out; }
+
+# === Test:
+# Likewise for --follow=name (without --retry).
+tail --follow=name missing >out 2>&1 && fail=1
+[ $( wc -l < out ) = 2 ] || { fail=1; cat out; }
+grep -F 'cannot open' out || { fail=1; cat out; }
+grep -F 'no files remaining' out || { fail=1; cat out; }
+
+Exit $fail
--
1.8.1.3.619.g7b6e784
>From 0385077afb24e269eaf8375c6c6bb3524a64095c Mon Sep 17 00:00:00 2001
From: Bernhard Voelker <[email protected]>
Date: Wed, 17 Apr 2013 15:30:46 +0200
Subject: [PATCH 2/2] tail: exit following by descriptor when no tailable file
left
As a side effect of the previous commit which fixes 'tail -f --retry'
to wait for a file to appear, tail would not exit when the last file
appears untailable and gives up on this file.
This can happen, for example, when the argument file name appears
as directory. Tail sets the 'ignore' flag of this file to true,
but instead of exiting the program, tail would continue the loop.
* src/tail.c (any_live_files): Change the function to return true
if any of the files is still tailable or if tail should continue to
try to check again.
(tail_forever): Change the condition to break the loop in the
"no files remaining" case, because now any_live_files() will care
about it, as mentioned above.
(parse_options): When --retry is used without any follow mode,
then reset reopen_inaccessible_files to false.
* tests/tail-2/retry.sh: Add test case.
---
src/tail.c | 31 +++++++++++++++++++++++++------
tests/tail-2/retry.sh | 18 ++++++++++++++++++
2 files changed, 43 insertions(+), 6 deletions(-)
diff --git a/src/tail.c b/src/tail.c
index 3735757..6bbc725 100644
--- a/src/tail.c
+++ b/src/tail.c
@@ -1052,16 +1052,32 @@ recheck (struct File_spec *f, bool blocking)
}
/* Return true if any of the N_FILES files in F are live, i.e., have
- open file descriptors. */
+ open file descriptors, or should be checked again (see --retry).
+ When following descriptors, checking should only continue when any
+ of the files is not yet ignored. */
static bool
any_live_files (const struct File_spec *f, size_t n_files)
{
size_t i;
+ if (reopen_inaccessible_files && follow_mode == Follow_name)
+ return true; /* continue following for -F option */
+
for (i = 0; i < n_files; i++)
- if (0 <= f[i].fd)
- return true;
+ {
+ if (0 <= f[i].fd)
+ {
+ return true;
+ }
+ else
+ {
+ if (reopen_inaccessible_files && follow_mode == Follow_descriptor)
+ if (! f[i].ignore)
+ return true;
+ }
+ }
+
return false;
}
@@ -1189,7 +1205,7 @@ tail_forever (struct File_spec *f, size_t n_files, double sleep_interval)
f[i].size += bytes_read;
}
- if (! any_live_files (f, n_files) && ! reopen_inaccessible_files)
+ if (! any_live_files (f, n_files))
{
error (0, 0, _("no files remaining"));
break;
@@ -2031,8 +2047,11 @@ parse_options (int argc, char **argv,
if (reopen_inaccessible_files)
{
if (!forever)
- error (0, 0, _("warning: --retry ignored; --retry is useful"
- " only when following"));
+ {
+ reopen_inaccessible_files = false;
+ error (0, 0, _("warning: --retry ignored; --retry is useful"
+ " only when following"));
+ }
else if (follow_mode == Follow_descriptor)
error (0, 0, _("warning: --retry only effective for the initial open"));
}
diff --git a/tests/tail-2/retry.sh b/tests/tail-2/retry.sh
index dc5218f..805b93f 100644
--- a/tests/tail-2/retry.sh
+++ b/tests/tail-2/retry.sh
@@ -78,6 +78,24 @@ grep '^X$' out || { fail=1; cat out; }
rm -f missing out || fail=1
# === Test:
+# Ensure that tail --follow=descriptor --retry exits when the file appears
+# untailable. Expect exit status 1.
+timeout 10 tail -s.1 --follow=descriptor --retry missing >out 2>&1 & pid=$!
+wait4lines_ 2 || fail=1 # Wait for "cannot open" error.
+mkdir missing # Create an untailable 'missing' directory.
+wait4lines_ 4 || fail=1 # Wait for the expected output.
+wait $pid
+rc=$?
+[ $( wc -l < out ) = 4 ] || { fail=1; cat out; }
+grep -F 'retry only effective for the initial open' out \
+ || { fail=1; cat out; }
+grep -F 'cannot open' out || { fail=1; cat out; }
+grep -F 'replaced with an untailable file' out || { fail=1; cat out; }
+grep -F 'no files remaining' out || { fail=1; cat out; }
+[ $rc = 1 ] || { fail=1; cat out; }
+rm -fd missing out || fail=1
+
+# === Test:
# Ensure that --follow=descriptor (without --retry) does *not wait* for the
# file to appear. Expect 2 lines in the output file ("cannot open" +
# "no files remaining") and exit status 1.
--
1.8.1.3.619.g7b6e784
