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.
Concept ACK. These methods clearly belong to the Sock API.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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
This could be easier to review if you did a MOVEONLY commit before/after the changes
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?
Concept ACK
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)
I do not know if the next 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.
Code review ACK b527b549504672704a61f70d2565b9489aaaba91