[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: inotify back end for tail -f on linux
From: |
Jim Meyering |
Subject: |
Re: inotify back end for tail -f on linux |
Date: |
Thu, 11 Jun 2009 22:12:49 +0200 |
Giuseppe Scrivano wrote:
> Jim, thank for the review.
>
> Jim Meyering <address@hidden> writes:
>
>> Perhaps "prev_wd" would be more accurate?
>
> I fixed it. The same name is used in `tail_forever', that is why I used
> it, should it be changed in `tail_forever' too?
>
>
>> Another regression:
>>
>> touch k; chmod 0 k; tail -F k
>>
>> fails to open "k" (as it must), but even
>> when I run "chmod u+r k" in another window,
>> the inotify-based version of tail fails to open it.
>
> I fixed it.
>
> This version includes, among other things, a refactoring of the new
> tests as previously reported.
...
Thanks for yet another round.
I'll look more closely tomorrow, but here are things I noticed right away:
Comment style. You added comments like this. Thanks!
/* Add an inotify watch for each watched file. If -F is specified then watch
* its parent directory too, in this way when they re-appear we can add them
* again to the watch list. */
Please remove the leading '*'s:
/* Add an inotify watch for each watched file. If -F is specified then watch
its parent directory too, in this way when they re-appear we can add them
again to the watch list. */
> diff --git a/tests/test-lib.sh b/tests/test-lib.sh
> index a765bd6..a9684fb 100644
> --- a/tests/test-lib.sh
> +++ b/tests/test-lib.sh
> @@ -122,6 +122,11 @@ uid_is_privileged_()
> esac
> }
>
> +get_process_status_()
> +{
> + echo $(sed -n '/^State:[ \t]*\([[:alpha:]]\)\(.*\)/s//\1/p'
> /proc/$1/status)
> +}
No need for echo and the subshell.
Sadly, \t is not portable
No need for 2nd \(...\) group
get_process_status_()
{
sed -n '/^State:[ ]*\([[:alpha:]]\).*/s//\1/p' /proc/$1/status
}
> # Convert an ls-style permission string, like drwxr----x and -rw-r-x-wx
> # to the equivalent chmod --mode (-m) argument, (=,u=rwx,g=r,o=x and
> # =,u=rw,g=rx,o=wx). Ignore ACLs.
> @@ -234,6 +239,17 @@ of group names or numbers. E.g.,
> esac
> }
>
> +# Is /proc/$PID/status supported?
> +require_proc_pid_status_()
> +{
> + sleep 2 &
> + local pid=$!
> + sleep .5
> + grep '^State:[ ]*[S]' /proc/$pid/status > /dev/null 2>&1 ||
> + skip_test_ "/proc/$pid/status: missing or 'different'"
Indent a statement like that to make it easier to see that
it is conditional:
grep '^State:[ ]*[S]' /proc/$pid/status > /dev/null 2>&1 ||
skip_test_ "/proc/$pid/status: missing or 'different'"
> + kill $pid
> +}
> +
> # Does the current (working-dir) file system support sparse files?
> require_sparse_support_()
> {
- Re: inotify back end for tail -f on linux, (continued)
- Re: inotify back end for tail -f on linux, Giuseppe Scrivano, 2009/06/06
- Re: inotify back end for tail -f on linux, Jim Meyering, 2009/06/06
- Re: inotify back end for tail -f on linux, Giuseppe Scrivano, 2009/06/07
- Re: inotify back end for tail -f on linux, Jim Meyering, 2009/06/07
- Re: inotify back end for tail -f on linux, Jim Meyering, 2009/06/07
- Re: inotify back end for tail -f on linux, Jim Meyering, 2009/06/11
- Re: inotify back end for tail -f on linux, Giuseppe Scrivano, 2009/06/11
- Re: inotify back end for tail -f on linux,
Jim Meyering <=
- Re: inotify back end for tail -f on linux, Jim Meyering, 2009/06/13
- Re: inotify back end for tail -f on linux, Giuseppe Scrivano, 2009/06/14
- Re: inotify back end for tail -f on linux, Jim Meyering, 2009/06/15
- Re: inotify back end for tail -f on linux, Pádraig Brady, 2009/06/25
- Re: inotify back end for tail -f on linux, Jim Meyering, 2009/06/25
- Re: inotify back end for tail -f on linux, Pádraig Brady, 2009/06/25
- Re: inotify back end for tail -f on linux, Jim Meyering, 2009/06/25
- Re: inotify back end for tail -f on linux, Jim Meyering, 2009/06/06
- Re: inotify back end for tail -f on linux, Giuseppe Scrivano, 2009/06/06
- Re: inotify back end for tail -f on linux, Jim Meyering, 2009/06/07