Re: [PATCH] tail should dereference watched file

[Pádraig Brady]

On 11/22/2013 02:32 AM, Bernhard Voelker wrote:
> 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
> 

Well we bail once we see a symlink now
so as to diagnose that edge case.
I'm note sure about handling your edge case above :)
What might get back to previous behavior
would be to diagnose but continue checking?
I was copying the "remote file" logic in this regard,
but that mightn't be best.
Also all the above items are in the same dir,
in which inotify would work fine.
So perhaps we need to restrict this special
handling to when the symlink points to a
different target dir.

thanks!
Pádraig.