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

[Arsen Arsenović]

Carl Edquist <edquist@cs.wisc.edu> writes:

> 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.

Huh, I never tried that, honestly.  Did polling help your use-case?

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

Or worse, is unable to run at all (and always fails), if the binary is
for a different kernel or architecture.

> 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.

This might require POLLPRI or POLLRDHUP or such.  Can you try with those
to the set of events in pollfd?

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

Could this be handled by get_next_out?

> ... 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.)

Yes, there's no point poll-driving those, since it'll be always
readable, up to EOF, and never hesitate to bring more data.  It might
just end up being a no-op if used in current form (but I haven't tried).

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

Have a great day.
-- 
Arsen Arsenović

signature.asc
Description: PGP signature