Hi Carl, Apologies for my absence, Tuesdays and Wednesdays are long workdays for me.
Carl Edquist <edqu...@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
