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?= <ar...@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?= <ar...@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
