net: add new method Sock::GetSockName() that wraps getsockname() and use it in GetBindAddress()

vasild commented at 12:58 pm on June 20, 2022: member

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

Wrap the syscall getsockname() in Sock::GetSockName() and change GetBindAddress() to take a Sock argument so that it can use the wrapper.

This further encapsulates syscalls inside the Sock class and makes the callers mockable.

net: add new method Sock::GetSockName() that wraps getsockname()

This will help to increase `Sock` usage and make more code mockable.

748dbcd9f2

net: change GetBindAddress() to take Sock argument

This avoids the direct call to `getsockname()` and allows mocking.

a8d6abba5e

DrahtBot added the label P2P on Jun 20, 2022

DrahtBot added the label Utils/log/libs on Jun 20, 2022

DrahtBot commented at 1:38 pm on June 20, 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:

#25421 (net: convert standalone IsSelectableSocket() and SetSocketNonBlocking() to Sock methods by vasild)
#23531 (Add Yggdrasil support by prusnak)
#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.

laanwj commented at 3:11 pm on June 20, 2022: member

Code review ACK a8d6abba5ec4ae2a3375e9be0b739f298899eca2 I was about to propose a different return value strategy for GetSockName (eg, return the data instead of take out-arg pointers) but i don’t think it’s worth it. At least this change is trivial to review like this.

in src/net.cpp:431 in a8d6abba5e

429     struct sockaddr_storage sockaddr_bind;
430     socklen_t sockaddr_bind_len = sizeof(sockaddr_bind);
431-    if (sock != INVALID_SOCKET) {
432-        if (!getsockname(sock, (struct sockaddr*)&sockaddr_bind, &sockaddr_bind_len)) {
433+    if (sock.Get() != INVALID_SOCKET) {
434+        if (!sock.GetSockName((struct sockaddr*)&sockaddr_bind, &sockaddr_bind_len)) {

laanwj commented at 8:59 pm on June 26, 2022:

I just reallize, this is the only use of getsockname. We could also add GetBindAddress to Sock directly? Or is it better to keep it separate from the Address abstraction?

vasild commented at 11:11 am on June 28, 2022:

Yeah, the intention of Sock is to be a thin wrapper over syscalls and not depend on or use any of the other Bitcoin Core stuff (mostly). Right now sock.{h,cpp} does not include any of the net* stuff because the net* stuff includes sock.h. I checked and using CAddress in Sock does not create a circular dependency, here is the diff:

  0diff --git i/src/net.cpp w/src/net.cpp
  1index 7499d4c72f..d026770565 100644
  2--- i/src/net.cpp
  3+++ w/src/net.cpp
  4@@ -418,28 +418,12 @@ bool CConnman::CheckIncomingNonce(uint64_t nonce)
  5         if (!pnode->fSuccessfullyConnected && !pnode->IsInboundConn() && pnode->GetLocalNonce() == nonce)
  6             return false;
  7     }
  8     return true;
  9 }
 10 
 11-/** Get the bind address for a socket as CAddress */
 12-static CAddress GetBindAddress(const Sock& sock)
 13-{
 14-    CAddress addr_bind;
 15-    struct sockaddr_storage sockaddr_bind;
 16-    socklen_t sockaddr_bind_len = sizeof(sockaddr_bind);
 17-    if (sock.Get() != INVALID_SOCKET) {
 18-        if (!sock.GetSockName((struct sockaddr*)&sockaddr_bind, &sockaddr_bind_len)) {
 19-            addr_bind.SetSockAddr((const struct sockaddr*)&sockaddr_bind);
 20-        } else {
 21-            LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "getsockname failed\n");
 22-        }
 23-    }
 24-    return addr_bind;
 25-}
 26-
 27 CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type)
 28 {
 29     assert(conn_type != ConnectionType::INBOUND);
 30 
 31     if (pszDest == nullptr) {
 32         if (IsLocal(addrConnect))
 33@@ -537,13 +521,13 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
 34     }
 35 
 36     // Add node
 37     NodeId id = GetNewNodeId();
 38     uint64_t nonce = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(id).Finalize();
 39     if (!addr_bind.IsValid()) {
 40-        addr_bind = GetBindAddress(*sock);
 41+        addr_bind = sock->GetBindAddress();
 42     }
 43     CNode* pnode = new CNode(id,
 44                              nLocalServices,
 45                              std::move(sock),
 46                              addrConnect,
 47                              CalculateKeyedNetGroup(addrConnect),
 48@@ -1151,13 +1135,13 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
 49     if (!addr.SetSockAddr((const struct sockaddr*)&sockaddr)) {
 50         LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "Unknown socket family\n");
 51     } else {
 52         addr = CAddress{MaybeFlipIPv6toCJDNS(addr), NODE_NONE};
 53     }
 54 
 55-    const CAddress addr_bind{MaybeFlipIPv6toCJDNS(GetBindAddress(*sock)), NODE_NONE};
 56+    const CAddress addr_bind{MaybeFlipIPv6toCJDNS(sock->GetBindAddress()), NODE_NONE};
 57 
 58     NetPermissionFlags permissionFlags = NetPermissionFlags::None;
 59     hListenSocket.AddSocketPermissionFlags(permissionFlags);
 60 
 61     CreateNodeFromAcceptedSocket(std::move(sock), permissionFlags, addr_bind, addr);
 62 }
 63diff --git i/src/util/sock.cpp w/src/util/sock.cpp
 64index b4c0aa4205..6b77ddd12a 100644
 65--- i/src/util/sock.cpp
 66+++ w/src/util/sock.cpp
 67@@ -1,12 +1,13 @@
 68 // Copyright (c) 2020-2021 The Bitcoin Core developers
 69 // Distributed under the MIT software license, see the accompanying
 70 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 71 
 72 #include <compat.h>
 73 #include <logging.h>
 74+#include <protocol.h>
 75 #include <threadinterrupt.h>
 76 #include <tinyformat.h>
 77 #include <util/sock.h>
 78 #include <util/syserror.h>
 79 #include <util/system.h>
 80 #include <util/time.h>
 81@@ -113,12 +114,27 @@ int Sock::SetSockOpt(int level, int opt_name, const void* opt_val, socklen_t opt
 82 
 83 int Sock::GetSockName(sockaddr* name, socklen_t* name_len) const
 84 {
 85     return getsockname(m_socket, name, name_len);
 86 }
 87 
 88+CAddress Sock::GetBindAddress() const
 89+{
 90+    CAddress addr_bind;
 91+    struct sockaddr_storage sockaddr_bind;
 92+    socklen_t sockaddr_bind_len = sizeof(sockaddr_bind);
 93+    if (m_socket != INVALID_SOCKET) {
 94+        if (!GetSockName((struct sockaddr*)&sockaddr_bind, &sockaddr_bind_len)) {
 95+            addr_bind.SetSockAddr((const struct sockaddr*)&sockaddr_bind);
 96+        } else {
 97+            LogPrintLevel(BCLog::NET, BCLog::Level::Warning, "getsockname failed\n");
 98+        }
 99+    }
100+    return addr_bind;
101+}
102+
103 bool Sock::Wait(std::chrono::milliseconds timeout, Event requested, Event* occurred) const
104 {
105     // We need a `shared_ptr` owning `this` for `WaitMany()`, but don't want
106     // `this` to be destroyed when the `shared_ptr` goes out of scope at the
107     // end of this function. Create it with a custom noop deleter.
108     std::shared_ptr<const Sock> shared{this, [](const Sock*) {}};
109diff --git i/src/util/sock.h w/src/util/sock.h
110index 96d0b3b56b..681126cdd2 100644
111--- i/src/util/sock.h
112+++ w/src/util/sock.h
113@@ -3,12 +3,13 @@
114 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
115 
116 #ifndef BITCOIN_UTIL_SOCK_H
117 #define BITCOIN_UTIL_SOCK_H
118 
119 #include <compat.h>
120+#include <protocol.h>
121 #include <threadinterrupt.h>
122 #include <util/time.h>
123 
124 #include <chrono>
125 #include <memory>
126 #include <string>
127@@ -130,12 +131,14 @@ public:
128      * getsockname(2) wrapper. Equivalent to
129      * `getsockname(this->Get(), name, name_len)`. Code that uses this
130      * wrapper can be unit tested if this method is overridden by a mock Sock implementation.
131      */
132     [[nodiscard]] virtual int GetSockName(sockaddr* name, socklen_t* name_len) const;
133 
134+    [[nodiscard]] virtual CAddress GetBindAddress() const;
135+
136     using Event = uint8_t;
137 
138     /**
139      * If passed to `Wait()`, then it will wait for readiness to read from the socket.
140      */
141     static constexpr Event RECV = 0b001;

What do you think? Should I include the above patch as a separate commit in this PR?

Off-scope for this PR: I think the return value of GetBindAddress() should be CService, not CAddress. It is just an IP address and a port.

laanwj commented at 11:35 am on June 28, 2022:

Yes, fair enough, i’m fine with keeping it as-is, just wanted to ask in case it was an obvious thing we forgot to consider.

Off-scope for this PR: I think the return value of GetBindAddress() should be CService, not CAddress. It is just an IP address and a port.

I agree! (actually had to look up what is what first the class names are so undescriptive :blush: ).

laanwj merged this on Jun 28, 2022

laanwj closed this on Jun 28, 2022

vasild deleted the branch on Jun 28, 2022

sidhujag referenced this in commit 9957e2cd9f on Jun 28, 2022

DrahtBot locked this on Jun 28, 2023

net: add new method Sock::GetSockName() that wraps getsockname() and use it in GetBindAddress() #25426

Conflicts