Re: [PATCH] tail should dereference watched file

[Bernhard Voelker]

On 11/21/2013 06:01 PM, Pádraig Brady wrote:
> On 09/16/2013 11:11 PM, Pádraig Brady wrote:
>> On 09/15/2013 10:43 PM, Bernhard Voelker wrote:
>>> On 09/15/2013 08:58 PM, Pádraig Brady wrote:
>>>> True. Perhaps we should revert to polling for symlinks?
>>>
>>> That seems to be the safest way, yes.
>>
>> Though we'd have to be careful to handle the case
>> where a file was replaced with a symlink.
> 
> Hopefully the attached fixes up the common case of this,
> and diagnose the edge case where a (missing) file appears
> as a symlink, during inotify processing.

Hi Padraig,

Thanks for continue working on this - I almost forgot it.

The first part is minor nits in the test suite:

> --- /dev/null
> +++ b/tests/tail-2/symlink.sh

> +
> +# Ensure changing targets of cli specified symlinks are handled

s/$/./

> +# Prior to v8.22, inotify would fail to recognize changes in the targets

s/$/./

> +# Clear 'out' so that we can check its contents without races

s/$/./

> +:>out                           || framework_failure_
> +ln -nsf target symlink          || framework_failure_
> +timeout 10 tail -s.1 -F symlink >out 2>&1 & pid=$!
> +retry_delay_ wait4lines_ .1 6 1 || fail=1  # Wait for "cannot open..."
> +echo "X" > target               || fail=1
> +retry_delay_ wait4lines_ .1 6 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 appeared' out || { fail=1; cat out; }
> +grep '^X$' out             || { fail=1; cat out; }
> +rm -f target out           || framework_failure_

Looks good.

> +
> +# Ensure we correctly handle the source symlink itself changing.
> +# I.E. that we don't operate solely on the targets.
> +# Clear 'out' so that we can check its contents without races
> +:>out                           || framework_failure_
> +touch target1                   || framework_failure_

Why not use ":>target1" here, too? ... or alternatively move
the 'echo "X1" > target1' from 3 lines below to here?

> +ln -nsf target1 symlink         || framework_failure_
> +timeout 10 tail -s.1 -F symlink >out 2>&1 & pid=$!
> +echo "X1" > target1
> +retry_delay_ wait4lines_ .1 6 1 || fail=1  # Wait for the expected output.
> +ln -nsf target2 symlink         || framework_failure_
> +retry_delay_ wait4lines_ .1 6 2 || fail=1  # Wait for "become inaccess..."
> +echo "X2" > target2             || fail=1
> +retry_delay_ wait4lines_ .1 6 4 || fail=1  # Wait for the expected output.
> +kill $pid
> +wait $pid
> +# Expect 3 lines in the output file.

s/3/4/ ;-)

> +[ $( wc -l < out ) = 4 ]   || { fail=1; cat out; }
> +grep -F 'become inaccessible' out  || { fail=1; cat out; }
> +grep -F 'has appeared' out || { fail=1; cat out; }
> +grep '^X[12]$' out             || { fail=1; cat out; }

Better to test for X1 and X2 separately, to avoid eclipsing to determine
when one of the above wait4lines_ call ran into a timeout.


> +rm -f target1 target2 out  || framework_failure_
> +
> +# Note other symlink edge cases are currently just diagnosed
> +# rather than being handled. I.E. if you specify a missing item,
> +# or existing file that later change to a symlink, if inotify
> +# is in use, you'll get a diagnostic saying that link will
> +# no longer be tailed.
> +
> +Exit $fail


Now regarding the functionality change:
I can't say what it is, but the previous behavior was somehow
better in certain cases [*].

Compare the output of
  tail -F -s1 symlink
for the old (413a2415) and new program when executing the following
in a 3rd terminal.

Preparation:

  echo X > target1
  echo Y > target2
  ln -nsf target1 symlink

Now start both tail-{old,new} and do the following:

  ln -nsf target2 symlink
  ln -nsf target1 symlink
  ln -nsf target2 symlink
  rm symlink
  mkdir symlink
  rmdir symlink
  ln -nsf target1 symlink
  ln -nsf target2 symlink
  rm symlink
  echo Z > symlink

I didn't dig into what's going on, but from a certain
point on, tail-new doesn't output anything anymore.
Can you reproduce this?

[*]
Well, it's half past 3 at night now, so it's probably too much
for me now after a long day:  I somehow lost the whole point of
this proposed patch. ;-/

Thanks & have a nice day,
Berny