Hi Carl, Carl Edquist <edqu...@cs.wisc.edu> writes:
> Originally I had in mind to put the read() call inside the poll() loop. But if
> we keep this feature as an option, it felt it was a bit easier just to add the
> "if (pipe_check) {...}" block before the read().
Yes, I do agree that this is likely cleaner.
> For Pádraig, I think the same function & approach here could be used in other
> filters (cat for example). The stubborn part of me might say, for platforms
> that do not natively support poll(2), we could simply leave out support for
> this feature. If that's not acceptable, we could add a select(2)-based
> fallback for platforms that do not have a native poll(2).
There's no need to omit it. iopoll() seems sufficiently easy to
implement via select():
From 582e0a27b7995aac90cc360463ec8bde1a44cfe4 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Mon, 5 Dec 2022 18:42:19 -0800
Subject: [PATCH] tee: Support select fallback path
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
2022-12-06 Arsen Arsenović <ar...@aarsen.me>
iopoll: Support select fallback path
* src/tee.c (iopoll): Add logic to enable select usage.
---
src/tee.c | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/src/tee.c b/src/tee.c
index c17c5c788..7064ad27d 100644
--- a/src/tee.c
+++ b/src/tee.c
@@ -197,6 +197,7 @@ main (int argc, char **argv)
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)
@@ -206,7 +207,41 @@ 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)
+ {
+ 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;
+ }
+ }
+#endif
return IOPOLL_ERROR; /* poll error */
}
--
2.38.1
Note that I also needed to replace the ``/* falls through */'' comment with [[fallthrough]]; to build your patch on gcc 12.2.1 20221008. I'd guess there's some way to pick the correct marking method. I tested both codepaths in the patch above on Linux. I suggest adding the test case I provided before to test on more platforms (and I'll give some VMs a shot when I get home; currently in a lecture). The API here seems quite general, I'd be surprised if other utils couldn't make use of it too, though, maybe it should be given a slightly more descriptive name (iopoll feels a bit broad, maybe select_inout () to signify that it makes a selection between one input or one output exactly). > Unique to tee is its multiple outputs. The new get_next_out() helper simply > advances to select the next remaining (ie, not-yet-removed) output. As > described last time, it's sufficient to track a single output at a time, and > perhaps it even simplifies the implementation. It also avoids the need for a > malloc() for the pollfd array before every read(). I think this is okay, I struggle to find a case where it couldn't work. Note that removing polled files from a pollfd array does not require any reallocation (just setting the fd to -1, as in the code I initially posted), so there's no malloc either way ;). Thanks for working on this, have a great day. -- Arsen Arsenović
signature.asc
Description: PGP signature
