refactor: make setsockopt() and SetSocketNoDelay() mockable/testable #24357

pull vasild wants to merge 3 commits into bitcoin:master from vasild:mockable_SetSocketNoDelay_and_setsockopt changing 8 files +62 −20

vasild commented at 3:11 PM on February 16, 2022: member

This is a piece of #21878, chopped off to ease review.

Add a virtual (thus mockable) method Sock::SetSockOpt() that wraps the system setsockopt().

Convert the standalone SetSocketNoDelay() function to a virtual (thus mockable) method Sock::SetNoDelay().

This will help avoid syscalls during testing and to mock them to return whatever is suitable for the tests.
DrahtBot added the label P2P on Feb 16, 2022
DrahtBot added the label Refactoring on Feb 16, 2022
DrahtBot added the label Utils/log/libs on Feb 16, 2022
DrahtBot commented at 8:10 AM on February 17, 2022: member


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #24185 (refactor: only use explicit reinterpret/const casts, not implicit by PastaPastaPasta)
- #21878 (Make all networking code mockable 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.
jonatack commented at 4:56 PM on March 9, 2022: member

Concept ACK

in src/util/sock.h:120 in aaf8d7ed1f outdated

 114 | @@ -115,6 +115,22 @@ class Sock
 115 |                                           void* opt_val,
 116 |                                           socklen_t* opt_len) const;
 117 |  
 118 | +    /**
 119 | +     * setsockopt(2) wrapper. Equivalent to
 120 | +     * `setsockopt(this->Get(), level, opt_name, opt_val, opt_len)`. Code that uses this

jonatack commented at 10:47 PM on March 15, 2022:

03c630e

     * `setsockopt(/*sockfd=*/this->Get(), level, opt_name, opt_val, opt_len)`. Code that uses this

(same for getsockopt just above)

vasild commented at 11:41 AM on March 16, 2022:

Ignoring, on my system the parameter is named s: int setsockopt(int s, ...

jonatack commented at 12:08 AM on March 17, 2022:

Thanks, makes sense

in src/util/sock.h:126 in aaf8d7ed1f outdated
```
 121 | +     * wrapper can be unit tested if this method is overridden by a mock Sock implementation.
 122 | +     */
 123 | +    [[nodiscard]] virtual int SetSockOpt(int level,
 124 | +                                         int opt_name,
 125 | +                                         const void* opt_val,
 126 | +                                         socklen_t opt_len) const;
```
jonatack commented at 10:50 PM on March 15, 2022:

03c630e Placing all the SetSockOpt() declaration parameters on the same line, like in the definition, would be more consistent and easier to compare with the definition.
vasild commented at 11:43 AM on March 16, 2022:

Ignoring, the line would then become too long, and GetSockOpt() is already like that, so leaving it for consistency with surrounding code. Actually clang-format formats it like this, given a line length limit:

[[nodiscard]] virtual int SetSockOpt(int level, int opt_name, const void* opt_val, socklen_t opt_len) const;
jonatack commented at 12:14 AM on March 17, 2022:

given a line length limit:

There isn't a limit, for this project the line isn't particularly long, and it would be annoying for all the functions with several params to be formatted this way. That said, thanks for responding.
in src/net.cpp:2411 in aaf8d7ed1f outdated
```
2409 |          int nProtLevel = PROTECTION_LEVEL_UNRESTRICTED;
2410 | -        setsockopt(sock->Get(), IPPROTO_IPV6, IPV6_PROTECTION_LEVEL, (const char*)&nProtLevel, sizeof(int));
2411 | +        if (sock->SetSockOpt(IPPROTO_IPV6, IPV6_PROTECTION_LEVEL, (const char*)&nProtLevel, sizeof(int)) == SOCKET_ERROR) {
2412 | +            strError = strprintf(
2413 | +                Untranslated("Error setting IPV6_PROTECTION_LEVEL on socket: %s, continuing anyway"),
2414 | +                NetworkErrorString(WSAGetLastError()));
```
jonatack commented at 11:13 PM on March 15, 2022:

7c914fe2 These 3 error messages would be easier to read and review if they were on one line like the two pre-existing ones in the same BindListenPort() function. Also, the first added one is formatted differently from the next two added ones in this commit. Each being on one line would be simpler to write and to read.

vasild commented at 11:45 AM on March 16, 2022:

Changed to be all in one line, for consistency with the pre-existent ones (even though I do not like too long lines).

jonatack commented at 12:14 AM on March 17, 2022:

Thanks.
jonatack commented at 11:27 PM on March 15, 2022: member

ACK aaf8d7ed1fbcd2f1a90e4f02d50ca7d2864587d0

Some style feedback, feel free to ignore.
vasild force-pushed on Mar 16, 2022
vasild commented at 11:40 AM on March 16, 2022: member

aaf8d7ed1f...3ee641f452: style changes

Invalidates ACK from @jonatack
jonatack commented at 12:16 AM on March 17, 2022: member

ACK 3ee641f452dc98644246252613e7590ca5ad88f
in src/util/sock.cpp:113 in 3ee641f452 outdated
```
 104 | @@ -105,6 +105,17 @@ int Sock::GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len)
 105 |      return getsockopt(m_socket, level, opt_name, static_cast<char*>(opt_val), opt_len);
 106 |  }
 107 |  
 108 | +int Sock::SetSockOpt(int level, int opt_name, const void* opt_val, socklen_t opt_len) const
 109 | +{
 110 | +    return setsockopt(m_socket, level, opt_name, static_cast<const char*>(opt_val), opt_len);
 111 | +}
 112 | +
 113 | +bool Sock::SetNoDelay() const
```
laanwj commented at 6:52 AM on April 14, 2022:

It's a little bit ugly imo to have both a general SetSockOpt and a specific SetNoDelay on the same interface. I think we should decide either way:

Add a general SetSockOpt . Move the SetSockOpt(IPPROTO_TCP, TCP_NODELAY, &on, sizeof(on)) into the caller, or,

Add specific calls for SetNoDelay, ReuseAddr, V6only, etc etc.
vasild commented at 7:47 AM on April 15, 2022:

Good point! There is already a general SetSockOpt(), so I used it and ditched the SetNoDelay() method.
DrahtBot added the label Needs rebase on Apr 14, 2022
net: add new method Sock::SetSockOpt() that wraps setsockopt()
This will help to increase `Sock` usage and make more code mockable.
184e56d668
net: use Sock::SetSockOpt() instead of setsockopt() d65b6c3fb9

net: use Sock::SetSockOpt() instead of standalone SetSocketNoDelay()

Since the former is mockable, this makes it easier to test higher level
code that sets the TCP_NODELAY flag.

a2c4a7acd1

vasild force-pushed on Apr 15, 2022
vasild commented at 7:49 AM on April 15, 2022: member

3ee641f452...a2c4a7acd1: rebase due to conflicts and address suggestion.

Invalidates ACK from @jonatack
laanwj commented at 8:17 AM on April 15, 2022: member

Code review ACK a2c4a7acd1dfb2fb7e3c9dac6b3d8c9354b2e0a6
jonatack commented at 9:25 AM on April 15, 2022: member

ACK a2c4a7acd1dfb2fb7e3c9dac6b3d8c9354b2e0a6 change since last review is folding Sock::SetNoDelay() into the callers
DrahtBot removed the label Needs rebase on Apr 15, 2022
laanwj merged this on Apr 19, 2022
laanwj closed this on Apr 19, 2022
vasild deleted the branch on Apr 19, 2022
sidhujag referenced this in commit f41c649dc6 on Apr 19, 2022
DrahtBot locked this on Apr 19, 2023

Contributors

Labels

Refactoring P2P Utils/log/libs

Linked (view graph)

#21878 Make all networking code mockable #33378 Remove unnecessary casts when calling socket operations

refactor: make setsockopt() and SetSocketNoDelay() mockable/testable #24357

Conflicts