On 09/06/15 07:20, Bernhard Voelker wrote:
> Nice one, but I think the fix is not sufficient ... see below.
>
> On 06/09/2015 02:54 AM, Pádraig Brady wrote:
>> diff --git a/src/tail.c b/src/tail.c
>> index c9736ca..047d93c 100644
>> --- a/src/tail.c
>> +++ b/src/tail.c
>> @@ -1046,7 +1046,9 @@ recheck (struct File_spec *f, bool blocking)
>> {
>> /* This happens when one iteration finds the file missing,
>> then the preceding <dev,inode> pair is reused as the
>> - file is recreated. */
>> + file is recreated. XXX: This means the file is redisplayed
>> + in --follow=name mode if renamed away from and back to
>> + a monitored name. */
>> new_file = true;
>> }
>> else
>
> s/XXX: // ?
>
>> diff --git a/tests/tail-2/F-headers.sh b/tests/tail-2/F-headers.sh
>> new file mode 100755
>> index 0000000..9355c0c
>> --- /dev/null
>> +++ b/tests/tail-2/F-headers.sh
>> @@ -0,0 +1,59 @@
>> +#!/bin/sh
>> +# Ensure tail -F distinguishes output with the correct headers
>> +# Between coreutils 7.5 and 8.23 inclusive, 'tail -F ...' would
>> +# not output headers for cor created/renamed files in certain cases.
>
> s/ cor / /
>
>> +for mode in '' '---disable-inotify'; do
>> + rm -f a b out
>> +
>> + tail $mode -F $fastpoll a b > out 2>&1 & pid=$!
>> +
>> + # Wait up to 12.7s for tail to start.
>> + tail_re="cannot open 'b'" retry_delay_ check_tail_output .1 7 ||
>> + { cat out; fail=1; }
>> +
>> + echo x > a
>> + # Wait up to 12.7s for a's header to appear in the output:
>> + tail_re='==> a <==' retry_delay_ check_tail_output .1 7 ||
>> + { echo "$0: a: unexpected delay?"; cat out; fail=1; }
>> +
>> + echo y > b
>> + # Wait up to 12.7s for a's header to appear in the output:
>> + tail_re='==> b <==' retry_delay_ check_tail_output .1 7 ||
>> + { echo "$0: a: unexpected delay?"; cat out; fail=1; }
>
> s/ a: / b: /
and s/a's/b's/
> While this patch fixes the outputting of the header for multiple
> files, it does not fix the same issue for the same file, i.e. the
> "==> file <==" header is missing when a file re-appears the 2nd
> time. Reproducer:
>
> rm -f a b out
> src/tail -F a b > out 2>&1 &
>
> sleep 2; echo a > a
>
> sleep 2; rm a
>
> sleep 2; echo a > a
>
> sleep 2; kill $!
>
> Output:
>
> src/tail: cannot open ‘a’ for reading: No such file or directory
> src/tail: cannot open ‘b’ for reading: No such file or directory
> src/tail: ‘a’ has appeared; following new file
> ==> a <==
> a
> src/tail: ‘a’ has been replaced; following new file
> a
>
> Shouldn't tail(1) output the "==> a <==" header before the last
> line here, too?
Well the data being output by tail is logically connected,
even though the files are being rotated underneath.
So while we could add a redisplay flag to the fspec struct
to enable showing a new header upon truncation or creation,
it's probably best not to.
Now we should always output diagnostics to stderr in these cases,
and I see we don't when inodes are reused (which is common).
So I've a attached a further patch to improve that as well.
thanks for the careful review!
Pádraig.
From 6878ae412d8e7fb65ad77a3d68af986377d9d977 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]>
Date: Tue, 9 Jun 2015 00:53:35 +0100
Subject: [PATCH 1/2] tail: display file headers correctly with inotify
* src/tail.c (tail_forever_inotify): Use the fspec pointer to
distinguish previously output files, rather than a descriptor
from the inotify event. That event descriptor was that of
the parent directory when files were created or renamed etc.
(check_fspec): Adjust for the new comparison. Also show the
header when the file is truncated, since we show data
in this case also.
* tests/tail-2/F-headers.sh: A new test case.
* tests/local.mk: Reference the new test.
* NEWS: Mention the bug fix.
---
NEWS | 4 ++++
src/tail.c | 22 ++++++++++--------
tests/local.mk | 1 +
tests/tail-2/F-headers.sh | 59 +++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 76 insertions(+), 10 deletions(-)
create mode 100755 tests/tail-2/F-headers.sh
diff --git a/NEWS b/NEWS
index c2576c5..4c0f6e4 100644
--- a/NEWS
+++ b/NEWS
@@ -56,6 +56,10 @@ GNU coreutils NEWS -*- outline -*-
tail --follow consistently outputs all data for a truncated file.
[bug introduced in the beginning]
+ tail --follow=name correctly outputs headers for multiple files
+ when those files are being created or renamed.
+ [bug introduced in coreutils-7.5]
+
** New features
chroot accepts the new --skip-chdir option to not change the working directory
diff --git a/src/tail.c b/src/tail.c
index c9736ca..4d6b28a 100644
--- a/src/tail.c
+++ b/src/tail.c
@@ -1046,7 +1046,9 @@ recheck (struct File_spec *f, bool blocking)
{
/* This happens when one iteration finds the file missing,
then the preceding <dev,inode> pair is reused as the
- file is recreated. */
+ file is recreated. Note this also means the file is redisplayed
+ in --follow=name mode if renamed away from and back to
+ a monitored name. */
new_file = true;
}
else
@@ -1321,7 +1323,7 @@ wd_comparator (const void *e1, const void *e2)
/* Output (new) data for FSPEC->fd. */
static void
-check_fspec (struct File_spec *fspec, int wd, int *prev_wd)
+check_fspec (struct File_spec *fspec, struct File_spec **prev_fspec)
{
struct stat stats;
char const *name;
@@ -1347,7 +1349,6 @@ check_fspec (struct File_spec *fspec, int wd, int *prev_wd)
if (S_ISREG (fspec->mode) && stats.st_size < fspec->size)
{
error (0, 0, _("%s: file truncated"), name);
- *prev_wd = wd;
xlseek (fspec->fd, 0, SEEK_SET, name);
fspec->size = 0;
}
@@ -1355,11 +1356,11 @@ check_fspec (struct File_spec *fspec, int wd, int *prev_wd)
&& timespec_cmp (fspec->mtime, get_stat_mtime (&stats)) == 0)
return;
- if (wd != *prev_wd)
+ if (fspec != *prev_fspec)
{
if (print_headers)
write_header (name);
- *prev_wd = wd;
+ *prev_fspec = fspec;
}
uintmax_t bytes_read = dump_remainder (name, fspec->fd, COPY_TO_EOF);
@@ -1391,7 +1392,7 @@ tail_forever_inotify (int wd, struct File_spec *f, size_t n_files,
bool found_unwatchable_dir = false;
bool no_inotify_resources = false;
bool writer_is_dead = false;
- int prev_wd;
+ struct File_spec *prev_fspec;
size_t evlen = 0;
char *evbuf;
size_t evbuf_off = 0;
@@ -1492,7 +1493,7 @@ tail_forever_inotify (int wd, struct File_spec *f, size_t n_files,
if (follow_mode == Follow_descriptor && !found_watchable_file)
return false;
- prev_wd = f[n_files - 1].wd;
+ prev_fspec = &(f[n_files - 1]);
/* Check files again. New files or data can be available since last time we
checked and before they are watched by inotify. */
@@ -1524,7 +1525,7 @@ tail_forever_inotify (int wd, struct File_spec *f, size_t n_files,
}
/* check for new data. */
- check_fspec (&f[i], f[i].wd, &prev_wd);
+ check_fspec (&f[i], &prev_fspec);
}
}
@@ -1703,7 +1704,7 @@ tail_forever_inotify (int wd, struct File_spec *f, size_t n_files,
continue;
}
- check_fspec (fspec, ev->wd, &prev_wd);
+ check_fspec (fspec, &prev_fspec);
}
}
#endif
@@ -2316,7 +2317,8 @@ main (int argc, char **argv)
FIXME-maybe: inotify has a watch descriptor per inode, and hence with
our current hash implementation will only --follow data for one
- of the names when multiple hardlinked files are specified. */
+ of the names when multiple hardlinked files are specified, or
+ for one name when a name is specified multiple times. */
if (!disable_inotify && (tailable_stdin (F, n_files)
|| any_remote_file (F, n_files)
|| any_symlinks (F, n_files)
diff --git a/tests/local.mk b/tests/local.mk
index bb78796..7b8c91e 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -172,6 +172,7 @@ all_tests = \
tests/tail-2/inotify-hash-abuse2.sh \
tests/tail-2/F-vs-missing.sh \
tests/tail-2/F-vs-rename.sh \
+ tests/tail-2/F-headers.sh \
tests/tail-2/descriptor-vs-rename.sh \
tests/tail-2/inotify-rotate.sh \
tests/tail-2/inotify-rotate-resources.sh \
diff --git a/tests/tail-2/F-headers.sh b/tests/tail-2/F-headers.sh
new file mode 100755
index 0000000..31d0fa6
--- /dev/null
+++ b/tests/tail-2/F-headers.sh
@@ -0,0 +1,59 @@
+#!/bin/sh
+# Ensure tail -F distinguishes output with the correct headers
+# Between coreutils 7.5 and 8.23 inclusive, 'tail -F ...' would
+# not output headers for or created/renamed files in certain cases.
+
+# Copyright (C) 2015 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
+
+check_tail_output()
+{
+ local delay="$1"
+ grep "$tail_re" out ||
+ { sleep $delay; return 1; }
+}
+
+# Terminate any background tail process
+cleanup_() { kill $pid 2>/dev/null && wait $pid; }
+
+# Speedup the non inotify case
+fastpoll='-s.1 --max-unchanged-stats=1'
+
+for mode in '' '---disable-inotify'; do
+ rm -f a b out
+
+ tail $mode -F $fastpoll a b > out 2>&1 & pid=$!
+
+ # Wait up to 12.7s for tail to start.
+ tail_re="cannot open 'b'" retry_delay_ check_tail_output .1 7 ||
+ { cat out; fail=1; }
+
+ echo x > a
+ # Wait up to 12.7s for a's header to appear in the output:
+ tail_re='==> a <==' retry_delay_ check_tail_output .1 7 ||
+ { echo "$0: a: unexpected delay?"; cat out; fail=1; }
+
+ echo y > b
+ # Wait up to 12.7s for b's header to appear in the output:
+ tail_re='==> b <==' retry_delay_ check_tail_output .1 7 ||
+ { echo "$0: b: unexpected delay?"; cat out; fail=1; }
+
+ cleanup_
+done
+
+Exit $fail
--
2.4.1
From aee982627874e9719bc151007b7e0144edd3963e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]>
Date: Tue, 9 Jun 2015 11:33:44 +0100
Subject: [PATCH 2/2] tail: display consistent diagnostics upon file
replacement
* src/tail.c (recheck): Display diagnostices for replaced files
even with reused inodes which is a common case.
* tests/tail-2/F-vs-missing.sh: Use correct diagnostic in comment.
* tests/tail-2/F-vs-rename.sh: Likewise.
---
src/tail.c | 55 ++++++++++++++++++++------------------------
tests/tail-2/F-vs-missing.sh | 2 +-
tests/tail-2/F-vs-rename.sh | 2 +-
3 files changed, 27 insertions(+), 32 deletions(-)
diff --git a/src/tail.c b/src/tail.c
index 4d6b28a..c062d40 100644
--- a/src/tail.c
+++ b/src/tail.c
@@ -1019,42 +1019,37 @@ recheck (struct File_spec *f, bool blocking)
assert (f->fd == -1);
error (0, 0, _("%s has become accessible"), quote (pretty_name (f)));
}
+ else if (f->fd == -1)
+ {
+ /* A new file even when inodes haven't changed as <dev,inode>
+ pairs can be reused, and we know the file was missing
+ on the previous iteration. Note this also means the file
+ is redisplayed in --follow=name mode if renamed away from
+ and back to a monitored name. */
+ new_file = true;
+
+ error (0, 0,
+ _("%s has appeared; following new file"),
+ quote (pretty_name (f)));
+ }
else if (f->ino != new_stats.st_ino || f->dev != new_stats.st_dev)
{
+ /* File has been replaced (e.g., via log rotation) --
+ tail the new one. */
new_file = true;
- if (f->fd == -1)
- {
- error (0, 0,
- _("%s has appeared; following new file"),
- quote (pretty_name (f)));
- }
- else
- {
- /* Close the old one. */
- close_fd (f->fd, pretty_name (f));
-
- /* File has been replaced (e.g., via log rotation) --
- tail the new one. */
- error (0, 0,
- _("%s has been replaced; following new file"),
- quote (pretty_name (f)));
- }
+
+ error (0, 0,
+ _("%s has been replaced; following new file"),
+ quote (pretty_name (f)));
+
+ /* Close the old one. */
+ close_fd (f->fd, pretty_name (f));
+
}
else
{
- if (f->fd == -1)
- {
- /* This happens when one iteration finds the file missing,
- then the preceding <dev,inode> pair is reused as the
- file is recreated. Note this also means the file is redisplayed
- in --follow=name mode if renamed away from and back to
- a monitored name. */
- new_file = true;
- }
- else
- {
- close_fd (fd, pretty_name (f));
- }
+ /* No changes detected, so close new fd. */
+ close_fd (fd, pretty_name (f));
}
/* FIXME: When a log is rotated, daemons tend to log to the
diff --git a/tests/tail-2/F-vs-missing.sh b/tests/tail-2/F-vs-missing.sh
index be20ee3..54fe30a 100755
--- a/tests/tail-2/F-vs-missing.sh
+++ b/tests/tail-2/F-vs-missing.sh
@@ -48,7 +48,7 @@ for mode in '' '---disable-inotify'; do
(cd missing && echo x > file) || framework_failure_
# Wait up to 12.7s for this to appear in the output:
- # "tail: '...' has appeared; following end of new file"
+ # "tail: '...' has appeared; following new file"
tail_re='has appeared' retry_delay_ check_tail_output .1 7 ||
{ echo "$0: file: unexpected delay?"; cat out; fail=1; }
diff --git a/tests/tail-2/F-vs-rename.sh b/tests/tail-2/F-vs-rename.sh
index ee61a54..06733fb 100755
--- a/tests/tail-2/F-vs-rename.sh
+++ b/tests/tail-2/F-vs-rename.sh
@@ -53,7 +53,7 @@ for mode in '' '---disable-inotify'; do
echo x > a
# Wait up to 12.7s for this to appear in the output:
- # "tail: '...' has appeared; following end of new file"
+ # "tail: '...' has appeared; following new file"
tail_re='has appeared' retry_delay_ check_tail_output .1 7 ||
{ echo "$0: a: unexpected delay?"; cat out; fail=1; }
--
2.4.1
