From owner-svn-src-all@FreeBSD.ORG Sun Aug 23 21:38:02 2009 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 0A997106568E; Sun, 23 Aug 2009 21:38:02 +0000 (UTC) (envelope-from jilles@stack.nl) Received: from mx1.stack.nl (relay02.stack.nl [IPv6:2001:610:1108:5010::104]) by mx1.freebsd.org (Postfix) with ESMTP id 9508B8FC1D; Sun, 23 Aug 2009 21:38:01 +0000 (UTC) Received: from snail.stack.nl (snail.stack.nl [IPv6:2001:610:1108:5010::131]) by mx1.stack.nl (Postfix) with ESMTP id 488AB35A836; Sun, 23 Aug 2009 23:38:00 +0200 (CEST) Received: by snail.stack.nl (Postfix, from userid 1677) id 34B3D228CD; Sun, 23 Aug 2009 23:38:00 +0200 (CEST) Date: Sun, 23 Aug 2009 23:38:00 +0200 From: Jilles Tjoelker To: Konstantin Belousov Message-ID: <20090823213759.GA55039@stack.nl> References: <200908231244.n7NCiFgc061095@svn.freebsd.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="n8g4imXOkfNTN/H1" Content-Disposition: inline In-Reply-To: <200908231244.n7NCiFgc061095@svn.freebsd.org> User-Agent: Mutt/1.5.18 (2008-05-17) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r196460 - head/sys/kern X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 23 Aug 2009 21:38:02 -0000 --n8g4imXOkfNTN/H1 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Sun, Aug 23, 2009 at 12:44:15PM +0000, Konstantin Belousov wrote: > Author: kib > Date: Sun Aug 23 12:44:15 2009 > New Revision: 196460 > URL: http://svn.freebsd.org/changeset/base/196460 > Log: > Fix the conformance of poll(2) for sockets after r195423 by > returning POLLHUP instead of POLLIN for several cases. Now, the > tools/regression/poll results for FreeBSD are closer to that of the > Solaris and Linux. > Also, improve the POSIX conformance by explicitely clearing POLLOUT > when POLLHUP is reported in pollscan(), making the fix global. > Submitted by: bde > Reviewed by: rwatson > MFC after: 1 week > Modified: > head/sys/kern/sys_generic.c > head/sys/kern/uipc_socket.c > Modified: head/sys/kern/sys_generic.c > ============================================================================== > --- head/sys/kern/sys_generic.c Sun Aug 23 12:23:24 2009 (r196459) > +++ head/sys/kern/sys_generic.c Sun Aug 23 12:44:15 2009 (r196460) > @@ -1228,6 +1228,13 @@ pollscan(td, fds, nfd) > selfdalloc(td, fds); > fds->revents = fo_poll(fp, fds->events, > td->td_ucred, td); > + /* > + * POSIX requires POLLOUT to be never > + * set simultaneously with POLLHUP. > + */ > + if ((fds->revents & POLLHUP) != 0) > + fds->revents &= ~POLLOUT; > + > if (fds->revents != 0) > n++; > } This looks useful. > Modified: head/sys/kern/uipc_socket.c > ============================================================================== > --- head/sys/kern/uipc_socket.c Sun Aug 23 12:23:24 2009 (r196459) > +++ head/sys/kern/uipc_socket.c Sun Aug 23 12:44:15 2009 (r196460) > @@ -2898,13 +2898,11 @@ sopoll_generic(struct socket *so, int ev > if (so->so_oobmark || (so->so_rcv.sb_state & SBS_RCVATMARK)) > revents |= events & (POLLPRI | POLLRDBAND); > > - if ((events & POLLINIGNEOF) == 0) { > - if (so->so_rcv.sb_state & SBS_CANTRCVMORE) { > - revents |= events & (POLLIN | POLLRDNORM); > - if (so->so_snd.sb_state & SBS_CANTSENDMORE) > - revents |= POLLHUP; > - } > - } > + if ((events & POLLINIGNEOF) == 0) > + if (so->so_rcv.sb_state & SBS_CANTRCVMORE) > + revents |= POLLHUP; > + if (so->so_snd.sb_state & SBS_CANTSENDMORE) > + revents |= POLLHUP; > > if (revents == 0) { > if (events & (POLLIN | POLLPRI | POLLRDNORM | POLLRDBAND)) { This sets POLLHUP when either of the directions has shut down, instead of the entire connection. This breaks use of select and poll with shutdown(2): * sending some data, shutdown(SHUT_WR) then polling for input * (the other side for the above) receiving some data, getting EOF on that direction then polling for output * a paranoid HTTP-like server that wants to wait for the client to close the read end after shutting down the write end Either some data is lost or the program busy-waits for the data. I think the POLLHUP setting before this commit was correct for sockets: POLLHUP is set if both directions are closed/error. An EOF that only affects one direction sets the corresponding POLLIN/POLLOUT so that the EOF becomes known but the other direction can still be used normally. (The POSIX spec explicitly describes something like this for POLLIN (zero length message) although it erroneously restricts it to STREAMS files only; the POLLOUT case has to be derived from the fact that the read end should still work normally but the EOF should be notified.) I think poll on fifos should instead be fixed by closing the half-connection corresponding to writing from fi_readsock to fi_writesock. I have tried this out, see attached patch. With the patch, pipepoll only gives "expected POLLHUP; got POLLIN | POLLHUP" errors and an error in fifo case 6b caused by our distinction between new and old readers. tools/regression/poll does not test shutdown(2) interaction, so it does not find this problem. -- Jilles Tjoelker --n8g4imXOkfNTN/H1 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="poll-fifo.patch" Index: sys/kern/uipc_socket.c =================================================================== --- sys/kern/uipc_socket.c (revision 196469) +++ sys/kern/uipc_socket.c (working copy) @@ -2898,11 +2898,13 @@ if (so->so_oobmark || (so->so_rcv.sb_state & SBS_RCVATMARK)) revents |= events & (POLLPRI | POLLRDBAND); - if ((events & POLLINIGNEOF) == 0) - if (so->so_rcv.sb_state & SBS_CANTRCVMORE) - revents |= POLLHUP; - if (so->so_snd.sb_state & SBS_CANTSENDMORE) - revents |= POLLHUP; + if ((events & POLLINIGNEOF) == 0) { + if (so->so_rcv.sb_state & SBS_CANTRCVMORE) { + revents |= events & (POLLIN | POLLRDNORM); + if (so->so_snd.sb_state & SBS_CANTSENDMORE) + revents |= POLLHUP; + } + } if (revents == 0) { if (events & (POLLIN | POLLPRI | POLLRDNORM | POLLRDBAND)) { Index: sys/fs/fifofs/fifo_vnops.c =================================================================== --- sys/fs/fifofs/fifo_vnops.c (revision 196459) +++ sys/fs/fifofs/fifo_vnops.c (working copy) @@ -193,6 +193,10 @@ goto fail2; fip->fi_writesock = wso; error = soconnect2(wso, rso); + if (error == 0) + error = soshutdown(rso, SHUT_WR); + if (error == 0) + error = soshutdown(wso, SHUT_RD); if (error) { (void)soclose(wso); fail2: --n8g4imXOkfNTN/H1--