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

[Carl Edquist]

On Thu, 8 Dec 2022, Arsen Arsenović wrote:

> Apologies for my absence, Tuesdays and Wednesdays are long workdays for 
> me.

No need for apologies - I feel like i am the one who should apologize for 
my high volume of email to the list.  People have lives after all!  :)

The timing of this thread caught my attention because I had recently been 
wrestling with a similar issue, trying to use shell utils to talk over a 
socket with the help of bash's /dev/tpc/host/port interface.

Similar to the situation here, i was seeing things annoyingly look like 
they are still 'alive' longer than they ought to be when providing input 
from the terminal.


> > Biggest item is making a new configure macro based on whether poll() is 
> > present and and works as intended for pipes.  With 0 timeout, polling 
> > the write-end of a pipe that is open on both ends for errors does not 
> > indicate a broken pipe; but polling the write-end of a pipe with the 
> > read-end closed does indicate a broken pipe.
>
> This might be a bit problematic when cross compiling (which is why I 
> imagine systems were hard-coded before).

Oh interesting.  That wasn't on my radar at all.  I guess this means that 
when cross-compiling, the configure script is run on the cross-compiling 
host, rather than on the target platform; so any test programs in 
configure.ac with AC_RUN_IFELSE don't necessarily check the target 
platform functionality (?)

That's too bad.  I had hoped to come up with a better way to indicate a 
working poll() for this feature than maintaining a list of platforms.


> > > So I guess (on Linux at least) that means a "readable event on STDOUT" is
> > > equivalent to (POLLRDNORM | POLLRDBAND | POLLIN | POLLHUP | POLLERR).
> > >
> > > So, it'll catch the relevant poll() errors (POLLHUP | POLLERR), but 
> > > the inclusion of POLLIN results in the gotcha that it will be a false 
> > > positive if stdout is already open for RW (eg a socket) and there is 
> > > actually data ready.
>
> Ah - yes.  tail.c guards against this by checking the type of the file
> descriptor before selecting it, and makes sure it's among the "one-way"
> file descriptors:
>
>  if (forever && ignore_fifo_and_pipe (F, n_files))
>    {
>      /* If stdout is a fifo or pipe, then monitor it
>         so that we exit if the reader goes away.  */
>      struct stat out_stat;
>      if (fstat (STDOUT_FILENO, &out_stat) < 0)
>        die (EXIT_FAILURE, errno, _("standard output"));
>      monitor_output = (S_ISFIFO (out_stat.st_mode)
>                        || (HAVE_FIFO_PIPES != 1 && isapipe (STDOUT_FILENO)));
>
> Good catch!  It completely slipped by my mind.

Ah, yeah if we know it's a pipe we shouldn't have to worry about an output 
being open for RW.

Originally i had imagined (or hoped) that this broken-pipe detection could 
also be used for sockets (that was how the issue came up for me), but it 
seems the semantics for sockets are different than for pipes.

Experimentally, it seems that once the remote read end of the socket is 
shutdown, poll() does not detect a broken pipe - it will wait 
indefinitely.  But at this point if a write() is done on the local end of 
the socket, the first write() will succeed, and then _after_ this it will 
behave like a broken pipe - poll() returns POLLERR|POLLHUP, and write() 
results in SIGPIPE/EPIPE.

It's fairly confusing.  But it seems due to the difference in semantics 
with sockets, likely this broken-pipe detection will only really work 
properly for pipes.

So yeah, back to your point, there is a little room for improvement here 
by fstat()ing the output and only doing the iopoll() waiting if the output 
is a pipe.

A quick note, this check only needs to be done a total of once per output, 
it shouldn't be done inside iopoll(), which would result in an additional 
redundant fstat() per read().

... Also, i suspect that the pipe_check option can be disabled if the 
_input_ is a regular file (or block device), since (i think) these always 
show up as ready for reading.  (This check would only need to be done once 
for fd 0 at program start.)

But ... one step at a time!  :)


Carl