This is a piece of #21878, chopped off to ease review.
IsSelectableSocket() to Sock::IsSelectable()SetSocketNonBlocking() to Sock::SetNonBlocking()This further encapsulates syscalls inside the Sock class and makes the callers mockable.
Sock API.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
e7b846aa4d...0c4516b26b: rebase due to conflicts
Looks like this is the last one to go for #21878?
There is more! :) will be two more PRs:
Sock::Get() and Sock::Sock() - no PR yet because removing Sock::Get() will be possible once the construct IsSelectableSocket(sock->Get()) is removed by this PR
0c4516b26b...e99a11eb8c: rebase and add moveonly commits
Cumulative diff is identical before and after this forced push:
before: git diff 55c9e2d790..0c4516b26b
after: git diff 87d012324a..e99a11eb8c
This could be easier to review if you did a MOVEONLY commit before/after the changes
Better now?
To be converted to a method of the `Sock` class.
This makes the callers mockable.
To be converted to a method of the `Sock` class.
This further encapsulates syscalls inside the `Sock` class.
Co-authored-by: practicalswift <practicalswift@users.noreply.github.com>
e99a11eb8c...b527b54950: rebase due to conflicts
ACK b527b549504672704a61f70d2565b9489aaaba91 review/debug build/unit tests at each commit, cross-referenced the changes with man select and man errno, ran a signet node on the last commit with ip4/ip6//tor/i2p/cjdns and network connections were nominal
One global nit, this pull uses the doxygen format for classes on methods and variables that have a different doxygen format, per our developer notes.
One global nit, this pull uses the doxygen format for classes on methods and variables that have a different doxygen format, per our developer notes.
That is for consistency with surrounding code and because I assume (assumed) any doxygen-compatible comment is acceptable since the doxygen generated docs are identical either way. If it is desirable I could change all of sock.{h,cpp} to use some other style in a separate PR, but I think that would be just noise and waste of reviewing power.
124+ if (ioctlsocket(m_socket, FIONBIO, &on) == SOCKET_ERROR) {
125+ return false;
126+ }
127+#else
128+ const int flags{fcntl(m_socket, F_GETFL, 0)};
129+ if (flags == SOCKET_ERROR) {
We were previously not checking for an error on this fcntl call. Is that really necessary or would the next fcntl call fail when being passed SOCKET_ERROR | O_NONBLOCK?
(Not against adding the error check, just wanted to note that i noticed the addition)
fcntl() call would fail. The doc is a bit unclear what happens when passed unknown/invalid flags to fcntl(). Also, given that SOCKET_ERROR is -1 and O_NONBLOCK is 0x0004, then what is -1 | 0x0004? It is -1 actually, but is definitely something we don’t want to do.