net: convert standalone IsSelectableSocket() and SetSocketNonBlocking() to Sock methods #25421

pull vasild wants to merge 4 commits into bitcoin:master from vasild:convert_IsSelectableSocket_and_SetSocketNonBlocking changing 9 files +80 −34

vasild commented at 10:47 AM on June 20, 2022: contributor
This is a piece of #21878, chopped off to ease review.
- convert standalone IsSelectableSocket() to Sock::IsSelectable()
- convert standalone SetSocketNonBlocking() to Sock::SetNonBlocking()
This further encapsulates syscalls inside the Sock class and makes the callers mockable.
DrahtBot added the label P2P on Jun 20, 2022
DrahtBot added the label Utils/log/libs on Jun 20, 2022
laanwj commented at 11:22 AM on June 20, 2022: member

Concept ACK. These methods clearly belong to the Sock API.
DrahtBot commented at 1:44 PM on June 20, 2022: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #25619 (net: avoid overriding non-virtual ToString() in CService and use better naming by vasild)
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.
DrahtBot added the label Needs rebase on Jun 28, 2022
laanwj commented at 1:48 PM on June 28, 2022: member

Looks like this is the last one to go for #21878? Needs rebase, though.
vasild force-pushed on Jun 28, 2022
vasild commented at 2:57 PM on June 28, 2022: contributor
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:
- remove 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
- the fuzz tests themselves
DrahtBot removed the label Needs rebase on Jun 28, 2022
luke-jr commented at 12:10 AM on June 30, 2022: member

This could be easier to review if you did a MOVEONLY commit before/after the changes
vasild force-pushed on Jul 5, 2022
vasild commented at 10:30 AM on July 5, 2022: contributor

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?
theStack commented at 9:03 PM on July 16, 2022: contributor

Concept ACK
DrahtBot added the label Needs rebase on Jul 20, 2022
moveonly: move IsSelectableSocket() from compat.h to sock.{h,cpp}
To be converted to a method of the `Sock` class.
5db7d2ca0a
net: convert standalone IsSelectableSocket() to Sock::IsSelectable()
This makes the callers mockable.
b4bac55679
moveonly: move SetSocketNonBlocking() from netbase to util/sock
To be converted to a method of the `Sock` class.
29f66f7682

net: convert standalone SetSocketNonBlocking() to Sock::SetNonBlocking()

This further encapsulates syscalls inside the `Sock` class.

Co-authored-by: practicalswift <practicalswift@users.noreply.github.com>

b527b54950

vasild force-pushed on Jul 20, 2022
vasild commented at 2:27 PM on July 20, 2022: contributor

e99a11eb8c...b527b54950: rebase due to conflicts
DrahtBot removed the label Needs rebase on Jul 20, 2022
jonatack commented at 4:23 PM on July 22, 2022: contributor

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.
vasild commented at 11:10 AM on July 25, 2022: contributor

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.
in src/util/sock.cpp:129 in b527b54950
```
 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) {
```
dergoegge commented at 9:46 AM on October 7, 2022:

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)

vasild commented at 12:15 PM on October 7, 2022:

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.
dergoegge commented at 9:51 AM on October 7, 2022: member

Code review ACK b527b549504672704a61f70d2565b9489aaaba91
glozow merged this on Oct 12, 2022
glozow closed this on Oct 12, 2022
vasild deleted the branch on Oct 13, 2022
sidhujag referenced this in commit f73aef5a64 on Oct 23, 2022
bitcoin locked this on Oct 13, 2023

Contributors

Labels

Linked (view graph)

#21878 Make all networking code mockable #25870 Call for Maintainer: P2P & Networking

net: convert standalone IsSelectableSocket() and SetSocketNonBlocking() to Sock methods #25421

Conflicts