Re: [PATCH] tee: Add --pipe-check to allow instantly detecting closed outputs

[Arsen Arsenović]

Hi Carl,

Carl Edquist <edquist@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 <eggert@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ć  <arsen@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