Subject: Re: bug#19760: [bug] "tail -f" with inotify fails to follow a file after a rename()

From: Pádraig Brady <[email-masked]>
Date: Tue, 31 Mar 2015 11:56:40 +0100

On 31/03/15 07:30, Bernhard Voelker wrote:
> On 03/31/2015 05:15 AM, Pádraig Brady wrote:
>> +  tail -f continues to follow changes to a file even after it's renamed.
>> +  [bug introduced in coreutils-7.5]
>> +
> 
> It is not 100% clear to me by this sentence what was the actual change;
> maybe a little "again" or "now" would help?
> 
>> --- /dev/null
>> +++ b/tests/tail-2/f-vs-rename.sh
>> @@ -0,0 +1,51 @@
>> +#!/bin/sh
>> +# demonstrate that tail -f works when renaming the tailed files
> 
> s/^d/D/; s/$/./
> 
>> +# Before coreutils-8.24, tail -f a would stop tracking additions to b
>> +# after "mv a b".
>> +
>> +# 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
>> +
>> +touch a || framework_failure_
>> +
>> +debug='---disable-inotify'
>> +debug=
>> +tail $debug -f -s.1 a > out 2>&1 & pid=$!
> 
> Shouldn't $debug be removed?  Otherwise maybe a loop over both the
> inotify and the non-inotify mode would make sense?
> 
>> +
>> +check_tail_output()
>> +{
>> +  local delay="$1"
>> +  grep "$tail_re" out > /dev/null ||
>> +    { sleep $delay; return 1; }
>> +}
> 
> Please don't discard grep's output: reading the test's log file is
> easier with this included.
> 
>> +
>> +# Wait up to 12.7s for tail to start
> 
> s/$/./
> 
>> +echo x > a
>> +tail_re='^x$' retry_delay_ check_tail_output .1 7 || fail=1
>> +
>> +mv a b || fail=1
>> +
>> +echo y >> b
>> +# Wait up to 12.7s for "y" to appear in the output:
>> +tail_re='^y$' retry_delay_ check_tail_output .1 7 || fail=1
>> +
>> +kill $pid
>> +
>> +wait
>> +
>> +Exit $fail
> 
> Otherwise +1 (including the changes in tail.c).

All good suggestions.
Latest attached.

thanks for the review!
Pádraig.

>From d313a0b24234d3366ec263111469f219f5b4634f Mon Sep 17 00:00:00 2001
From: Stephane Chazelas <[email-masked]>
Date: Tue, 3 Feb 2015 21:22:06 +0000
Subject: [PATCH] tail: fix -f to follow changes after a rename

* src/tail.c (tail_forever_inotify): Only monitor write()s and
truncate()s to files in --follow=descriptor mode, thus avoiding
the bug where we removed the watch on renamed files.
Also adjust the inotify event processing code that is
now significant only in --follow=name mode.
* tests/tail-2/F-vs-rename.sh: Improve this existing test by running
in both polling and inotify modes.
* tests/tail-2/f-vs-rename.sh: A new test based on the existing one.
* tests/local.mk: Reference the new test.
* NEWS: Mention the bug.
Fixes http://bugs.gnu.org/19760
---
 NEWS                        |  3 ++
 src/tail.c                  | 31 +++++++--------
 tests/local.mk              |  1 +
 tests/tail-2/F-vs-rename.sh | 94 +++++++++++++++++++++++----------------------
 tests/tail-2/f-vs-rename.sh | 51 ++++++++++++++++++++++++
 5 files changed, 117 insertions(+), 63 deletions(-)
 create mode 100755 tests/tail-2/f-vs-rename.sh

diff --git a/NEWS b/NEWS
index 81031c6..4b12e46 100644
--- a/NEWS
+++ b/NEWS
@@ -39,6 +39,9 @@ GNU coreutils NEWS                                    -*- outline -*-
   resources with many files, or with -F if files were replaced many times.
   [bug introduced in coreutils-7.5]
 
+  tail -f again follows changes to a file after it's 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 c5380cb..f75d7a9 100644
--- a/src/tail.c
+++ b/src/tail.c
@@ -159,13 +159,6 @@ struct File_spec
   uintmax_t n_unchanged_stats;
 };
 
-#if HAVE_INOTIFY
-/* The events mask used with inotify on files.  This mask is not used on
-   directories.  */
-static const uint32_t inotify_wd_mask = (IN_MODIFY | IN_ATTRIB
-                                         | IN_DELETE_SELF | IN_MOVE_SELF);
-#endif
-
 /* Keep trying to open a file even if it is inaccessible when tail starts
    or if it becomes inaccessible later -- useful only with -f.  */
 static bool reopen_inaccessible_files;
@@ -1390,6 +1383,13 @@ tail_forever_inotify (int wd, struct File_spec *f, size_t n_files,
   if (! wd_to_name)
     xalloc_die ();
 
+  /* The events mask used with inotify on files (not directories).  */
+  uint32_t inotify_wd_mask = IN_MODIFY;
+  /* TODO: Perhaps monitor these events in Follow_descriptor mode also,
+     to tag reported file names with "deleted", "moved" etc.  */
+  if (follow_mode == Follow_name)
+    inotify_wd_mask |= (IN_ATTRIB | IN_DELETE_SELF | IN_MOVE_SELF);
+
   /* Add an inotify watch for each watched file.  If -F is specified then watch
      its parent directory too, in this way when they re-appear we can add them
      again to the watch list.  */
@@ -1641,20 +1641,17 @@ tail_forever_inotify (int wd, struct File_spec *f, size_t n_files,
 
       if (ev->mask & (IN_ATTRIB | IN_DELETE_SELF | IN_MOVE_SELF))
         {
-          /* For IN_DELETE_SELF, we always want to remove the watch.
-             However, for IN_MOVE_SELF (the file we're watching has
-             been clobbered via a rename), when tailing by NAME, we
-             must continue to watch the file.  It's only when following
-             by file descriptor that we must remove the watch.  */
-          if ((ev->mask & IN_DELETE_SELF)
-              || ((ev->mask & IN_MOVE_SELF)
-                  && follow_mode == Follow_descriptor))
+          /* Note for IN_MOVE_SELF (the file we're watching has
+             been clobbered via a rename) we leave the watch
+             in place since it may still be part of the set
+             of watched names.  */
+          if (ev->mask & IN_DELETE_SELF)
             {
               inotify_rm_watch (wd, fspec->wd);
               hash_delete (wd_to_name, fspec);
             }
-          if (follow_mode == Follow_name)
-            recheck (fspec, false);
+
+          recheck (fspec, false);
 
           continue;
         }
diff --git a/tests/local.mk b/tests/local.mk
index 56cba69..1be31ad 100644
--- a/tests/local.mk
+++ b/tests/local.mk
@@ -171,6 +171,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-vs-rename.sh			\
   tests/tail-2/inotify-rotate.sh		\
   tests/tail-2/inotify-rotate-resources.sh	\
   tests/chmod/no-x.sh				\
diff --git a/tests/tail-2/F-vs-rename.sh b/tests/tail-2/F-vs-rename.sh
index cb6a3ed..f95b71a 100755
--- a/tests/tail-2/F-vs-rename.sh
+++ b/tests/tail-2/F-vs-rename.sh
@@ -1,7 +1,7 @@
 #!/bin/sh
-# demonstrate that tail -F works when renaming the tailed files
-# Before coreutils-8.3, tail -F a b would stop tracking additions to b
-# after "mv a b".
+# Demonstrate that tail -F works when renaming the tailed files.
+# Between coreutils 7.5 and 8.2 inclusive, 'tail -F a b' would
+# stop tracking additions to b after 'mv a b'.
 
 # Copyright (C) 2009-2015 Free Software Foundation, Inc.
 
@@ -21,55 +21,57 @@
 . "${srcdir=.}/tests/init.sh"; path_prepend_ ./src
 print_ver_ tail
 
-touch a b || framework_failure_
-
-debug='---disable-inotify'
-debug=
-tail $debug -F -s.1 a b > out 2>&1 & pid=$!
-
 check_tail_output()
 {
   local delay="$1"
-  grep "$tail_re" out > /dev/null ||
+  grep "$tail_re" out ||
     { sleep $delay; return 1; }
 }
 
-# Wait up to 12.7s for tail to start
-echo x > a
-tail_re='^x$' retry_delay_ check_tail_output .1 7 || fail=1
-
-mv a b || fail=1
-
-# Wait 12.7s for this diagnostic:
-# tail: 'a' has become inaccessible: No such file or directory
-tail_re='inaccessible' retry_delay_ check_tail_output .1 7 || fail=1
-
-echo x > a
-# Wait up to 12.7s for this to appear in the output:
-# "tail: '...' has appeared;  following end of new file"
-tail_re='has appeared' 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 "y" to appear in the output:
-tail_f_vs_rename_2() {
-  local delay="$1"
-  tr '\n' @ < out | grep '@@==> b <==@y@$' > /dev/null ||
-    { sleep $delay; return 1; }
-}
-retry_delay_ tail_f_vs_rename_2 .1 7 ||
-  { echo "$0: b: unexpected delay?"; cat out; fail=1; }
-
-echo z >> a
-# Wait up to 12.7s for "z" to appear in the output:
-tail_f_vs_rename_3() {
-  local delay="$1"
-  tr '\n' @ < out | grep '@@==> a <==@z@$' > /dev/null ||
-    { sleep $delay; return 1; }
-}
-retry_delay_ tail_f_vs_rename_3 .1 7 ||
-  { echo "$0: a: unexpected delay?"; cat out; fail=1; }
+touch a b || framework_failure_
 
-kill -HUP $pid
+for mode in '' '---disable-inotify'; do
+  tail $mode -F -s.1 a b > out 2>&1 & pid=$!
+
+  # Wait up to 12.7s for tail to start.
+  echo x > a
+  tail_re='^x$' retry_delay_ check_tail_output .1 7 || fail=1
+
+  mv a b || fail=1
+
+  # Wait 12.7s for this diagnostic:
+  # tail: 'a' has become inaccessible: No such file or directory
+  tail_re='inaccessible' retry_delay_ check_tail_output .1 7 || fail=1
+
+  echo x > a
+  # Wait up to 12.7s for this to appear in the output:
+  # "tail: '...' has appeared;  following end of new file"
+  tail_re='has appeared' 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 "y" to appear in the output:
+  tail_f_vs_rename_2() {
+    local delay="$1"
+    tr '\n' @ < out | grep '@@==> b <==@y@$' > /dev/null ||
+      { sleep $delay; return 1; }
+  }
+  retry_delay_ tail_f_vs_rename_2 .1 7 ||
+    { echo "$0: b: unexpected delay?"; cat out; fail=1; }
+
+  echo z >> a
+  # Wait up to 12.7s for "z" to appear in the output:
+  tail_f_vs_rename_3() {
+    local delay="$1"
+    tr '\n' @ < out | grep '@@==> a <==@z@$' > /dev/null ||
+      { sleep $delay; return 1; }
+  }
+  retry_delay_ tail_f_vs_rename_3 .1 7 ||
+    { echo "$0: a: unexpected delay?"; cat out; fail=1; }
+
+  kill $pid
+
+  wait $pid
+done
 
 Exit $fail
diff --git a/tests/tail-2/f-vs-rename.sh b/tests/tail-2/f-vs-rename.sh
new file mode 100755
index 0000000..4bff41c
--- /dev/null
+++ b/tests/tail-2/f-vs-rename.sh
@@ -0,0 +1,51 @@
+#!/bin/sh
+# Demonstrate that tail -f works when renaming the tailed files.
+# Between coreutils 7.5 and 8.23 inclusive, 'tail -f a' would
+# stop tracking additions to b after 'mv a b'.
+
+# 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; }
+}
+
+touch a || framework_failure_
+
+for mode in '' '---disable-inotify'; do
+  tail $mode -f -s.1 a > out 2>&1 & pid=$!
+
+  # Wait up to 12.7s for tail to start.
+  echo x > a
+  tail_re='^x$' retry_delay_ check_tail_output .1 7 || fail=1
+
+  mv a b || fail=1
+
+  echo y >> b
+  # Wait up to 12.7s for "y" to appear in the output:
+  tail_re='^y$' retry_delay_ check_tail_output .1 7 || fail=1
+
+  kill $pid
+
+  wait $pid
+done
+
+Exit $fail
-- 
2.1.0