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

[Pádraig Brady]

On 03/01/2023 17:49, Arsen Arsenović wrote:
> 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.

Squashed that in locally, thanks.

> I also attached the test case I mentioned.

Async stuff like this is tricky to test.
Your test waited at least 5 seconds, was still a bit racy,
and had some bashisms. I think I'll go with the following
which runs in around 0.1s normally.

diff --git a/tests/misc/tee.sh b/tests/misc/tee.sh
index b96523186..30d64a9d2 100755
--- a/tests/misc/tee.sh
+++ b/tests/misc/tee.sh
@@ -63,6 +63,16 @@ if test -w /dev/full && test -c /dev/full; then
   test $(wc -l < err) = 1 || { cat err; fail=1; }
 fi

+# Test iopoll-powered early exit for closed pipes
+tee_exited() { sleep $1; test -f tee.exited; }
+# Currently this functionality is most useful with
+# intermittent input from a terminal, but here we
+# use an input pipe that doesn't write anything
+# but will exit as soon as tee does, or it times out
+retry_delay_ tee_exited .1 7 | # 12.7s (Must be > following timeout)
+{ timeout 10 tee -p 2>err && touch tee.exited; } | :
+test $(wc -l < err) = 0 || { cat err; fail=1; }
+test -f tee.exited || fail=1


> 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

We rely quite often on -Wundef being disabled,
so it's probably best for cleaner code to avoid
the explicit define in that case.


cheers,
Pádraig