svn commit: r238936 - in head/sys: fs/fifofs kern sys

[Bruce Evans]

On Thu, 2 Aug 2012, David Xu wrote:

> On 2012/8/2 3:29, Bruce Evans wrote:
>> On Wed, 1 Aug 2012, Giovanni Trematerra wrote:
>>> ...
>>> [gianni at bombay] /usr/src/tools/regression/poll#./pipepoll
>>> 1..20
>>> not ok 17 FIFO state 6a: expected POLLHUP; got POLLIN | POLLHUP
>>> not ok 18 FIFO state 6b: poll result 0 expected 1. expected POLLHUP; got 0
>>> not ok 19 FIFO state 6c: expected POLLHUP; got POLLIN | POLLHUP
>>> not ok 20 FIFO state 6d: expected POLLHUP; got POLLIN | POLLHUP
>>> 
>>> As you can see, sub-tests 6c and 6d failed too on 9. So it's not a problem 
>>> with
>>> new code though is irrelevant wrt the commit.
>> 
>> The failure is very differnt.  Failure to clear POLLIN in 6a, 6c and 6d
>> is a normal bug in FreeBSD.
>
> I have attached a patch to fix it, it should make the regression tool happy.
> Is it worth to commit ?

This is your patch quoted inline:

% Index: sys_pipe.c
% ===================================================================
% --- sys_pipe.c	(revision 238936)
% +++ sys_pipe.c	(working copy)
% @@ -1447,7 +1447,6 @@
% 
%  	if ((events & POLLINIGNEOF) == 0) {
%  		if (rpipe->pipe_state & PIPE_EOF) {
% -			revents |= (events & (POLLIN | POLLRDNORM));
%  			if (wpipe->pipe_present != PIPE_ACTIVE ||
%  			    (wpipe->pipe_state & PIPE_EOF))
%  				revents |= POLLHUP;

My old patches use this:

% Index: sys_pipe.c
% ===================================================================
% RCS file: /home/ncvs/src/sys/kern/sys_pipe.c,v
% retrieving revision 1.171
% diff -u -2 -r1.171 sys_pipe.c
% --- sys_pipe.c	27 Mar 2004 19:50:22 -0000	1.171
% +++ sys_pipe.c	13 Aug 2009 11:33:08 -0000
% @@ -1296,6 +1295,5 @@
%  	if (events & (POLLIN | POLLRDNORM))
%  		if ((rpipe->pipe_state & PIPE_DIRECTW) ||
% -		    (rpipe->pipe_buffer.cnt > 0) ||
% -		    (rpipe->pipe_state & PIPE_EOF))
% +		    (rpipe->pipe_buffer.cnt > 0))
%  			revents |= events & (POLLIN | POLLRDNORM);
%

I'm not sure if there is any difference.  The pipe code seems to have
been changed to be more like the socket code.

I made similar patches for sockets (to set POLLHUP on hangup (now in
-current) and to not set POLLIN on hangup unless there is still data
to be read).  I started killing POLLINIGNEOF for sockets.  -current
added it for nameless pipes instead :-(.  With the new fifo
implementation, POLLINIGNEOF is even more of a mistake for sockets,
but more needed for pipes since named pipes are fifos.

% Index: uipc_socket.c
% ===================================================================
% RCS file: /home/ncvs/src/sys/kern/uipc_socket.c,v
% retrieving revision 1.189
% diff -u -2 -r1.189 uipc_socket.c
% --- uipc_socket.c	24 Jun 2004 04:28:30 -0000	1.189
% +++ uipc_socket.c	26 Aug 2009 22:49:12 -0000
% @@ -1862,4 +1861,9 @@
%  }
% 
% +#define	soreadabledata(so) \
% +    (((so)->so_rcv.sb_cc > 0 && \
% +	(so)->so_rcv.sb_cc >= (so)->so_rcv.sb_lowat) || \
% +	!TAILQ_EMPTY(&(so)->so_comp) || (so)->so_error)
% +
%  int
%  sopoll(struct socket *so, int events, struct ucred *active_cred,

-current already has this in a header (don't count hangup as data).
But -current still sets POLLIN for compatibility later, except in the
bogus POLLINIGNEOF case.  It only uses the above change to avoid
setting POLLIN initially for hangup in the POLLINIGNEOF case.

% @@ -1869,12 +1873,7 @@
% 
%  	if (events & (POLLIN | POLLRDNORM))
% -		if (soreadable(so))
% +		if (soreadabledata(so))
%  			revents |= events & (POLLIN | POLLRDNORM);

Make POLLIN actually track data, not disconnection.

% 
% -	if (events & POLLINIGNEOF)
% -		if (so->so_rcv.sb_cc >= so->so_rcv.sb_lowat ||
% -		    !TAILQ_EMPTY(&so->so_comp) || so->so_error)
% -			revents |= POLLINIGNEOF;
% -
%  	if (events & (POLLOUT | POLLWRNORM))
%  		if (sowriteable(so))

Start killing this.

% @@ -1885,8 +1884,15 @@
%  			revents |= events & (POLLPRI | POLLRDBAND);
% 
% +	if ((events & POLLINIGNEOF) == 0) {
% +		if (so->so_rcv.sb_state & SBS_CANTRCVMORE) {
% +			if (so->so_snd.sb_state & SBS_CANTSENDMORE)
% +				revents |= POLLHUP;
% +			else
% +				revents |= events & (POLLIN | POLLRDNORM);
% +		}
% +	}
% +

Don't completely kill POLLINIGNEOF.  I forget how the 2 socket state EOF
flags work.  Both this and -current set POLLHUP iff both socket state
EOF flags are set.  Then this never sets POLLIN, while -current always
sets it.  Both set POLLIN of SBS_CANTRCVMORE is set but SBS_CANTSENDMORE
is not set.  Note that POLLIN has already been set if there is actual data,
so any setting of it here is bogus, but there seems to be a problem
when only SBS_CANTRCVMORE is set.  Then !SBS_CANTSENDMORE implies that
output is still possible, so we must be able to return POLLOUT, but
POLLOUT is incompatible with POLLHUP so we can't set POLLHUP.  We
apparently set POLLIN to fake this partial EOF.

%  	if (revents == 0) {
% -		if (events &
% -		    (POLLIN | POLLINIGNEOF | POLLPRI | POLLRDNORM |
% -		     POLLRDBAND)) {
% +		if (events & (POLLIN | POLLPRI | POLLRDNORM | POLLRDBAND)) {

Continue killing POLLINIGNEOF.

%  			SOCKBUF_LOCK(&so->so_rcv);
%  			selrecord(td, &so->so_rcv.sb_sel);

Bruce