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

[Arsen Arsenović]

Hi Carl,

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

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

> Alright, lest I be guilty of idle nay-saying, I've attached another patch to
> address all of my complaints.
>
> (Apply it after Arsen's last one, which comes after my previous one. Otherwise
> if desired I can send a single summary patch.)
>
> 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).

> Beyond that, I revised the select()-based implementation of iopoll() to 
> address
> my previous comments.  Sorry I got my grubby hands all over it.
> I do hope you'll like it though! :)

Thanks :)

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

/me shrugs

I cannot speak for many platforms, so I just opted to follow what tail.c
already did.

>> Ah!
>>
>> Well, it's not documented in my (oldish) select(2), but I do find the
>> following in a newer version of that manpage:
>>
>>
>>>  Correspondence between select() and poll() notifications
>>>
>>>   Within the Linux kernel source, we find the following definitions which
>>>   show the correspondence between the readable, writable, and exceptional
>>>   condition  notifications  of  select() and the event notifications pro-
>>>   vided by poll(2) (and epoll(7)):
>>>
>>>       #define POLLIN_SET (POLLRDNORM | POLLRDBAND | POLLIN | POLLHUP |
>>>                          POLLERR)
>>>                          /* Ready for reading */
>>>       #define POLLOUT_SET (POLLWRBAND | POLLWRNORM | POLLOUT | POLLERR)
>>>                          /* Ready for writing */
>>>       #define POLLEX_SET (POLLPRI)
>>>                          /* Exceptional condition */
>>> 
>>
>>
>> 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.

>> Also, the POLLEX_SET definition of (POLLPRI) doesn't seem relevant; so I
>> might suggest removing the 'xfd' arg for the select()-based implementation:
>>
>>>  POLLPRI
>>>
>>>    There is some exceptional  condition  on  the  file  descriptor.
>>>    Possibilities include:
>>>
>>>    *  There is out-of-band data on a TCP socket (see tcp(7)).
>>>
>>>    *  A  pseudoterminal  master  in  packet  mode  has seen a state
>>>       change on the slave (see ioctl_tty(2)).
>>>
>>>    *  A cgroup.events file has been modified (see cgroups(7)).

Yes, adding POLLPRI and xfds is likely excessive.  I did the former
while quite tired (so, under a misunderstanding), and the latter was a
translation of the former.

In any case, iopoll appears to be the path ahead, regardless of
implementation details.

Thanks again.
-- 
Arsen Arsenović

signature.asc
Description: PGP signature