Re: tail -f: --pid *and* inotify
Pádraig Brady wrote: Pádraig Brady wrote: I'm not able to compile/test/push at the moment due to gnulib submodule weirdness. I was playing around with submodules today and thought I had messed up something. But I think the issue is that you synced to a private gnulib version? Hi Pádraig, Thanks for the heads up! I did indeed make a mistake. In testing prior to the move-update-copyright-to-gnulib change I committed a trail change in coreutils/gnulib, and never went back and fixed it. This calls for another syntax-check test. FYI, this is what I had: $ g show 29a35cf5baff3fa063a995e91c28986859ad865f commit 29a35cf5baff3fa063a995e91c28986859ad865f Merge: b53ff7b 5dac040 Author: Jim Meyering meyer...@redhat.com Date: Wed Jul 29 19:30:35 2009 +0200 Merge branch 'master' of /gnulib into HEAD I've just sync'd and pushed. ___ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: tail -f: --pid *and* inotify
I found another bug I think. Hopefully the attached is OK, but I'm not able to compile/test/push at the moment due to gnulib submodule weirdness. cheers, Pádraig. From 45e2e5f26d4d7641c3ef2bfc3e47aab5bc93b8fe Mon Sep 17 00:00:00 2001 From: =?utf-8?q?P=C3=A1draig=20Brady?= p...@draigbrady.com Date: Thu, 30 Jul 2009 15:29:13 +0100 Subject: [PATCH] tail: fix fractional seconds when monitoring a pid * src/tail.c (tail_forever_inotify): The fractional part of the delay was 1000 times to large. * tests/tail-2/pid: Add a test to ensure the timeout happens for this case. --- src/tail.c |2 +- tests/tail-2/pid |4 2 files changed, 5 insertions(+), 1 deletions(-) diff --git a/src/tail.c b/src/tail.c index 5efaf57..8aa31d5 100644 --- a/src/tail.c +++ b/src/tail.c @@ -1268,7 +1268,7 @@ tail_forever_inotify (int wd, struct File_spec *f, size_t n_files, FD_SET (wd, rfd); select_timeout.tv_sec = (time_t) sleep_interval; - select_timeout.tv_usec = 10 * (sleep_interval + select_timeout.tv_usec = 100 * (sleep_interval - select_timeout.tv_sec); n_descriptors = select (wd + 1, rfd, NULL, NULL, select_timeout); diff --git a/tests/tail-2/pid b/tests/tail-2/pid index 446c6db..ccf7823 100755 --- a/tests/tail-2/pid +++ b/tests/tail-2/pid @@ -70,4 +70,8 @@ fi getlimits_ tail --pid=$INT_MAX -f /dev/null || fail=1 +# Ensure sleep parameter is honoured with --pid +timeout 1 tail -s.1 -f /dev/null --pid=$PID_T_MAX +test $? = 124 fail=1 + Exit $fail -- 1.6.2.5 ___ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: tail -f: --pid *and* inotify
Pádraig Brady wrote: I found another bug I think. Hopefully the attached is OK, but I'm not able to compile/test/push at the moment due to gnulib submodule weirdness. Thanks! Pushed. ___ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: tail -f: --pid *and* inotify
Giuseppe Scrivano wrote:
Jim Meyering j...@meyering.net writes:
A couple of points:
Please move these declarations down into the scope where they are used.
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.
Thanks for the advises. I cleaned it following them.
I was checking for the pid every time exactly to avoid the problem that
it is never checked and can run indefinitely even if the process is
already not running.
Maybe we can add a real clock controlling how much time passed since
last check or even simpler, a counter like don't exit without check
more than N consecutive times. What do you think?
Thanks.
I've pushed it with this change to avoid a warning about the
PID parameter shadowing the global. It's best always to enable
all warnings, if you can:
./configure --enable-gcc-warnings
then such warnings are promoted to errors and stop the build.
diff --git a/src/tail.c b/src/tail.c
index 1474b06..a73ffa2 100644
--- a/src/tail.c
+++ b/src/tail.c
@@ -1164,7 +1164,7 @@ 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, int pid,
+tail_forever_inotify (int wd, struct File_spec *f, size_t n_files,
double sleep_interval)
{
size_t i;
@@ -1256,7 +1256,7 @@ tail_forever_inotify (int wd, struct File_spec *f, size_t
n_files, int pid,
struct inotify_event *ev;
- /* If a process is watched be sure that read from wd will not block
+ /* When watching a PID, ensure that a read from WD will not block
indefinetely. */
if (pid)
{
@@ -1278,7 +1278,7 @@ tail_forever_inotify (int wd, struct File_spec *f, size_t
n_files, int pid,
if (n_descriptors == 0)
{
- /* Check if the process we are monitoring is still alive. */
+ /* See if the process we are monitoring is still alive. */
if (kill (pid, 0) != 0 errno != EPERM)
break;
@@ -1978,7 +1978,7 @@ main (int argc, char **argv)
error (0, errno, _(inotify cannot be used, reverting to polling));
else
{
- tail_forever_inotify (wd, F, n_files, pid, sleep_interval);
+ tail_forever_inotify (wd, F, n_files, sleep_interval);
/* The only way the above returns is upon failure. */
exit (EXIT_FAILURE);
___
Bug-coreutils mailing list
Bug-coreutils@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: tail -f: --pid *and* inotify
Giuseppe Scrivano wrote:
Jim Meyering j...@meyering.net 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 gscriv...@gnu.org
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 = 10 * (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.
___
Bug-coreutils mailing list
Bug-coreutils@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: tail -f: --pid *and* inotify
Jim Meyering j...@meyering.net writes:
A couple of points:
Please move these declarations down into the scope where they are used.
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.
Thanks for the advises. I cleaned it following them.
I was checking for the pid every time exactly to avoid the problem that
it is never checked and can run indefinitely even if the process is
already not running.
Maybe we can add a real clock controlling how much time passed since
last check or even simpler, a counter like don't exit without check
more than N consecutive times. What do you think?
Regards,
Giuseppe
From 65f2737fa6e2519fbccbad7d285ca8923a893057 Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano gscriv...@gnu.org
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 | 52 +---
1 files changed, 41 insertions(+), 11 deletions(-)
diff --git a/src/tail.c b/src/tail.c
index fd44e22..1474b06 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;
@@ -1253,6 +1256,36 @@ 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)
+{
+ fd_set rfd;
+ struct timeval select_timeout;
+ int n_descriptors;
+
+ FD_ZERO (rfd);
+ FD_SET (wd, rfd);
+
+ select_timeout.tv_sec = (time_t) sleep_interval;
+ select_timeout.tv_usec = 10 * (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));
+
+ if (n_descriptors == 0)
+{
+ /* Check if the process we are monitoring is still alive. */
+ if (kill (pid, 0) != 0 errno != EPERM)
+break;
+
+ continue;
+}
+}
+
if (len = evbuf_off)
{
len = safe_read (wd, evbuf, evlen);
@@ -1940,18 +1973,15 @@ main (int argc, char **argv)
if (forever)
{
#if HAVE_INOTIFY
- if (pid == 0)
+ int wd = inotify_init ();
+ if (wd 0)
+error (0, errno, _(inotify cannot be used, reverting to polling));
+ else
{
- int wd = inotify_init ();
- if (wd 0)
-error (0, errno, _(inotify cannot be used, reverting to
polling));
- else
-{
- tail_forever_inotify (wd, F, n_files);
+ tail_forever_inotify (wd, F, n_files, pid, sleep_interval);
- /* The only way the above returns is upon failure. */
- exit (EXIT_FAILURE);
-}
+ /* The only way the above returns is upon failure. */
+ exit (EXIT_FAILURE);
}
#endif
tail_forever (F, n_files, sleep_interval);
--
1.6.3.3
___
Bug-coreutils mailing list
Bug-coreutils@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-coreutils
tail -f: --pid *and* inotify
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. ___ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils
Re: tail -f: --pid *and* inotify
Hi Jim,
Jim Meyering j...@meyering.net 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.
Regards,
Giuseppe
From 862c9d934dc2ee188e9fe29985e463e2ec4d16ca Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano gscriv...@gnu.org
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;
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 = 10 * (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;
+}
+
if (len = evbuf_off)
{
len = safe_read (wd, evbuf, evlen);
@@ -1940,18 +1970,15 @@ main (int argc, char **argv)
if (forever)
{
#if HAVE_INOTIFY
- if (pid == 0)
+ int wd = inotify_init ();
+ if (wd 0)
+error (0, errno, _(inotify cannot be used, reverting to polling));
+ else
{
- int wd = inotify_init ();
- if (wd 0)
-error (0, errno, _(inotify cannot be used, reverting to
polling));
- else
-{
- tail_forever_inotify (wd, F, n_files);
+ tail_forever_inotify (wd, F, n_files, pid, sleep_interval);
- /* The only way the above returns is upon failure. */
- exit (EXIT_FAILURE);
-}
+ /* The only way the above returns is upon failure. */
+ exit (EXIT_FAILURE);
}
#endif
tail_forever (F, n_files, sleep_interval);
--
1.6.3.3
___
Bug-coreutils mailing list
Bug-coreutils@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-coreutils
