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

[vasild]

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.

[laanwj]

Concept ACK. These methods clearly belong to the Sock API.

[DrahtBot]

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.

[laanwj]

Looks like this is the last one to go for #21878? Needs rebase, though.

[vasild]

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

[luke-jr]

This could be easier to review if you did a MOVEONLY commit before/after the changes

[vasild]

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]

Concept ACK

[vasild]

e99a11eb8c...b527b54950: rebase due to conflicts

[jonatack]

ACK b527b54 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]

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.

[dergoegge]

Code review ACK b527b54

[dergoegge]

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]

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.