Re: Bug#545422: coreutils: "tail -f -" fails

[Giuseppe Scrivano]

Hi Jim,

have you considered this patch for inclusion?  I don't see a clearer way
to avoid polling without inotify fd support.

Regards,
Giuseppe


Giuseppe Scrivano <address@hidden> writes:

> This patch changes `tail' to handle stdin separately from inotify
> events, similar to what we are already doing when a --pid is specified.
>
> Regards,
> Giuseppe
>
>
> From f3010bebf9e25be9a83868b4ad9db2cc6cb6613f Mon Sep 17 00:00:00 2001
> From: Giuseppe Scrivano <address@hidden>
> Date: Mon, 7 Sep 2009 16:35:16 +0200
> Subject: [PATCH] tail: handle "-" properly
>
> * src/tail.c (tail_forever_inotify): Handle stdin (i.e., "-", but not
> /dev/stdin) separately from inotify.
> * tests/tail-2/wait: Ensure that when a stdin is watched, tail does not
> raise errors.
> ---
>  src/tail.c        |  176 
> ++++++++++++++++++++++++++++++++++-------------------
>  tests/tail-2/wait |    6 ++
>  2 files changed, 119 insertions(+), 63 deletions(-)
>
> diff --git a/src/tail.c b/src/tail.c
> index e3b9529..b817ecb 100644
> --- a/src/tail.c
> +++ b/src/tail.c
> @@ -134,7 +134,7 @@ struct File_spec
>    int errnum;
>  
>  #if HAVE_INOTIFY
> -  /* The watch descriptor used by inotify.  */
> +  /* The watch descriptor used by inotify, -1 on error, -2 if stdin.  */
>    int wd;
>  
>    /* The parent directory watch descriptor.  It is used only
> @@ -1184,6 +1184,7 @@ tail_forever_inotify (int wd, struct File_spec *f, 
> size_t n_files,
>    char *evbuf;
>    size_t evbuf_off = 0;
>    size_t len = 0;
> +  struct File_spec *stdin_spec = NULL;
>  
>    wd_table = hash_initialize (n_files, NULL, wd_hasher, wd_comparator, NULL);
>    if (! wd_table)
> @@ -1196,6 +1197,34 @@ tail_forever_inotify (int wd, struct File_spec *f, 
> size_t n_files,
>      {
>        if (!f[i].ignore)
>          {
> +          if (STREQ (f[i].name, "-"))
> +            {
> +              int old_flags = fcntl (f[i].fd, F_GETFL);
> +              int new_flags = old_flags | O_NONBLOCK;
> +
> +              stdin_spec = &f[i];
> +              found_watchable = true;
> +
> +              if (old_flags < 0
> +                  || (new_flags != old_flags
> +                      && fcntl (f[i].fd, F_SETFL, new_flags) == -1))
> +                {
> +                  /* Don't update f[i].blocking if fcntl fails.  */
> +                  if (S_ISREG (f[i].mode) && errno == EPERM)
> +                    {
> +                      /* This happens when using tail -f on a file with
> +                         the append-only attribute.  */
> +                    }
> +                  else
> +                    error (EXIT_FAILURE, errno,
> +                           _("%s: cannot change stdin nonblocking mode"));
> +                }
> +              f[i].blocking = false;
> +              f[i].wd = -2;
> +              prev_wd = f[i].wd;
> +              continue;
> +            }
> +
>            size_t fnlen = strlen (f[i].name);
>            if (evlen < fnlen)
>              evlen = fnlen;
> @@ -1235,6 +1264,8 @@ tail_forever_inotify (int wd, struct File_spec *f, 
> size_t n_files,
>                continue;
>              }
>  
> +          prev_wd = f[i].wd;
> +
>            if (hash_insert (wd_table, &(f[i])) == NULL)
>              xalloc_die ();
>  
> @@ -1245,8 +1276,6 @@ tail_forever_inotify (int wd, struct File_spec *f, 
> size_t n_files,
>    if (follow_mode == Follow_descriptor && !found_watchable)
>      return;
>  
> -  prev_wd = f[n_files - 1].wd;
> -
>    evlen += sizeof (struct inotify_event) + 1;
>    evbuf = xmalloc (evlen);
>  
> @@ -1259,12 +1288,12 @@ tail_forever_inotify (int wd, struct File_spec *f, 
> size_t n_files,
>        struct File_spec *fspec;
>        uintmax_t bytes_read;
>        struct stat stats;
> -
> +      bool check_stdin = false;
>        struct inotify_event *ev;
>  
> -      /* When watching a PID, ensure that a read from WD will not block
> -         indefinetely.  */
> -      if (pid)
> +      /* When watching a PID or stdin, ensure that a read from WD will not 
> block
> +         indefinitely.  */
> +      if (pid || stdin_spec)
>          {
>            fd_set rfd;
>            struct timeval select_timeout;
> @@ -1284,78 +1313,92 @@ tail_forever_inotify (int wd, struct File_spec *f, 
> size_t n_files,
>  
>            if (n_descriptors == 0)
>              {
> -              /* See if the process we are monitoring is still alive.  */
> -              if (kill (pid, 0) != 0 && errno != EPERM)
> -                exit (EXIT_SUCCESS);
> +              if (stdin_spec)
> +                check_stdin = true;
> +              if (pid)
> +                {
> +                  /* See if the process we are monitoring is still alive.  */
> +                  if (kill (pid, 0) != 0 && errno != EPERM)
> +                    exit (EXIT_SUCCESS);
>  
> -              continue;
> +                  if (!check_stdin)
> +                    continue;
> +                }
>              }
>          }
>  
> -      if (len <= evbuf_off)
> +      if (check_stdin)
>          {
> -          len = safe_read (wd, evbuf, evlen);
> -          evbuf_off = 0;
> -
> -          /* For kernels prior to 2.6.21, read returns 0 when the buffer
> -             is too small.  */
> -          if ((len == 0 || (len == SAFE_READ_ERROR && errno == EINVAL))
> -              && max_realloc--)
> +          ev = NULL;
> +          fspec = stdin_spec;
> +          check_stdin = false;
> +        }
> +      else
> +        {
> +          if (len <= evbuf_off)
>              {
> -              len = 0;
> -              evlen *= 2;
> -              evbuf = xrealloc (evbuf, evlen);
> -              continue;
> -            }
> +              len = safe_read (wd, evbuf, evlen);
> +              evbuf_off = 0;
>  
> -          if (len == 0 || len == SAFE_READ_ERROR)
> -            error (EXIT_FAILURE, errno, _("error reading inotify event"));
> -        }
> +              /* For kernels prior to 2.6.21, read returns 0 when the buffer
> +                 is too small.  */
> +              if ((len == 0 || (len == SAFE_READ_ERROR && errno == EINVAL))
> +                  && max_realloc--)
> +                {
> +                  len = 0;
> +                  evlen *= 2;
> +                  evbuf = xrealloc (evbuf, evlen);
> +                  continue;
> +                }
>  
> -      ev = (struct inotify_event *) (evbuf + evbuf_off);
> -      evbuf_off += sizeof (*ev) + ev->len;
> +              if (len == 0 || len == SAFE_READ_ERROR)
> +                error (EXIT_FAILURE, errno, _("error reading inotify 
> event"));
> +            }
>  
> -      if (ev->len)
> -        {
> -          for (i = 0; i < n_files; i++)
> +          ev = (struct inotify_event *) (evbuf + evbuf_off);
> +          evbuf_off += sizeof (*ev) + ev->len;
> +
> +          if (ev->len)
>              {
> -              /* With N=hundreds of frequently-changing files, this O(N^2)
> -                 process might be a problem.  FIXME: use a hash table?  */
> -              if (f[i].parent_wd == ev->wd
> -                  && STREQ (ev->name, f[i].name + f[i].basename_start))
> -                break;
> -            }
> +              for (i = 0; i < n_files; i++)
> +                {
> +                  /* With N=hundreds of frequently-changing files, this 
> O(N^2)
> +                     process might be a problem.  FIXME: use a hash table?  
> */
> +                  if (f[i].parent_wd == ev->wd
> +                      && STREQ (ev->name, f[i].name + f[i].basename_start))
> +                    break;
> +                }
>  
> -          /* It is not a watched file.  */
> -          if (i == n_files)
> -            continue;
> +              /* It is not a watched file.  */
> +              if (i == n_files)
> +                continue;
>  
> -          f[i].wd = inotify_add_watch (wd, f[i].name, inotify_wd_mask);
> +              f[i].wd = inotify_add_watch (wd, f[i].name, inotify_wd_mask);
>  
> -          if (f[i].wd < 0)
> -            {
> -              error (0, errno, _("cannot watch %s"), quote (f[i].name));
> -              continue;
> -            }
> +              if (f[i].wd < 0)
> +                {
> +                  error (0, errno, _("cannot watch %s"), quote (f[i].name));
> +                  continue;
> +                }
>  
> -          fspec = &(f[i]);
> -          if (hash_insert (wd_table, fspec) == NULL)
> -            xalloc_die ();
> +              fspec = &(f[i]);
> +              if (hash_insert (wd_table, fspec) == NULL)
> +                xalloc_die ();
>  
> -          if (follow_mode == Follow_name)
> -            recheck (&(f[i]), false);
> -        }
> -      else
> -        {
> -          struct File_spec key;
> -          key.wd = ev->wd;
> -          fspec = hash_lookup (wd_table, &key);
> +              if (follow_mode == Follow_name)
> +                recheck (&(f[i]), false);
> +            }
> +          else
> +            {
> +              struct File_spec key;
> +              key.wd = ev->wd;
> +              fspec = hash_lookup (wd_table, &key);
> +            }
>          }
> -
>        if (! fspec)
>          continue;
>  
> -      if (ev->mask & (IN_ATTRIB | IN_DELETE_SELF | IN_MOVE_SELF))
> +      if (ev && ev->mask & (IN_ATTRIB | IN_DELETE_SELF | IN_MOVE_SELF))
>          {
>            if (ev->mask & (IN_DELETE_SELF | IN_MOVE_SELF))
>              {
> @@ -1364,7 +1407,6 @@ tail_forever_inotify (int wd, struct File_spec *f, 
> size_t n_files,
>              }
>            if (follow_mode == Follow_name)
>              recheck (fspec, false);
> -
>            continue;
>          }
>  
> @@ -1386,11 +1428,19 @@ tail_forever_inotify (int wd, struct File_spec *f, 
> size_t n_files,
>            fspec->size = stats.st_size;
>          }
>  
> -      if (ev->wd != prev_wd)
> +      if (fspec->mode == stats.st_mode
> +          && (! S_ISREG (stats.st_mode) || fspec->size == stats.st_size)
> +          && timespec_cmp (fspec->mtime, get_stat_mtime (&stats)) == 0)
> +        continue;
> +
> +      fspec->mtime = get_stat_mtime (&stats);
> +      fspec->mode = stats.st_mode;
> +
> +      if (fspec->wd != prev_wd)
>          {
>            if (print_headers)
>              write_header (name);
> -          prev_wd = ev->wd;
> +          prev_wd = fspec->wd;
>          }
>  
>        bytes_read = dump_remainder (name, fspec->fd, COPY_TO_EOF);
> diff --git a/tests/tail-2/wait b/tests/tail-2/wait
> index 62498d5..aa6e2dd 100755
> --- a/tests/tail-2/wait
> +++ b/tests/tail-2/wait
> @@ -42,6 +42,12 @@ for inotify in ---disable-inotify ''; do
>    timeout 1 tail -s0.1 -f $inotify here 2>tail.err
>    test $? = 124 || fail=1
>  
> +  timeout 1 tail -s0.1 -f $inotify < here 2>tail.err
> +  test $? = 124 || fail=1
> +
> +  echo hello | timeout 1 tail -s0.1 -f $inotify 2>tail.err
> +  test $? = 124 || fail=1
> +
>    # `tail -F' must wait in any case.
>  
>    timeout 1 tail -s0.1 -F $inotify here 2>>tail.err