Re: inotify back end for tail -f on linux

[Jim Meyering]

Giuseppe Scrivano wrote:
> Jim Meyering <address@hidden> writes:
>
>> No test is too small or too "obvious" to add ;-)
>>
>> Adding this simple test would be great.
>> Thanks!

Hi Giuseppe,

> I found another problem.  The inotify version can't be used when --pid
> is specified.  There are other inotify events that I want to investigate
> better if they can be used with --pid and in this case we can add them
> later when the inotify back end is stable enough for me to break it
> again :)  What do you think?

Good catch.

> This is the updated version, I integrated your test-case in the
> tests/tail-2/wait file and I added a new file (tests/tail-2/pid) with
> additional tests for the --pid option.

Thanks!

>>From f0824b8b79c1fb22e9a48cbf831a87433fc4d3e8 Mon Sep 17 00:00:00 2001
> From: Giuseppe Scrivano <address@hidden>
> Date: Tue, 2 Jun 2009 08:28:23 +0200
> Subject: [PATCH] tail: Use inotify if it is available.
>
...
> +  tail uses inotify if it is present.

We can be slightly more precise:

     tail uses inotify when possible

>  * Noteworthy changes in release 7.4 (2009-05-07) [stable]
>
> diff --git a/configure.ac b/configure.ac
> index 4eb640e..a63ac0c 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -410,6 +410,8 @@ AM_GNU_GETTEXT_VERSION([0.15])
>  # For a test of uniq: it uses the $LOCALE_FR envvar.
>  gt_LOCALE_FR
>
> +AC_CHECK_FUNCS([inotify_init], [AC_DEFINE([HAVE_INOTIFY], [1], [Check for 
> libinotify])])
> +

We like to avoid adding lines that occupy more than 80 columns.
How about something like this instead?
The comment is the one that ends up in lib/config.h.
This is not checking for a library.

  AC_CHECK_FUNCS([inotify_init],
    [AC_DEFINE([HAVE_INOTIFY], [1],
      [Define to 1 if you have usable inotify support.])])

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

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

> +          evbuff_off = 0;
> +
> +          if (len <= 0 && errno == EINVAL && max_realloc--)
> +            {
> +              len = 0;
> +              evlen *= 2;
> +              evbuff = xrealloc (evbuff, evlen);
> +              continue;
> +            }
> +
> +          if (len <= 0 && errno == EINTR)
> +            {
> +              len = 0;
> +              continue;
> +            }
> +
> +          if (len <= 0)
> +            error (EXIT_FAILURE, errno, _("error reading inotify event"));
> +        }
...

>    if (forever)
> -    tail_forever (F, n_files, sleep_interval);
> +    {
> +#ifdef HAVE_INOTIFY
> +      int wd = inotify_init ();
> +      if (wd > 0 && pid == 0)
> +        tail_forever_inotify (wd, F, n_files);
> +#endif
> +      tail_forever (F, n_files, sleep_interval);
> +    }
>
>    if (have_read_stdin && close (STDIN_FILENO) < 0)
>      error (EXIT_FAILURE, errno, "-");

Thanks.
I had to make some small changes:

  - chmod a+x tests/tail-2/{pid,wait}
     That works around this "make check" failure:

        --- t1  2009-06-06 09:50:14.000000000 +0200
        +++ t2  2009-06-06 09:50:17.000000000 +0200
        @@ -359,11 +359,9 @@
         tail-2/assert-2
         tail-2/big-4gb
         tail-2/infloop-1
        -tail-2/pid
         tail-2/proc-ksyms
         tail-2/start-middle
         tail-2/tail-n0f
        -tail-2/wait
         touch/dangling-symlink
         touch/dir-1
         touch/empty-file
        make[2]: *** [vc_exe_in_TESTS] Error 1
        make[2]: *** Waiting for unfinished jobs....

--------------------------------------
Notice how when tail_forever_inotify returns,
execution continues into tail_forever.  It shouldn't.

--------------------------------------
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.
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);
     }
--
1.6.3.2.277.gd10543