Details

Reviewers

Commits

rG5fac6f16877f: poll(2): Add POLLRDHUP.
rGcc4c1e916a37: Update tools/regression/poll/sockpoll.c for POLLRDPOLL.
rG18f21f035548: Update tools/regression/poll/sockpoll.c for POLLRDPOLL.
rG3aaaa2efde89: poll(2): Add POLLRDHUP.

Summary

POSIX doesn't provide a way to find out if the other end has closed a socket without attempting to read (EOF) or write (connection reset etc). Linux 2.6.something added POLLRDHUP to detect that, and by now it's fairly widely used. It's a bit like POLLHUP, but it fires when remote shutdown(SHUT_WR) or close() sends a FIN, not only on errors/full-shutdown. At least illumos has also adopted this flag.

I am not sure what visibility the flag should have (__BSD_VISIBLE) or whether it should be in POLLSTANDARD.

Random examples of users of this flags in the wild (I happened to write one of these):

https://github.com/Exim/exim/blob/e4e884faa7f5a04d937282113681d97a355ed2af/src/src/acl.c#L3523
https://github.com/mysql/mysql-server/blob/7ed30a748964c009d4909cb8b4b22036ebdef239/vio/viosocket.cc#L750
https://github.com/postgres/postgres/blob/c30f54ad732ca5c8762bb68bbe0f51de9137dd72/src/backend/libpq/pqcomm.c#L1936

Test Plan

I added a simple test to tests/sys/netinet/socket_afinet.c. Is there a better place for this? It also works for Unix domain sockets, but I didn't include a test for that because it's going through the same code path anyway. I also considered tools/regression/poll/sockpoll.c but it is full of tests that fail already so I'm not sure what its status is.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

tmunro created this revision.Apr 14 2021, 5:37 AM

Herald added subscribers: melifaro, asomers. · View Herald TranscriptApr 14 2021, 5:37 AM

tmunro requested review of this revision.Apr 14 2021, 5:37 AM

Please re-upload the patch with content (diff -U9999999)

sys/kern/uipc_socket.c
3565	Are you sure that the right semantic is to not send POLLRDHUP if POLLINIGNEOF is set? POLLINIGNEOF is used to implement required semantic of the generations for named pipes AFAIR,

val_packett.cool added a subscriber: val_packett.cool.Apr 14 2021, 9:32 AM

val_packett.cool added inline comments.

lib/libc/sys/poll.2
275	maybe mention illumos here too?

This time with -U999999, and mentioning illumos as requested.

tmunro marked 2 inline comments as done.Apr 14 2021, 10:51 AM

tmunro added inline comments.

sys/kern/uipc_socket.c
3565	This is only intended to work for stream sockets. If we wanted to make it work for other kinds of descriptors, we'd have to invent the semantics for that because it doesn't work on other OSes either; it's not that clear what 'hangup' even means for a fifo anyway, since it can be opened and closed many times.

kib added inline comments.Apr 14 2021, 3:28 PM

sys/kern/uipc_socket.c
3565	Well, this is exactly why POLLINIGNEOF does. But if POLLRDHUP is only defined for streams, and POLLINIGNEOF is kernel-set bit for pipes/fifos, shouldn't it be completely ignored for POLLRDHUP?

Ok, now ignoring POLLINIGNEOF for this.

I am. in fact. more interested in a test which makes kernel report POLLRDHUP despite it not specified in events. Could you please add such test as well?

tests/sys/netinet/socket_afinet.c
28	Order include files properly.

Added a test to show that we don't report POLLRDHUP unless you asked for it.

This all looks fine to me, two notes:

Please split the test into two
Please take a look at tools/regression/poll even if you do not do anything there. But ideally you would add a test there and record the result for Linux and us.

I think item 1 is the only pre-req for the commit, item 2 can be handled later.

tmunro updated this revision to Diff 88159.Apr 26 2021, 11:08 AM

In D29757#668704, @kib wrote:

This all looks fine to me, two notes:

Please split the test into two

Done.

Please take a look at tools/regression/poll even if you do not do anything there. But ideally you would add a test there and record the result for Linux and us.

Done. Will commit that part separately.

Do you agree that this is MFCable?