[lldb] Updated TCPSocket to listen multiple ports on the same single thread #104797
Conversation
…thread This is prerequisite for llvm#104238.
|
@llvm/pr-subscribers-lldb Author: Dmitry Vasilyev (slydiman) ChangesThis is prerequisite for #104238. Full diff: https://github.com/llvm/llvm-project/pull/104797.diff 3 Files Affected:
diff --git a/lldb/include/lldb/Host/common/TCPSocket.h b/lldb/include/lldb/Host/common/TCPSocket.h
index b782c9e6096c44..b45fdae00c6b43 100644
--- a/lldb/include/lldb/Host/common/TCPSocket.h
+++ b/lldb/include/lldb/Host/common/TCPSocket.h
@@ -24,6 +24,9 @@ class TCPSocket : public Socket {
// returns port number or 0 if error
uint16_t GetLocalPortNumber() const;
+ // returns port numbers of all listening sockets
+ std::set<uint16_t> GetLocalPortNumbers() const;
+
// returns ip address string or empty string if error
std::string GetLocalIPAddress() const;
diff --git a/lldb/source/Host/common/TCPSocket.cpp b/lldb/source/Host/common/TCPSocket.cpp
index df4737216ed3ac..e659b20bb0045e 100644
--- a/lldb/source/Host/common/TCPSocket.cpp
+++ b/lldb/source/Host/common/TCPSocket.cpp
@@ -17,6 +17,7 @@
#include "lldb/Utility/LLDBLog.h"
#include "lldb/Utility/Log.h"
+#include "llvm/ADT/StringExtras.h"
#include "llvm/Config/llvm-config.h"
#include "llvm/Support/Errno.h"
#include "llvm/Support/WindowsError.h"
@@ -94,6 +95,25 @@ uint16_t TCPSocket::GetLocalPortNumber() const {
return 0;
}
+// Return all the port numbers that is being used by the socket.
+std::set<uint16_t> TCPSocket::GetLocalPortNumbers() const {
+ std::set<uint16_t> ports;
+ if (m_socket != kInvalidSocketValue) {
+ SocketAddress sock_addr;
+ socklen_t sock_addr_len = sock_addr.GetMaxLength();
+ if (::getsockname(m_socket, sock_addr, &sock_addr_len) == 0)
+ ports.insert(sock_addr.GetPort());
+ } else if (!m_listen_sockets.empty()) {
+ SocketAddress sock_addr;
+ socklen_t sock_addr_len = sock_addr.GetMaxLength();
+ for (auto listen_socket : m_listen_sockets) {
+ if (::getsockname(listen_socket.first, sock_addr, &sock_addr_len) == 0)
+ ports.insert(sock_addr.GetPort());
+ }
+ }
+ return ports;
+}
+
std::string TCPSocket::GetLocalIPAddress() const {
// We bound to port zero, so we need to figure out which port we actually
// bound to
@@ -196,49 +216,66 @@ Status TCPSocket::Listen(llvm::StringRef name, int backlog) {
if (!host_port)
return Status(host_port.takeError());
+ llvm::SmallVector<uint16_t, 2> ports;
+ ports.push_back(host_port->port);
+
+ llvm::SmallVector<llvm::StringRef, 2> extra_ports;
+ name.split(extra_ports, ',', -1, false);
+ if (extra_ports.size() > 1) {
+ for (auto i = extra_ports.begin() + 1; i != extra_ports.end(); ++i) {
+ uint16_t port;
+ if (!llvm::to_integer(*i, port, 10))
+ return Status("invalid extra port number %s", i->str().c_str());
+ ports.push_back(port);
+ }
+ }
+
if (host_port->hostname == "*")
host_port->hostname = "0.0.0.0";
std::vector<SocketAddress> addresses = SocketAddress::GetAddressInfo(
host_port->hostname.c_str(), nullptr, AF_UNSPEC, SOCK_STREAM, IPPROTO_TCP);
for (SocketAddress &address : addresses) {
- int fd = Socket::CreateSocket(address.GetFamily(), kType, IPPROTO_TCP,
- m_child_processes_inherit, error);
- if (error.Fail() || fd < 0)
- continue;
-
- // enable local address reuse
- int option_value = 1;
- set_socket_option_arg_type option_value_p =
- reinterpret_cast<set_socket_option_arg_type>(&option_value);
- if (::setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, option_value_p,
- sizeof(option_value)) == -1) {
- CLOSE_SOCKET(fd);
- continue;
- }
-
- SocketAddress listen_address = address;
- if(!listen_address.IsLocalhost())
- listen_address.SetToAnyAddress(address.GetFamily(), host_port->port);
- else
- listen_address.SetPort(host_port->port);
+ for (size_t i = 0; i < ports.size(); ++i) {
+ int fd = Socket::CreateSocket(address.GetFamily(), kType, IPPROTO_TCP,
+ m_child_processes_inherit, error);
+ if (error.Fail() || fd < 0)
+ continue;
+
+ // enable local address reuse
+ int option_value = 1;
+ set_socket_option_arg_type option_value_p =
+ reinterpret_cast<set_socket_option_arg_type>(&option_value);
+ if (::setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, option_value_p,
+ sizeof(option_value)) == -1) {
+ CLOSE_SOCKET(fd);
+ continue;
+ }
- int err =
- ::bind(fd, &listen_address.sockaddr(), listen_address.GetLength());
- if (err != -1)
- err = ::listen(fd, backlog);
+ SocketAddress listen_address = address;
+ if (!listen_address.IsLocalhost())
+ listen_address.SetToAnyAddress(address.GetFamily(), ports[i]);
+ else
+ listen_address.SetPort(ports[i]);
+
+ int err =
+ ::bind(fd, &listen_address.sockaddr(), listen_address.GetLength());
+ if (err != -1)
+ err = ::listen(fd, backlog);
+
+ if (err == -1) {
+ error = GetLastSocketError();
+ CLOSE_SOCKET(fd);
+ continue;
+ }
- if (err == -1) {
- error = GetLastSocketError();
- CLOSE_SOCKET(fd);
- continue;
- }
+ if (ports[i] == 0) {
+ socklen_t sa_len = address.GetLength();
+ if (getsockname(fd, &address.sockaddr(), &sa_len) == 0)
+ ports[i] = address.GetPort();
+ }
- if (host_port->port == 0) {
- socklen_t sa_len = address.GetLength();
- if (getsockname(fd, &address.sockaddr(), &sa_len) == 0)
- host_port->port = address.GetPort();
+ m_listen_sockets[fd] = address;
}
- m_listen_sockets[fd] = address;
}
if (m_listen_sockets.empty()) {
diff --git a/lldb/unittests/Host/SocketTest.cpp b/lldb/unittests/Host/SocketTest.cpp
index 78e1e11df06562..3e87af57d3ce51 100644
--- a/lldb/unittests/Host/SocketTest.cpp
+++ b/lldb/unittests/Host/SocketTest.cpp
@@ -136,6 +136,19 @@ TEST_P(SocketTest, TCPListen0GetPort) {
EXPECT_NE(sock.get()->GetLocalPortNumber(), 0);
}
+TEST_P(SocketTest, TCPListen00GetPort) {
+ if (!HostSupportsIPv4())
+ return;
+ llvm::Expected<std::unique_ptr<TCPSocket>> sock =
+ Socket::TcpListen("10.10.12.3:0,0", false);
+ ASSERT_THAT_EXPECTED(sock, llvm::Succeeded());
+ ASSERT_TRUE(sock.get()->IsValid());
+ std::set<uint16_t> ports = sock.get()->GetLocalPortNumbers();
+ ASSERT_EQ(2, ports.size());
+ EXPECT_NE(*ports.begin(), 0);
+ EXPECT_NE(*std::next(ports.begin()), 0);
+}
+
TEST_P(SocketTest, TCPGetConnectURI) {
std::unique_ptr<TCPSocket> socket_a_up;
std::unique_ptr<TCPSocket> socket_b_up;
|
|
Having a single socket listen on multiple ports sounds like a bad idea to me. The socket class is complicated enough as it is, and it's not the way the MainLoop class was meant to be used (the idea being for it to be created at the topmost level possible, so that any number of users could register their handlers). I think that a better and more versatile API would be something like: Then we could create two sockets and have them listen on the same main loop instance. It should be possible to reimplement the existing TCPSocket::Accept function on top of this API. |
Note that currently the class TCPSocket already contains a list of NativeSocket We have 2 options:
I have implemented the option B. It was easy because the class TCPSocket already contains
It is already implemented inside TCPSocket but for different addresses. I just added different ports. The changes are minimal really. Unfortunately the diff viewer does not detect the block shifting (2 spaces right). |
I am aware of that, and I'm not entirely happy with how the class implements listening. I think it would be better to have a separate class for a listening socket, because those need a completely different APIs. But, even ignoring that, I think there's a difference between listening on two addresses with the same port (I believe the only way to reach that state is if a hostname resolves to multiple addresses, in which case one really could argue that it's the same "logical" address), and two addresses with completely unrelated ports.
We have (at least) three options, the third one being what I outlined in the previous message. I'm sorry this work of yours is going to waste. I could've been more explicit about what I wanted to do on the first PR, but I was more focused on what I don't want to do.
That's true, but I draw a different conclusion from that. Instead of saying "this should be extended to support multiple ports", my take is that "this wasn't a good place for multiplexing to begin with".
Yes, but that's because you extended a stringly typed api to do that you wanted. That function is used from other places as well, and this means that other places in lldb (including user-facing code, I believe) can accept these multi-port connection strings. I don't think we want to support that. |
|
I have updated #104238. The class Acceptor uses the MainLoop and contains the own list of listen sockets. So, this PR may be closed. |
Currently the class TCPSocket already contains a list of NativeSocket
m_listen_socketsfor different addresses.I just added different ports. So Accept() can wait for connections to different ports in a single thread.
This is the prerequisite for #104238 (see the implementation and usage of the class Acceptor).