Date: Tue, 25 Aug 2009 23:08:15 +0200 From: Jilles Tjoelker <jilles@stack.nl> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Konstantin Belousov <kib@FreeBSD.org> Subject: Re: svn commit: r196460 - head/sys/kern Message-ID: <20090825210815.GA8792@stack.nl> In-Reply-To: <20090825031355.B729@besplex.bde.org> References: <200908231244.n7NCiFgc061095@svn.freebsd.org> <20090823213759.GA55039@stack.nl> <20090825031355.B729@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Aug 25, 2009 at 04:07:11AM +1000, Bruce Evans wrote:
> On Sun, 23 Aug 2009, Jilles Tjoelker wrote:
> > 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.
> This sort of worked for me, but has several problems:
> % 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:
> The second soshutdown() is only harmful. I see the following state changes
> - the first soshutdown() sets SBS_CANTRCVMORE on rso like you would expect,
> and also sets SBS_CANTSENDMORE on wso. This gives the desired state.
> - the second soshutdown() then clears SBS_CANTRCVMORE on rso (without
> the first soshutdown() it leaves both flags clear in both directions).
> This clobbers the desired state. The failure shows in just one of my
> uncommitted regression tests (when there is a writer and there was
> a reader, poll() returns POLLOUT for the writer, but should return
> POLLHUP; the missing SBS_CANTRCVMORE on rso prevents it ever returning
> POLLHUP for writers).
> After removing the second soshutdown() and fixing a spurious POLLIN (see
> below), all my tests pass.
I have removed the second shutdown, it is not necessary.
> Elsewhere, fifo_vnops.c hacks on SBS_CANT*MORE directly. Perhaps it should
> call soshutdown(), or if the direct access there is safe then it is probably
> safe above.
That's for a later commit to fix.
> % 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) {
> Old problems become larger:
> I don't like POLLINIGNEOF (for input) affecting POLLHUP for output. This
> seems to cause no problems for fifos, at least when the kernel sets
> POLLINIGNEOF, but it is hard to understand even why it doesn't cause
> problems, and kib@ wants POLLINIGNEOF to remain user-settable, so the
> complications might remain exported to userland for for fifos and
> sockets, where they are harder to document and understand.
I do not like userland POLLINIGNEOF either. I think programs can do fine
with the standard functionality (closing and reopening a fifo to reset
the POLLHUP state).
> % + revents |= events & (POLLIN | POLLRDNORM);
> This gives spurious POLLINs when POLLHUP is also returned, and thus defeats
> the point of soreadable_data() being different from soreadable(). Tests
> 6a-6d show the spurious POLLIN. I don't understand how tests 6a and 6c-6d
> passed for you.
Same problem here. I think kib@ wants to keep this in 8.x for the sake
of buggy programs that do not check for POLLHUP. I suppose we can do it
properly in 9.x.
> % + if (so->so_snd.sb_state & SBS_CANTSENDMORE)
> % + revents |= POLLHUP;
> Tests 6a-6d pass with the above 3 lines changed to:
> if (so->so_snd.sb_state & SBS_CANTSENDMORE)
> revents |= POLLHUP;
> else
> revents |= events & (POLLIN | POLLRDNORM);
> Returning POLLIN will cause poll() to not block on input descriptors, but
> this seems to be as correct as possible since there really is a read-EOF.
> Applications just can't see this EOF as POLLHUP -- they will see POLLIN
> and have to try to read(), and then interpret read() returning 0 as meaning
> EOF. Device-independent applications must do precisely this anyway since
> the input descriptor might be a regular file and there is no POLLHUP for
> regular files.
Yes. Even more, any program must handle it because it is also possible
that an EOF happens between poll() and read().
--
Jilles Tjoelker
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090825210815.GA8792>