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

[Carl Edquist]

Hi Arsen, thanks for your feedback!

On Tue, 6 Dec 2022, Arsen Arsenović wrote:

> > 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():

So, you're right...

I think the stubborn part of me was reacting (badly) to what looked to be 
a messy situation in tail.c's mix of poll() and select().  I got further 
disenchanted when i saw gnulib's select()-based emulation of poll()  :(

But hey, if it works...


Anyway a few comments:

(1.)

> +#if defined _AIX || defined __sun || defined __APPLE__ || HAVE_INOTIFY

I know this is what tail.c has, but I would like to seriously rethink 
this.  What do these macros have to do with poll()?  (And just as 
importantly, how can you tell by looking at it?)

First of all, I think poll(2) (the native system one, not the gnulib 
emulation) should be used wherever possible.  Which is to say, select() 
should only be used where native poll() is unavailable (or if it's 
"available" but broken in some important way).

I believe the HAVE_INOTIFY bit is specific to tail.c, which relates to its 
inotify mode.  I don't think it should be part of the equation here in 
tee.c.

The remaining macros _AIX, __sun, __APPLE__ -- do these cover all systems 
that have a working native poll() -- eg, Linux and the BSDs?  (I suspect 
not?)  And they do not clearly communicate anything to the reader about 
why they were picked / what they represent in relation to the availability 
of poll().

*Ideally,* there might be some configure macro like HAVE_POLL, except one 
that refers specifically to the system one and not the gnulib emulation. 
Something like "HAVE_NATIVE_poll".  Then it would be clear what the 
preprocessor logic is trying to accomplish.


(2.) if we want to use #if preprocessor logic (and preferably "#if 
HAVE_NATIVE_poll") for this poll/select fallback mechanism, I'd suggest to 
include each entire version of the function (including the function header 
and comments) in separate preprocessor blocks.  (One complete function in 
#if, and the other in #else.) The iopoll() implementation I provided is 
straightforward by itself, but its readability gets trampled badly when 
trying to mix the body of the select()-based implementation in with it. 
Just my 2 cents.


(3.)

> +  FD_SET(fdout, &xfd);
> +  FD_SET(fdin, &xfd);

Is it documented somewhere that the 'exceptfds' argument (xfd) to 
select(2) can be used in a useful way here?

The only thing I've seen (from select_tut(2)) is that these can be used to 
detect out-of-band data for reading on a socket, or that control status 
information is available for a pty in packet mode (tty_ioctl(4)).  I 
believe both of these would be false positives if encountered on input, 
and it's not clear what they would mean for output.


(4.)

> +  /* readable event on STDOUT is equivalent to POLLERR,
> +     and implies an error condition on output like broken pipe.  */

I know this is what the comment from tail.c says, but is it actually 
documented to be true somewhere?  And on what platforms?  I don't see it 
being documented in my select(2) on Linux, anyway.  (Though it does seem 
to work.)  Wondering if this behavior is "standard".


I guess there is also a little gotcha.  In tee, all the outputs *except* 
stdout are explicitly open()ed, but stdout itself is not, and therefore it 
inherits the open mode that fd 1 had when tee was exec()ed.

In particular, if fd 1 / stdout is a socket, open for RW, then a "readable 
event on STDOUT" could be a legitimate indication that data is ready for 
reading.  So that would be a false positive causing stdout to be removed 
even though successful writes are still possible.

(Maybe nobody will care about this, but it's an interesting corner-case.)

...

"But other than that", sure!  I guess I'm all for it  :)

Carl