Re: inotify back end for tail -f on linux

[Jim Meyering]

Giuseppe Scrivano wrote:
> Jim Meyering <address@hidden> writes:
>
>> I haven't fixed that last problem.
>> Please fold the following patch into yours for the next iteration.
>
> in this version I included all the problems you reported me, the "touch
> k; chmod 0 k; ./tail -F k" test case is included in the tests suite
> now.
...
> +static void
> +tail_forever_inotify (int wd, struct File_spec *f, int nfiles)

Please rename to n_files, to be consistent with other variables
in this file.  Besides, it's slightly more readable.

> +{
> +  unsigned int i;
> +  unsigned int max_realloc = 3;
> +  Hash_table *wd_table;
> +
> +  bool found_watchable = false;
> +  size_t last;
> +  size_t evlen = 0;
> +  char *evbuff;
> +  size_t evbuff_off = 0;

Typically we abbreviate with "buf", not "buff".
How about evbuf and evbuf_off?

...
> +          hash_insert (wd_table, fspec);

Whoops.
You forgot to detect insertion failure.
I'm adding __attribute__ ((__warn_unused_result__))
to that function and a few others in gnulib's hash.h,
so the compiler will detect this if it happens again.

> +          if (follow_mode == Follow_name)
> +            recheck (&(f[i]), false);
> +        }
...
>
>    if (forever)
> -    tail_forever (F, n_files, sleep_interval);
> +    {
> +#ifdef HAVE_INOTIFY

I have a slight preference for the shorter #if ...:

  #if HAVE_INOTIFY

> +      if (pid == 0)
> +        {
> +          int wd = inotify_init ();
> +          if (0 <= wd)
> +            {
> +              tail_forever_inotify (wd, F, n_files);
> +
> +              /* The only way the above returns is upon failure.  */
> +              exit (EXIT_FAILURE);
> +            }
> +          else
> +            error (0, errno, _("inotify cannot be used, reverting to 
> polling"));

For readability, I usually prefer not to use a single-line else block
unless the then-block is also a one-liner.  Please do this instead:

           if (wd < 0)
             error (0, errno, _("inotify cannot be used, reverting to 
polling"));
           else
             {
               ...
             }

...
> +sleep 2 &
> +pid=$!
> +sleep .5
> +grep '^State:[        ]*[S]' /proc/$pid/status > /dev/null 2>&1 ||
> +  skip_test_ "/proc/$pid/status: missing or 'different'"
> +kill $pid
> +
> +if [ ! -e here ]; then
> +  touch here || framework_failure
> +fi

How about just this (like below)?

    touch here || framework_failure

Each test is guaranteed to start in an empty directory.

> +fail=0
> +
> +
> +# Use tail itself to create a background process.
> +
> +tail -f here &
> +bg_pid=$!
> +
> +tail -f here --pid=$bg_pid &
> +
> +pid=$!
> +
> +sleep 2

Bonus points if you can test effectively (i.e., still avoid race conditions)
without sleeping for long.  I consider 4-5 seconds "long".

> +set _ `sed -n '/^State:[      ]*\([^  ]\)/s//\1/p' /proc/$pid/status`
> +shift # Remove the leading `_'.
> +state=$1
> +
> +if [ ! -z $state ]; then
> +  case $state in
> +    S*) ;;
> +    *) echo $0: process dead 1>&2; fail=1 ;;
> +  esac
> +  kill $pid
> +fi
> +
> +sleep 2
> +
> +set _ `sed -n '/^State:[      ]*\([^  ]\)/s//\1/p' /proc/$pid/status`
> +shift
> +state=$1
> +
> +if [ ! -z $state ]; then
> +  case $state in
> +    *) ;;
> +    S*) echo $0: process still active 1>&2; fail=1 ;;
> +  esac
> +  kill $pid
> +fi
> +
> +
> +kill $bg_pid
> +rm here

No need to remove files.
The entire temporary directory is removed automatically.
Same below.

> diff --git a/tests/tail-2/wait b/tests/tail-2/wait
...
> +if test "$VERBOSE" = yes; then
> +  set -x
> +  tail --version
> +fi
> +
> +. $srcdir/test-lib.sh
> +
> +sleep 2 &
> +pid=$!
> +sleep .5
> +grep '^State:[        ]*[S]' /proc/$pid/status > /dev/null 2>&1 ||
> +  skip_test_ "/proc/$pid/status: missing or 'different'"

With these two new uses, the above test has been repeated enough times
now that it deserves to be factored into a require_proc_pid_status_
function in test-lib.sh.

> +kill $pid
> +
> +touch here || framework_failure
> +
> +(touch not_accessible && chmod 0 not_accessible) || framework_failure
> +
> +if [ -e not_here ]; then
> +  rm -f not_here || framework_failure
> +fi

No need for the -e not_here test,
since it starts in an empty directory.

> +fail=0
> +
> +rm -f  tail.err
> +
> +(tail -f not_here 2> tail.err) &
> +pid=$!
> +sleep .5
> +set _ `sed -n '/^State:[      ]*\([^  ]\)/s//\1/p' /proc/$pid/status`
> +shift # Remove the leading `_'.
> +state=$1

The above 3 lines are repeated far too many times.
Please define a function in test-lib.sh, and then do e.g.,

state=$(get_process_state_ $pid)

> +if [ ! -z $state ]; then

- insufficient quoting; that will evoke a syntax error if $state is empty
    (btw, below, no quotes in "case $state in..." is fine)
- use -n in place of "! -z";
- I prefer to use "test" in place of "["..."]"

   if test -n "$state"; then

> +  case $state in
> +    *) ;;
> +    S*) echo $0: process still active 1>&2; fail=1 ;;
> +  esac
> +  kill $pid
> +fi
...

> +test  "$(wc -w < tail.err) eq 0"  || fail=1

This can be written more simply:

  test -s tail.err && fail=1

> +rm -f here
> +rm -f tail.err
> +rm -f not_accessible
> +
> +
> +Exit $fail