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

[Arsen Arsenović]

Hi Padraig,

Pádraig Brady <P@draigBrady.com> writes:

> Really nice work on this.
> Only very small syntax tweaks (attached) at this point.
> I'm going to do testing with this and will add an appropriate test case.

I spotted some a slightly less minor error, and notified Carl off-list,
but you beat us to resubmitting a fixed patchset ;)

Namely, select (rfds, ...) would leave the state of rfds undefined.  On
Linux, this didn't cause errors, but I can definitely see it doing so on
other platforms.  I attached a patch that fixes that.  I also attached
the test case I mentioned.

From 2e26d25475b1541ff6f03685c671c63277b837d5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Arsen=20Arsenovi=C4=87?= <arsen@aarsen.me>
Date: Tue, 3 Jan 2023 18:05:07 +0100
Subject: [PATCH 1/2] iopoll: Fix select fd_set UB in iopoll

* src/iopoll.c (iopoll): Reinitialize rfds fd_set on each select
iteration.
---
 src/iopoll.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/src/iopoll.c b/src/iopoll.c
index 9424af6fa..e1714728c 100644
--- a/src/iopoll.c
+++ b/src/iopoll.c
@@ -66,19 +66,22 @@ iopoll(int fdin, int fdout)
 #else  /* fall back to select()-based implementation */
 
 extern int
-iopoll(int fdin, int fdout)
+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);
+  int ret = 0;
 
   /* If fdout has an error condition (like a broken pipe) it will be seen
      as ready for reading.  Assumes fdout is not actually readable.  */
-  while (select (nfds, &rfds, NULL, NULL, NULL) > 0 || errno == EINTR)
+  while (ret >= 0 || errno == EINTR)
     {
-      if (errno == EINTR)
+      fd_set rfds;
+      FD_ZERO (&rfds);
+      FD_SET (fdin, &rfds);
+      FD_SET (fdout, &rfds);
+      ret = select (nfds, &rfds, NULL, NULL, NULL);
+
+      if (ret < 0)
         continue;
       if (FD_ISSET (fdin, &rfds))  /* input available or EOF; should now */
         return 0;                  /* be able to read() without blocking */
-- 
2.39.0

From 28abc6c347e74a0e61dc3dfac40f09b186fab65f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Arsen=20Arsenovi=C4=87?= <arsen@aarsen.me>
Date: Tue, 3 Jan 2023 18:06:45 +0100
Subject: [PATCH 2/2] tests: Add test for tee -p

* tests/misc/tee.sh: Add tee -p test.
---
 tests/misc/tee.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tests/misc/tee.sh b/tests/misc/tee.sh
index 0bd91b6cb..6945865ff 100755
--- a/tests/misc/tee.sh
+++ b/tests/misc/tee.sh
@@ -63,6 +63,23 @@ if test -w /dev/full && test -c /dev/full; then
   test $(wc -l < err) = 1 || { cat err; fail=1; }
 fi
 
+# This is a testcase for the iopoll-powered read-write loop in tee.  In
+# essence, the test checks for sleep exiting as soon as all it's outputs die.
+# With the presence of some bashisms, this test could be more complete, so that
+# it includes tests for outputting to named pipes too, but the handling of
+# outputs in tee is sufficiently elegant to make it hopefully identical.
+#
+# Component breakdown of this pipeline:
+# - sleep emulates misbehaving input.
+# - The timeout is our failure safety-net.
+# - We ignore stderr from tee, and should have no stdout anyway.
+# - If the tee succeeds, we print TEST_PASSED into FD 8 to grep for later.
+# (FD 8 was selected by a D20 roll, or rather, a software emulation of one)
+# - The colon is the immediately closed output process.
+# - We redirect 8 back into stdout to grep it.
+( sleep 5 | (timeout 3 tee -p 2>/dev/null && echo TEST_PASSED >&8) | : ) 8>&1 \
+    | grep -x TEST_PASSED >/dev/null || fail=1
+
 
 # Ensure tee honors --output-error modes
 mkfifo_or_skip_ fifo
-- 
2.39.0

I originally wanted to include these squashed into the original two
commits, which is why I held off from posting an amended patchset.

Oh, I also just noticed: In the non-poll case, a warning will be emitted
because an undefined macro value is used in an #if.  Please also add a

  #else
  # define IOPOLL_USES_POLL 0

... branch.

Thanks, and happy holidays!
-- 
Arsen Arsenović

signature.asc
Description: PGP signature