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

[Pádraig Brady]

On 12/12/2022 22:29, Carl Edquist wrote:
> On Mon, 12 Dec 2022, Arsen Arsenović wrote:
>
> > Hi Carl,
> >
> > Carl Edquist <edquist@cs.wisc.edu> writes:
> >
> > > [2. text/x-diff; 0001-tee-only-fstat-outputs-if-pipe_check-is-active.patch]...
> > >
> > > [3. text/x-diff; 
> > > 0002-tee-skip-pipe-checks-if-input-is-always-ready-for-re.patch]...
> >
> > Thanks for writing these, and the other patches.  I've once again been
> > stripped of time, but I think we've nailed the concept down for the most
> > part.  I think we should wait for Pádraig to voice his opinion at this
> > point.  Details can be ironed out later, and pretty easily too.
>
> Sure...  Sorry for throwing so many patches at the list  :)
>
> I did have in mind also to send a summarized patch series in a single
> email to make reviewing everything a bit easier.  Pádraig is the
> gatekeeper here, in any case.
>
> The main question on my mind currently is still what's the best way to do
> the platform preprocessor logic for poll vs select ... (assuming
> HAVE_INOTIFY is only in tail.c for "tail -f" mode)
>
> And, i don't know if this would help "sell" it or not, but locally i have
> separated out the iopoll implementation to a separate iopoll.c/.h, and as
> an example successfully added a --pipe-check option to cat and tr.  (Not
> to pressure anyone to take it, but, sometimes it's easier to tell if you
> like an idea or not when you have a patch to look at...)
>
> Personally, i see this broken-pipe detection being useful in tee, cat,
> sed, and grep for starters.  (Though i know sed and grep live outside
> coreutils.)
>
> In some ways i feel like tee is actually the most immediate use case
> though.
>
> cat and sed mostly came into play for me as a way to type commands into a
> socket; though as it seems to me now, "broken-pipe" detection for a socket
> may not even be possible.

Thanks to both of you for working through the details on this.
This does seem like a useful feature, and would be appropriate to add.

A summarized patch set would be appreciated at this stage, thanks.

Re HAVE_INOTIFY, that's really a proxy for a linux kernel,
and so would be most appropriately changed to:
  defined __linux__ || defined __ANDROID__
I'm thinking these hardcoded defines would be best for now at least
as it covers the vast majority of systems, and avoids complicated
(cross) compile time checks.

A modularised iopoll.c would be better, given the potential
uses by other tools, though we'd probably release for just tee initially.

As for interface to this functionality I'm wondering if we
could just have the existing tee {-p,--output-error} imply the use of poll() on 
output.
I.e. from a high level -p just means to deal more appropriately with non file 
outputs,
and as part of that, dealing immediately with closed outputs would be an 
improvement.
Note also tail(1) enables this functionality by default.
I'm not sure about other utilities, but we can deal with that later if needed.

thanks!
Pádraig