Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 22 Mar 2006 21:20:16 GMT
From:      Oliver Fromme <olli@lurza.secnetix.de>
To:        freebsd-bugs@FreeBSD.org
Subject:   Re: kern/94772: FIFOs (named pipes) + select() == broken
Message-ID:  <200603222120.k2MLKG2C034313@freefall.freebsd.org>

next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/94772; it has been noted by GNATS.

From: Oliver Fromme <olli@lurza.secnetix.de>
To: bde@zeta.org.au (Bruce Evans)
Cc: bug-followup@freebsd.org
Subject: Re: kern/94772: FIFOs (named pipes) + select() == broken
Date: Wed, 22 Mar 2006 22:12:32 +0100 (CET)

 Hi,
 
 Sorry for replying to myself, but there are a few new
 things ...
 
 Oliver Fromme wrote:
  > Bruce Evans wrote:
  > > On Tue, 21 Mar 2006, Oliver Fromme wrote:
  > > > When a FIFO had been opened for reading and the writing
  > > > process closes it, the reading process blocks in select(),
  > > > even though the descriptor is ready for read().  If the
  > > > select() call is omitted, read() returns 0 immediately
  > > > indicating EOF.  But as soon as you use select(), it blocks
  > > > and there is no chance to detect the EOF condition.
  > > 
  > > See also:
  > > 
  > > PR 76125 (about the same bug)
  > > PR 76144 (about a related bug in poll())
  > > PR 53447 (about a another or the same related bug in poll())
  > > PR 34020 (about the inverse of the bug (return on non-EOF) for select()
  > >            and poll())
 
 The first one (76125) seems completely unrelated.  Typo?
 
  > (By the way, DEC UNIX 4.0D _does_ have a bug:  If the FIFO
  > has O_NONBLOCK set and no writer has opened the FIFO, then
  > select() doesn't block.
 
 Actually, it's not a bug.  I've read SUSv3 wrong.  That
 behaviour is perfectly fine.  In fact, SUSv3 (a.k.a.
 POSIX-2001) requires that select() doesn't block in that
 case, and the behaviour of select() and poll() must be
 independet of whether O_NONBLOCK is set or not.
 
  > > Fix (?):
  > > - actually implement returning POLLHUP in sopoll() and other places.  Return
  > >    POLLHUP but not POLLIN for the pure-EOF case.  Return POLLIN* | POLLHUP
  > >    for EOF-with-data.
  > > - remove POLLINIGNEOF and associated complications in sopoll(), fifo_poll()
  > >    and <sys/poll.h>
  > > - change selscan() to check for POLLHUP so that POLLIN, POLLIN | POLLHUP
  > >    and POLLHUP all act the same for select()
  > > - remove POLLHUP from the comment in selscan().  Fix the rest of this
  > >    comment or remove it (most backends are too broken to return poll flags
  > >    if appropriate, and the comment only mentions one of the other poll flags
  > >    that selscan() ignores)
  > > - remove the corresponding comment in pollscan() since it is wrong and says
  > >    nothing relevant (pollscan() just accepts whatever flags the backends set).
  > 
  > I would be happy to help out, but I'm not familiar with
  > that part of the kernel code.
 
 Well, now (a few hours later) I'm a little bit more
 familiar with it.  Patch attached below, and also
 available from this URL:
 http://www.secnetix.de/~olli/tmp/fifodiff.txt
 
 With that patch, my own test program (the one from
 the top of this PR) runs successfully, i.e. EOF is
 recognized correctly in all cases that I have tested
 (with and without select()), and it also behaves
 standard-compliant when O_NONBLOCK is used (see
 above).
 
 Also, with that patch applied, your test program
 runs successfully (without producing any output).
 I've tested everything under RELENG_6 (cvsupped
 today).  I've also had a look at the Current sources,
 and the relevant parts don't seem to be any different,
 so the diff should be applicable to HEAD as well.
 
 The patch addresses the following things:
  - implement POLLHUP in sopoll().
  - remove POLLINIGNEOF.
  - selscan() doesn't need to be patched: it already
    works as expected when fo_poll() returns POLLHUP.
  - I don't think the comment is entirely incorrect,
    but I'm not sure, so I left it alone.
 
 Please give this patch a try and let me know if there
 are any problems (functional, style, whatever).  As
 far as I can tell, the patch fixes the existing bugs
 with FIFOs + select()/poll(), so I would be happy to
 see it committed to Current and RELENG_6.  (Maybe
 even in time for the release of 6.1?)
 
 Best regards
    Oliver
 
 
 --- src/sys/fs/fifofs/fifo_vnops.c.orig	Tue Mar 21 09:42:32 2006
 +++ src/sys/fs/fifofs/fifo_vnops.c	Wed Mar 22 18:49:27 2006
 @@ -661,31 +661,11 @@
  	int levents, revents = 0;
  
  	fip = fp->f_data;
 -	levents = events &
 -	    (POLLIN | POLLINIGNEOF | POLLPRI | POLLRDNORM | POLLRDBAND);
 +	levents = events & (POLLIN | POLLPRI | POLLRDNORM | POLLRDBAND);
  	if ((fp->f_flag & FREAD) && levents) {
 -		/*
 -		 * If POLLIN or POLLRDNORM is requested and POLLINIGNEOF is
 -		 * not, then convert the first two to the last one.  This
 -		 * tells the socket poll function to ignore EOF so that we
 -		 * block if there is no writer (and no data).  Callers can
 -		 * set POLLINIGNEOF to get non-blocking behavior.
 -		 */
 -		if (levents & (POLLIN | POLLRDNORM) &&
 -		    !(levents & POLLINIGNEOF)) {
 -			levents &= ~(POLLIN | POLLRDNORM);
 -			levents |= POLLINIGNEOF;
 -		}
 -
  		filetmp.f_data = fip->fi_readsock;
  		filetmp.f_cred = cred;
  		revents |= soo_poll(&filetmp, levents, cred, td);
 -
 -		/* Reverse the above conversion. */
 -		if ((revents & POLLINIGNEOF) && !(events & POLLINIGNEOF)) {
 -			revents |= (events & (POLLIN | POLLRDNORM));
 -			revents &= ~POLLINIGNEOF;
 -		}
  	}
  	levents = events & (POLLOUT | POLLWRNORM | POLLWRBAND);
  	if ((fp->f_flag & FWRITE) && levents) {
 --- src/sys/kern/uipc_socket.c.orig	Wed Dec 28 19:05:13 2005
 +++ src/sys/kern/uipc_socket.c	Wed Mar 22 18:44:08 2006
 @@ -2036,23 +2036,26 @@
  		if (soreadable(so))
  			revents |= events & (POLLIN | POLLRDNORM);
  
 -	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))
 -			revents |= events & (POLLOUT | POLLWRNORM);
 +	if (events & (POLLOUT | POLLWRNORM) && sowriteable(so))
 +		revents |= events & (POLLOUT | POLLWRNORM);
 +	else {
 +		/*
 +		 * POLLOUT and POLLHUP shall not both be set.
 +		 * Therefore check only for POLLHUP if POLLOUT
 +		 * has not been set.  (Note that POLLHUP need
 +		 * not be in events; it's always checked.)
 +		 */
 +		if (so->so_rcv.sb_state & SBS_CANTRCVMORE &&
 +		    so->so_rcv.sb_cc == 0)
 +			revents |= POLLHUP;
 +	}
  
  	if (events & (POLLPRI | POLLRDBAND))
  		if (so->so_oobmark || (so->so_rcv.sb_state & SBS_RCVATMARK))
  			revents |= events & (POLLPRI | POLLRDBAND);
  
  	if (revents == 0) {
 -		if (events &
 -		    (POLLIN | POLLINIGNEOF | POLLPRI | POLLRDNORM |
 -		     POLLRDBAND)) {
 +		if (events & (POLLIN | POLLPRI | POLLRDNORM | POLLRDBAND)) {
  			selrecord(td, &so->so_rcv.sb_sel);
  			so->so_rcv.sb_flags |= SB_SEL;
  		}
 --- src/sys/sys/poll.h.orig	Wed Jul 10 06:47:25 2002
 +++ src/sys/sys/poll.h	Wed Mar 22 18:41:03 2006
 @@ -66,11 +66,6 @@
  #define	POLLRDBAND	0x0080		/* OOB/Urgent readable data */
  #define	POLLWRBAND	0x0100		/* OOB/Urgent data can be written */
  
 -#if __BSD_VISIBLE
 -/* General FreeBSD extension (currently only supported for sockets): */
 -#define	POLLINIGNEOF	0x2000		/* like POLLIN, except ignore EOF */
 -#endif
 -
  /*
   * These events are set if they occur regardless of whether they were
   * requested.
 
 
 
 -- 
 Oliver Fromme,  secnetix GmbH & Co. KG, Marktplatz 29, 85567 Grafing
 Dienstleistungen mit Schwerpunkt FreeBSD: http://www.secnetix.de/bsd
 Any opinions expressed in this message may be personal to the author
 and may not necessarily reflect the opinions of secnetix in any way.
 
 "I learned Java 3 years before Python.  It was my language of
 choice.  It took me two weekends with Python before I was more
 productive with it than with Java." -- Anthony Roberts



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200603222120.k2MLKG2C034313>