Re: tail -f: --pid *and* inotify

[Jim Meyering]

Giuseppe Scrivano wrote:
> Jim Meyering <address@hidden> writes:
>> Hi Giuseppe,
>>
>> I've realized that there is a good way to remove the ugly exclusion
>> that currently disables inotify-based tail -f when --pid is specified.
>> Instead of the existing while-1-loop around code that reads the inotify
>> FD, we can use a loop that polls that single FD with a 1-2-second timeout.
>> Then, it can periodically run the usual PID-alive check.
>> Are you interested in doing that?
>>
>> Between this, the ls -1U bug, and the btrfs O(1) copy,
>> I'm now expecting to defer the release by at least a week.
>
> I modified tail as you suggested.

Thanks!
A couple of points:

>>From 862c9d934dc2ee188e9fe29985e463e2ec4d16ca Mon Sep 17 00:00:00 2001
> From: Giuseppe Scrivano <address@hidden>
> Date: Sun, 26 Jul 2009 13:22:57 +0200
> Subject: [PATCH] tail: use the inotify backend when a PID is specified.
>
> * src/tail.c (tail_forever_inotify): When a PID is specified avoid read
> to sleep indefinitely.  Check if the specified PID if it is still alive,
> otherwise exit from tail.
> (main): Use the new tail_forever_inotify interface.
> ---
>  src/tail.c |   49 ++++++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 38 insertions(+), 11 deletions(-)
>
> diff --git a/src/tail.c b/src/tail.c
> index fd44e22..74bda55 100644
> --- a/src/tail.c
> +++ b/src/tail.c
> @@ -50,6 +50,8 @@
>  #if HAVE_INOTIFY
>  # include "hash.h"
>  # include <sys/inotify.h>
> +/* `select' is used by tail_forever_inotify.  */
> +# include <sys/select.h>
>  #endif
>
>  /* The official name of this program (e.g., no `g' prefix).  */
> @@ -1162,7 +1164,8 @@ wd_comparator (const void *e1, const void *e2)
>     Check modifications using the inotify events system.  */
>
>  static void
> -tail_forever_inotify (int wd, struct File_spec *f, size_t n_files)
> +tail_forever_inotify (int wd, struct File_spec *f, size_t n_files, int pid,
> +                      double sleep_interval)
>  {
>    size_t i;
>    unsigned int max_realloc = 3;
> @@ -1174,6 +1177,8 @@ tail_forever_inotify (int wd, struct File_spec *f, 
> size_t n_files)
>    char *evbuf;
>    size_t evbuf_off = 0;
>    size_t len = 0;
> +  fd_set rfd;
> +  struct timeval select_timeout;

Please move these declarations down into the scope where they are used.

>    wd_table = hash_initialize (n_files, NULL, wd_hasher, wd_comparator, NULL);
>    if (! wd_table)
> @@ -1253,6 +1258,31 @@ tail_forever_inotify (int wd, struct File_spec *f, 
> size_t n_files)
>
>        struct inotify_event *ev;
>
> +      /* If a process is watched be sure that read from wd will not block
> +         indefinetely.  */
> +      if (pid)
> +        {
> +          int n_descriptors;
> +          FD_ZERO (&rfd);
> +          FD_SET (wd, &rfd);
> +
> +          select_timeout.tv_sec = (time_t) sleep_interval;
> +          select_timeout.tv_usec = 1000000000 * (sleep_interval
> +                                                 - select_timeout.tv_sec);
> +
> +          n_descriptors = select (wd + 1, &rfd, NULL, NULL, &select_timeout);
> +
> +          if (n_descriptors == -1)
> +            error (EXIT_FAILURE, errno, _("error monitoring inotify event"));
> +
> +          /* Check if the process we are monitoring is still alive.  */
> +          if (kill (pid, 0) != 0 && errno != EPERM)
> +            break;
> +
> +          if (n_descriptors == 0)
> +            continue;

It would be better not to perform the kill test after every
single select call when actively tailing files.
Considering how --pid is documented (in the texinfo manual),
it should be ok to call kill only when select times out (returns 0).
Of course, that means a constantly-running tail -f will
never check for process death, but that is consistent
with the documentation.