Alright, lest I be guilty of idle nay-saying, I've attached another patch
to address all of my complaints.
(Apply it after Arsen's last one, which comes after my previous one.
Otherwise if desired I can send a single summary patch.)
Biggest item is making a new configure macro based on whether poll() is
present and and works as intended for pipes. With 0 timeout, polling the
write-end of a pipe that is open on both ends for errors does not indicate
a broken pipe; but polling the write-end of a pipe with the read-end
closed does indicate a broken pipe.
Hopefully this test covers all platforms. The only part that I'm iffy
about was this bit that Pádraig mentioned:
Portability problems fixed by Gnulib:
This function doesn't work on special files like @file{/dev/null} and
ttys like @file{/dev/tty} on some platforms: AIX 5.3, Mac OS X 10.4.0
If indeed there are issues on these platforms using poll() against these
special files (character devices), I suspect we can just test the relevant
behavior in the test program for the new configure macro. (eg, poll() for
/dev/null should always show POLLIN when open for reading, and should not
returning any errors when open for writing.)
Beyond that, I revised the select()-based implementation of iopoll() to
address my previous comments. Sorry I got my grubby hands all over it.
I do hope you'll like it though! :)
Cheers,
Carl
On Tue, 6 Dec 2022, Carl Edquist wrote:
(4.)
+ /* readable event on STDOUT is equivalent to POLLERR,
+ and implies an error condition on output like broken pipe. */
I know this is what the comment from tail.c says, but is it actually
documented to be true somewhere? And on what platforms? I don't see it
being documented in my select(2) on Linux, anyway. (Though it does seem
to work.) Wondering if this behavior is "standard".
Ah!
Well, it's not documented in my (oldish) select(2), but I do find the
following in a newer version of that manpage:
Correspondence between select() and poll() notifications
Within the Linux kernel source, we find the following definitions which
show the correspondence between the readable, writable, and exceptional
condition notifications of select() and the event notifications pro-
vided by poll(2) (and epoll(7)):
#define POLLIN_SET (POLLRDNORM | POLLRDBAND | POLLIN | POLLHUP |
POLLERR)
/* Ready for reading */
#define POLLOUT_SET (POLLWRBAND | POLLWRNORM | POLLOUT | POLLERR)
/* Ready for writing */
#define POLLEX_SET (POLLPRI)
/* Exceptional condition */
So I guess (on Linux at least) that means a "readable event on STDOUT" is
equivalent to (POLLRDNORM | POLLRDBAND | POLLIN | POLLHUP | POLLERR).
So, it'll catch the relevant poll() errors (POLLHUP | POLLERR), but the
inclusion of POLLIN results in the gotcha that it will be a false positive if
stdout is already open for RW (eg a socket) and there is actually data ready.
Also, the POLLEX_SET definition of (POLLPRI) doesn't seem relevant; so I
might suggest removing the 'xfd' arg for the select()-based implementation:
POLLPRI
There is some exceptional condition on the file descriptor.
Possibilities include:
* There is out-of-band data on a TCP socket (see tcp(7)).
* A pseudoterminal master in packet mode has seen a state
change on the slave (see ioctl_tty(2)).
* A cgroup.events file has been modified (see cgroups(7)).
Of course, those definitions live in the Linux kernel source, and on Linux
we'd get to use poll() instead of select().
Don't know if the select() <-> poll() correspondence differs at all on other
platforms .. ?
(But, this does give me a bit more confidence about select() being a (mostly)
viable alternative.)
Carl
From 26f991a2a3d8c3d4ea6300b5e00e47276d7544be Mon Sep 17 00:00:00 2001
From: Carl Edquist <edqu...@cs.wisc.edu>
Date: Wed, 7 Dec 2022 09:01:35 -0600
Subject: [PATCH] tee: define and use HAVE_POLL_PIPE_CLOSE_DETECTION
It's preferable to use native poll(2) wherever possible for broken pipe
detection, otherwise fall back to using select(2). Rather than doing
the preprocessor logic using a collection of macros representing
different platforms, simply define a macro at configure time based on
whether or not poll can be used for this purpose.
While we're at it, revise the select()-based implementation of iopoll().
('xfd' shouldn't be used; ret isn't needed; some comments needed
clarification; and a few items were arranged to match the poll()-based
version of iopoll().)
* configure.ac (HAVE_POLL_PIPE_CLOSE_DETECTION): New macro based on
program testing whether poll() can be used to detect broken pipes.
* src/tee.c (headers): include sys/select.h when using select()-based
implementation of iopoll().
(iopoll): use HAVE_POLL_PIPE_CLOSE_DETECTION macro to determine poll()
or select()-based implementation; revise select()-based implementation:
drop 'xfd' (shouldn't be checking exceptfds), improve/fix comments, drop
unused 'ret' variable, rearrange items to better match poll()-based
implementation.
---
configure.ac | 30 ++++++++++++++++++++++++++++
src/tee.c | 64 +++++++++++++++++++++++++++++-------------------------------
2 files changed, 61 insertions(+), 33 deletions(-)
diff --git a/configure.ac b/configure.ac
index 7e4afc9..24ab587 100644
--- a/configure.ac
+++ b/configure.ac
@@ -648,6 +648,36 @@ AM_CONDITIONAL([USE_AVX2_WC_LINECOUNT],
CFLAGS=$ac_save_CFLAGS
+AC_MSG_CHECKING([if poll available for pipe-close detection])
+AC_RUN_IFELSE(
+ [AC_LANG_SOURCE([[
+ #include <poll.h>
+ #include <unistd.h>
+
+ int
+ main (void)
+ {
+ int pipefd[2];
+ struct pollfd pfd;
+
+ if (pipe (pipefd) < 0)
+ return 1;
+ pfd = (struct pollfd) {pipefd[1], 0, 0};
+ return (poll (&pfd, 1, 0) == 0 &&
+ close (pipefd[0]) == 0 &&
+ poll (&pfd, 1, 0) == 1) ? 0 : 1;
+ }
+ ]])
+ ],[
+ AC_MSG_RESULT([yes])
+ AC_DEFINE([HAVE_POLL_PIPE_CLOSE_DETECTION], [1],
+ [poll available for pipe-close detection])
+ ],[
+ AC_MSG_RESULT([no])
+ AC_DEFINE([HAVE_POLL_PIPE_CLOSE_DETECTION], [0],
+ [poll available for pipe-close detection])
+ ])
+
############################################################################
dnl Autogenerated by the 'gen-lists-of-programs.sh' auxiliary script.
diff --git a/src/tee.c b/src/tee.c
index 7064ad2..b254466 100644
--- a/src/tee.c
+++ b/src/tee.c
@@ -20,7 +20,12 @@
#include <sys/types.h>
#include <signal.h>
#include <getopt.h>
+
+#if HAVE_POLL_PIPE_CLOSE_DETECTION
#include <poll.h>
+#else
+#include <sys/select.h>
+#endif
#include "system.h"
#include "argmatch.h"
@@ -194,10 +199,11 @@ main (int argc, char **argv)
fdout becomes a broken pipe, otherwise IOPOLL_ERROR if there is a poll()
error. */
+#if HAVE_POLL_PIPE_CLOSE_DETECTION
+
static int
iopoll(int fdin, int fdout)
{
-#if defined _AIX || defined __sun || defined __APPLE__ || HAVE_INOTIFY
struct pollfd pfds[2] = {{fdin, POLLIN, 0}, {fdout, 0, 0}};
while (poll (pfds, 2, -1) > 0 || errno == EINTR)
@@ -207,44 +213,36 @@ iopoll(int fdin, int fdout)
if (pfds[1].revents) /* POLLERR, POLLHUP, or POLLNVAL */
return IOPOLL_BROKEN_OUTPUT; /* output error or broken pipe */
}
-#else
- int ret;
- int bigger_fd = fdin > fdout ? fdin : fdout;
- fd_set rfd;
- fd_set xfd;
- FD_ZERO(&xfd);
- FD_ZERO(&rfd);
- FD_SET(fdout, &rfd);
- FD_SET(fdout, &xfd);
- FD_SET(fdin, &xfd);
- FD_SET(fdin, &rfd);
-
- /* readable event on STDOUT is equivalent to POLLERR,
- and implies an error condition on output like broken pipe. */
- while ((ret = select (bigger_fd + 1, &rfd, NULL, &xfd, NULL)) > 0
- || errno == EINTR)
+ return IOPOLL_ERROR; /* poll error */
+}
+
+#else /* fall back to select()-based implementation */
+
+static int
+iopoll(int fdin, int fdout)
+{
+ int nfds = (fdin > fdout ? fdin : fdout) + 1;
+ fd_set rfds;
+ FD_ZERO (&rfds);
+ FD_SET (fdin, &rfds);
+ FD_SET (fdout, &rfds);
+
+ /* If fdout has an error condition (like a broken pipe) it will be seen
+ as ready for reading. XXX: Assumes fdout is not actually readable. */
+ while (select (nfds, &rfds, NULL, NULL, NULL) > 0 || errno == EINTR)
{
if (errno == EINTR)
continue;
-
- if (ret < 0)
- break;
-
- if (FD_ISSET(fdout, &xfd) || FD_ISSET(fdout, &rfd))
- {
- /* Implies broken fdout. */
- return IOPOLL_BROKEN_OUTPUT;
- }
- else if (FD_ISSET(fdin, &xfd) || FD_ISSET(fdin, &rfd))
- {
- /* Something on input. Error handled in subsequent read. */
- return 0;
- }
+ if (FD_ISSET (fdin, &rfds)) /* input available or EOF; should now */
+ return 0; /* be able to read() without blocking */
+ if (FD_ISSET (fdout, &rfds)) /* POLLERR, POLLHUP, or POLLIN :( */
+ return IOPOLL_BROKEN_OUTPUT; /* output error or broken pipe */
}
-#endif
- return IOPOLL_ERROR; /* poll error */
+ return IOPOLL_ERROR; /* select error */
}
+#endif
+
/* Return the index of the first non-NULL descriptor after first_out,
or -1 if all are NULL. */
--
2.9.0
