Page MenuHomeFreeBSD
Differential D29757

POLLRDHUP
ClosedPublic
Actions

Authored by tmunro on Apr 14 2021, 5:37 AM.
Tags
None
Referenced Files
F106642764: D29757.diff
Fri, Jan 3, 6:07 AM
Unknown Object (File)
Sat, Dec 14, 6:34 AM
Unknown Object (File)
Sat, Dec 14, 1:12 AM
Unknown Object (File)
Nov 30 2024, 2:36 AM
Unknown Object (File)
Nov 22 2024, 5:53 AM
Unknown Object (File)
Nov 18 2024, 3:47 PM
Unknown Object (File)
Nov 16 2024, 11:58 AM
Unknown Object (File)
Nov 5 2024, 2:38 PM
View All Files
Subscribers
asomers
glebius
imp
kib
melifaro
mjg
val_packett.cool

Details

Reviewers
kib
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

Repository
rG FreeBSD src repository
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
kib added a comment.Apr 14 2021, 6:10 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?

tmunro updated this revision to Diff 87429.EditedApr 14 2021, 10:47 AM

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?

tmunro updated this revision to Diff 87610.Apr 17 2021, 10:43 AM
tmunro marked an inline comment as done.
tmunro marked an inline comment as done.

Ok, now ignoring POLLINIGNEOF for this.

kib added a comment.Apr 17 2021, 1:09 PM

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.

tmunro updated this revision to Diff 87614.Apr 17 2021, 2:08 PM
tmunro marked an inline comment as done.

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

kib added a comment.Apr 17 2021, 2:37 PM

This all looks fine to me, two notes:

  1. Please split the test into two
  2. 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
tmunro added a comment.Apr 26 2021, 11:12 AM
In D29757#668704, @kib wrote:

This all looks fine to me, two notes:

  1. Please split the test into two

Done.

  1. 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?

kib added a comment.Apr 26 2021, 11:23 AM
In D29757#672805, @tmunro wrote:

Do you agree that this is MFCable?

Sure, why would it not be?

Please upload the diff with the context.

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

-U999999

kib accepted this revision.Apr 26 2021, 12:17 PM
kib added inline comments.
lib/libc/sys/poll.2
129

Don't forget to bump .Dd

274

It is really strange that .Dv face is not used for POLL* constants.

This revision is now accepted and ready to land.Apr 26 2021, 12:17 PM
Closed by commit rG3aaaa2efde89: poll(2): Add POLLRDHUP. (authored by tmunro). · Explain WhyApr 28 2021, 11:20 AM
This revision was automatically updated to reflect the committed changes.
tmunro added a commit: rG3aaaa2efde89: poll(2): Add POLLRDHUP..
tmunro added a commit: rG18f21f035548: Update tools/regression/poll/sockpoll.c for POLLRDPOLL..
dchagin added a commit: rGcc4c1e916a37: Update tools/regression/poll/sockpoll.c for POLLRDPOLL..Jun 17 2022, 7:24 PM
Herald added subscribers: glebius, imp. · View Herald TranscriptJun 17 2022, 7:24 PM
dchagin added a commit: rG5fac6f16877f: poll(2): Add POLLRDHUP..Jun 17 2022, 7:24 PM

Revision Contents
Changeset List

PathSize
lib/
libc/
sys/
12 lines
sys/
kern/
4 lines
sys/
1 line
tests/
sys/
netinet/
141 lines
tools/
regression/
poll/
14/
22 lines
l/
22 lines
111 lines
usr.bin/
truss/
2 lines

Diff 88159

View Options

lib/libc/sys/poll.2

Loading...
View Options

sys/kern/uipc_socket.c

Loading...
View Options

sys/sys/poll.h

Loading...
View Options

tests/sys/netinet/socket_afinet.c

Loading...
View Options

tools/regression/poll/14/sockpoll.out

Loading...
View Options

tools/regression/poll/l/sockpoll.out

Loading...
View Options

tools/regression/poll/sockpoll.c

Loading...
View Options

usr.bin/truss/syscalls.c

Loading...