Date: Wed, 26 Aug 2009 17:08:57 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Jilles Tjoelker <jilles@stack.nl> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Konstantin Belousov <kib@FreeBSD.org>, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r196460 - head/sys/kern Message-ID: <20090826163931.C41472@delplex.bde.org> In-Reply-To: <20090825210815.GA8792@stack.nl> References: <200908231244.n7NCiFgc061095@svn.freebsd.org> <20090823213759.GA55039@stack.nl> <20090825031355.B729@besplex.bde.org> <20090825210815.GA8792@stack.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 25 Aug 2009, Jilles Tjoelker wrote:
> 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.
The second shutdown became harmless for me when I fixed the clobbering of
sb_state. Does it have any effect?
>> % 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.
But it may break non-buggy programs, and doesn't help for buggy programs
like gdb. (gdb exits immediately when it sees POLLHUP, despite POLLIN
also being set and input being queued. gdb used to work before it was
converted to use poll(). Then it saw a ready input fd and had to read()
it to detect EOF.)
I now have better tests for POLLOUT, and have tested the buggy case
for select some more (test 9, and 6b (?) in current): if POLLINIGNEOF
is not applied for select(), then test 9 passes as expected. Test 9
only covers a fifo in initial state with a reader and no writers. This
is EOF for read() and input-ready for select(), but neither POLLIN nor
POLLHUP for poll(). Test 6b is for essentially the initial state for
read() and select(), but for poll() the state is too-sticky hangup
(POSIX) or back to the initial state (Linux-2.6.10/FreeBSD-current).
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090826163931.C41472>