Re: inotify back end for tail -f on linux

[Giuseppe Scrivano]

Hi Jim,

Jim Meyering <address@hidden> writes:

> Oh, and it'd be better to put that test in m4/jm-macros.m4,
> not in configure.ac.

Ok, I'll move it.


>>  AC_CONFIG_FILES(
>>    Makefile
>>    doc/Makefile
> ...
>> +      if (len <= evbuff_off)
>> +        {
>> +          len = read (wd, evbuff, evlen);
>
> Please use safe_read here.
> Then you can drop the EINTR test/block below.

Ok.

> --------------------------------------
> Notice how this waits, as it should:
>
>     $ touch k; chmod 0 k; ./tail --pid $$ -F k
>     ./tail: cannot open `k' for reading: Permission denied
>
> yet this exits immediately:
> [also, it'd be nice not to give two diagnostics about the same problem]
>
>     $ touch k; chmod 0 k; ./tail -F k
>     ./tail: cannot open `k' for reading: Permission denied
>     ./tail: cannot watch `k': Permission denied
>     [Exit 1]
> --------------------------------------
>
> I haven't fixed that last problem.

Thanks, I'll take a look at it.


> Please fold the following patch into yours for the next iteration.
>
>
> From 2a200db8a5f3b36d23b179bc4343c647957d6d13 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <address@hidden>
> Date: Sat, 6 Jun 2009 10:12:02 +0200
> Subject: [PATCH] tail: minor changes
>
> * src/tail.c (main): Don't call inotify_init when waiting for a PID.
> [HAVE_INOTIFY]: Exit nonzero whenever tail_forever_inotify returns.
> Change "wd > 0" to "0 <= wd", since 0 is a valid file descriptor.
> ---
>  src/tail.c |   12 +++++++++---
>  1 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/src/tail.c b/src/tail.c
> index 6d5f870..4d9270f 100644
> --- a/src/tail.c
> +++ b/src/tail.c
> @@ -1929,9 +1929,15 @@ main (int argc, char **argv)
>    if (forever)
>      {
>  #ifdef HAVE_INOTIFY
> -      int wd = inotify_init ();
> -      if (wd > 0 && pid == 0)
> -        tail_forever_inotify (wd, F, n_files);
> +      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);
> +        }
>  #endif
>        tail_forever (F, n_files, sleep_interval);
>      }

I think the exit (EXIT_FAILURE) should be included in the if:

if (0 <= wd)
  {
    tail_forever_inotify (wd, F, n_files);
    exit (EXIT_FAILURE);
  }

The inotify_init () call can fail if the inotify support is not present
at runtime, and in this case it is a good idea to use polling instead of
exit immediately.  Do you agree?

Regards,
Giuseppe