Re: inotify back end for tail -f on linux

[Jim Meyering]

Giuseppe Scrivano wrote:

> Jim, thank for the review.
>
> Jim Meyering <address@hidden> writes:
>
>> Perhaps "prev_wd" would be more accurate?
>
> I fixed it.  The same name is used in `tail_forever', that is why I used
> it, should it be changed in `tail_forever' too?
>
>
>> Another regression:
>>
>>     touch k; chmod 0 k; tail -F k
>>
>> fails to open "k" (as it must), but even
>> when I run "chmod u+r k" in another window,
>> the inotify-based version of tail fails to open it.
>
> I fixed it.
>
> This version includes, among other things, a refactoring of the new
> tests as previously reported.
...
Thanks for yet another round.
I'll look more closely tomorrow, but here are things I noticed right away:

Comment style.  You added comments like this.  Thanks!

   /* 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.  */

Please remove the leading '*'s:

   /* 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.  */


> diff --git a/tests/test-lib.sh b/tests/test-lib.sh
> index a765bd6..a9684fb 100644
> --- a/tests/test-lib.sh
> +++ b/tests/test-lib.sh
> @@ -122,6 +122,11 @@ uid_is_privileged_()
>    esac
>  }
>
> +get_process_status_()
> +{
> +  echo $(sed -n '/^State:[ \t]*\([[:alpha:]]\)\(.*\)/s//\1/p' 
> /proc/$1/status)
> +}

No need for echo and the subshell.
Sadly, \t is not portable
No need for 2nd \(...\) group

get_process_status_()
{
  sed -n '/^State:[      ]*\([[:alpha:]]\).*/s//\1/p' /proc/$1/status
}

>  # Convert an ls-style permission string, like drwxr----x and -rw-r-x-wx
>  # to the equivalent chmod --mode (-m) argument, (=,u=rwx,g=r,o=x and
>  # =,u=rw,g=rx,o=wx).  Ignore ACLs.
> @@ -234,6 +239,17 @@ of group names or numbers.  E.g.,
>    esac
>  }
>
> +# Is /proc/$PID/status supported?
> +require_proc_pid_status_()
> +{
> +    sleep 2 &
> +    local pid=$!
> +    sleep .5
> +    grep '^State:[    ]*[S]' /proc/$pid/status > /dev/null 2>&1 ||
> +    skip_test_ "/proc/$pid/status: missing or 'different'"

Indent a statement like that to make it easier to see that
it is conditional:

       grep '^State:[    ]*[S]' /proc/$pid/status > /dev/null 2>&1 ||
           skip_test_ "/proc/$pid/status: missing or 'different'"

> +    kill $pid
> +}
> +
>  # Does the current (working-dir) file system support sparse files?
>  require_sparse_support_()
>  {