Remove Sock::Get() and Sock::Sock() #26312
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
|
Why not add additional testing/mocking/fuzzing in this PR since this seems to be laying the groundwork for that? |
|
@yancyribbens, to ease review. Right now, #21878 consists of this PR + the tests (it is almost complete 🎈). If somebody has spare review cycles they can review/ACK #21878 directly. |
|
@vasild If the tests are not yet complete, then that may change this PR, right? Besides adding more safety, seeing the accompanying tests would make this easier (for me anyway) to review. |
The tests are "complete" in a sense that they are "production ready" and can be merged. "Not complete" in a sense that more tests can be added. I do not think that tweaking the tests can change this PR. The point of #21878 is to have high level code not deal with
The tests are in #21878, feel free to look at them ❤️ |
Thanks for the additional explanation. I was wondering if you could explain this a bit more. It's been a while since i've done anything with int sockets but if I remember, it was in
I looked over 21878 and I see there's some additional fuzz testing however I don't see the |
|
The Before #21878 we would use that OS C API directly from all over the place. E.g.: int s = socket(...); // create the socket, see man 2 socket
...
connect(s, to_addr); // see man 2 connectThe problem with this is that in order to test it we have to create a real socket that really connects to the given address which is very inconvenient. It cannot be mocked. #21878 aims to replace all such calls to the operating system with constructs like Sock sock = CreateSock(); // can return Sock, FuzzedSock, StaticContentsSock or another class that implements the interface
sock->Connect(to_addr);This way a mock |
I understand. What I am asking is if you wouldn't mind adding the |
I assume that by "the Mock tests" you mean the tests that are in the last 4 commits of #21878, right? If I add them to this PR then this PR would be the same as #21878. I created this PR as a smaller piece of #21878 to ease review. If you want to peek or fully review the tests, then see the last 4 commits in #21878.
Because those tests depend on removing the |
|
Are there any Mock tests that are not part of fuzz tests? |
|
There is one in |
|
Which PR is the one |
|
It is already merged. |
|
I'm confused. I thought this PR was required before a |
|
Depending on the code being tested various methods of |
7b6fe67 to
666c758
Compare
|
|
|
ACK 666c758 modulo trivial rebase |
666c758 to
c40c257
Compare
|
Invalidates ACK from @jonatack |
There was a problem hiding this comment.
re-ACK c40c257 per git range-diff dc905f6 666c758 c40c257
It's good to have them in place, just noting that the Sock::IsConnected() virtual member function overrides in src/test/util/net.h and src/test/fuzz/util/net.{h.cpp} aren't used currently or here or in the parent yet.
|
re-ACK e536b3a per |
|
This is the last part of parent #21878 opened 2 years ago, anyone else like to have a look? |
Peeking at the underlying socket file descriptor of `Sock` and checkig if it is `INVALID_SOCKET` is bad encapsulation and stands in the way of testing/mocking/fuzzing. Instead use an empty unique_ptr to denote that there is no valid socket.
The socket is always valid (the underlying file descriptor is not `INVALID_SOCKET`) when `GetBindAddress()` is called.
The socket is always valid (the underlying file descriptor is not `INVALID_SOCKET`) when `ConnectSocketDirectly()` is called.
e536b3a to
b44e493
Compare
|
|
`Sock::Get()` was used only in `sock.{cpp,h}`. Remove it and access
`Sock::m_socket` directly.
Unit tests that used `Get()` to test for equality still verify that the
behavior is correct by using the added `operator==()`.
b44e493 to
384e9e3
Compare
|
|
Use also a socket for the moved-to object and check which one is closed when.
384e9e3 to
7df4508
Compare
|
|
|
ACK 7df4508 |
|
ACK 7df4508 |
|
rfm or is anything left to be done here? |
@vasild pinged me on irc, and agree looks this looks ready for merge. Will merge shortly |
7df4508 test: improve sock_tests/move_assignment (Vasil Dimov) 5086a99 net: remove Sock default constructor, it's not necessary (Vasil Dimov) 7829272 net: remove now unnecessary Sock::Get() (Vasil Dimov) 944b21b net: don't check if the socket is valid in ConnectSocketDirectly() (Vasil Dimov) aeac68d net: don't check if the socket is valid in GetBindAddress() (Vasil Dimov) 5ac1a51 i2p: avoid using Sock::Get() for checking for a valid socket (Vasil Dimov) Pull request description: _This is a piece of bitcoin#21878, chopped off to ease review._ Peeking at the underlying socket file descriptor of `Sock` and checkig if it is `INVALID_SOCKET` is bad encapsulation and stands in the way of testing/mocking/fuzzing. Instead use an empty `unique_ptr` to denote that there is no valid socket where appropriate or outright remove such checks where they are not necessary. The default constructor `Sock::Sock()` is unnecessary now after recent changes, thus remove it. ACKs for top commit: ajtowns: ACK 7df4508 jonatack: ACK 7df4508 Tree-SHA512: 9742aeeeabe8690530bf74caa6ba296787028c52f4a3342afd193b05dbbb1f6645935c33ba0a5230199a09af01c666bd3c7fb16b48692a0d185356ea59a8ddbf
p2p: Refactor sock to add I2P unit tests (backported from bitcoin/bitcoin#21387) refactor: replace CConnman::SocketEvents() with mockable Sock::WaitMany() (backported from bitcoin/bitcoin#24356) refactor: make setsockopt() and SetSocketNoDelay() mockable/testable (backported from bitcoin/bitcoin#24357) I2P: add support for transient addresses for outbound connections (backported from bitcoin/bitcoin#25355) Remove Sock::Get() and Sock::Sock() (backported from bitcoin/bitcoin#26312) Remove now-unnecessary poll, fcntl includes from net(base).cpp (backported from bitcoin/bitcoin#27530)
This is a piece of #21878, chopped off to ease review.
Peeking at the underlying socket file descriptor of
Sockand checkig if it isINVALID_SOCKETis bad encapsulation and stands in the way of testing/mocking/fuzzing.Instead use an empty
unique_ptrto denote that there is no valid socket where appropriate or outright remove such checks where they are not necessary.The default constructor
Sock::Sock()is unnecessary now after recent changes, thus remove it.