From 073c5d0e4706f14b599f62c7bb115b843d0e4962 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Micha=C5=82=20G=C3=B3rny?= Date: Wed, 27 Oct 2021 17:52:45 +0200 Subject: [PATCH] [lldb] [Host/Socket] Make DecodeHostAndPort() return a dedicated struct Differential Revision: https://reviews.llvm.org/D112629 --- lldb/include/lldb/Host/Socket.h | 17 ++++- lldb/source/Host/common/Socket.cpp | 44 +++++-------- lldb/source/Host/common/TCPSocket.cpp | 46 ++++++------- lldb/source/Host/common/UDPSocket.cpp | 23 +++---- .../GDBRemoteCommunicationServerLLGS.cpp | 6 +- lldb/tools/lldb-server/Acceptor.cpp | 6 +- lldb/unittests/Host/SocketTest.cpp | 75 ++++++++-------------- 7 files changed, 87 insertions(+), 130 deletions(-) diff --git a/lldb/include/lldb/Host/Socket.h b/lldb/include/lldb/Host/Socket.h index 780c6e6..9176433 100644 --- a/lldb/include/lldb/Host/Socket.h +++ b/lldb/include/lldb/Host/Socket.h @@ -47,6 +47,15 @@ public: ProtocolUnixAbstract }; + struct HostAndPort { + std::string hostname; + uint16_t port; + + bool operator==(const HostAndPort &R) const { + return port == R.port && hostname == R.hostname; + } + }; + static const NativeSocket kInvalidSocketValue; ~Socket() override; @@ -102,9 +111,8 @@ public: bool IsValid() const override { return m_socket != kInvalidSocketValue; } WaitableHandle GetWaitableHandle() override; - static llvm::Error DecodeHostAndPort(llvm::StringRef host_and_port, - std::string &host_str, - std::string &port_str, uint16_t &port); + static llvm::Expected + DecodeHostAndPort(llvm::StringRef host_and_port); // If this Socket is connected then return the URI used to connect. virtual std::string GetRemoteConnectionURI() const { return ""; }; @@ -129,6 +137,9 @@ protected: bool m_should_close_fd; }; +llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, + const Socket::HostAndPort &HP); + } // namespace lldb_private #endif // LLDB_HOST_SOCKET_H diff --git a/lldb/source/Host/common/Socket.cpp b/lldb/source/Host/common/Socket.cpp index 1eaa695..b3fdb93 100644 --- a/lldb/source/Host/common/Socket.cpp +++ b/lldb/source/Host/common/Socket.cpp @@ -167,13 +167,6 @@ Socket::TcpListen(llvm::StringRef host_and_port, bool child_processes_inherit, Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION)); LLDB_LOG(log, "host_and_port = {0}", host_and_port); - std::string host_str; - std::string port_str; - uint16_t port; - if (llvm::Error decode_error = - DecodeHostAndPort(host_and_port, host_str, port_str, port)) - return std::move(decode_error); - std::unique_ptr listen_socket( new TCPSocket(true, child_processes_inherit)); @@ -181,12 +174,6 @@ Socket::TcpListen(llvm::StringRef host_and_port, bool child_processes_inherit, if (error.Fail()) return error.ToError(); - // We were asked to listen on port zero which means we must now read the - // actual port that was given to us as port zero is a special code for - // "find an open port for me". - if (port == 0) - port = listen_socket->GetLocalPortNumber(); - return std::move(listen_socket); } @@ -260,28 +247,22 @@ Status Socket::UnixAbstractAccept(llvm::StringRef name, return error; } -llvm::Error Socket::DecodeHostAndPort(llvm::StringRef host_and_port, - std::string &host_str, - std::string &port_str, uint16_t &port) { +llvm::Expected Socket::DecodeHostAndPort(llvm::StringRef host_and_port) { static llvm::Regex g_regex("([^:]+|\\[[0-9a-fA-F:]+.*\\]):([0-9]+)"); + HostAndPort ret; llvm::SmallVector matches; if (g_regex.match(host_and_port, &matches)) { - host_str = matches[1].str(); - port_str = matches[2].str(); + ret.hostname = matches[1].str(); // IPv6 addresses are wrapped in [] when specified with ports - if (host_str.front() == '[' && host_str.back() == ']') - host_str = host_str.substr(1, host_str.size() - 2); - if (to_integer(matches[2], port, 10)) - return llvm::Error::success(); + if (ret.hostname.front() == '[' && ret.hostname.back() == ']') + ret.hostname = ret.hostname.substr(1, ret.hostname.size() - 2); + if (to_integer(matches[2], ret.port, 10)) + return ret; } else { - // If this was unsuccessful, then check if it's simply a signed 32-bit + // If this was unsuccessful, then check if it's simply an unsigned 16-bit // integer, representing a port with an empty host. - host_str.clear(); - port_str.clear(); - if (to_integer(host_and_port, port, 10)) { - port_str = host_and_port.str(); - return llvm::Error::success(); - } + if (to_integer(host_and_port, ret.port, 10)) + return ret; } return llvm::createStringError(llvm::inconvertibleErrorCode(), @@ -455,3 +436,8 @@ NativeSocket Socket::AcceptSocket(NativeSocket sockfd, struct sockaddr *addr, SetLastError(error); return fd; } + +llvm::raw_ostream &lldb_private::operator<<(llvm::raw_ostream &OS, + const Socket::HostAndPort &HP) { + return OS << '[' << HP.hostname << ']' << ':' << HP.port; +} diff --git a/lldb/source/Host/common/TCPSocket.cpp b/lldb/source/Host/common/TCPSocket.cpp index 4e5bd69..28c3fa1 100644 --- a/lldb/source/Host/common/TCPSocket.cpp +++ b/lldb/source/Host/common/TCPSocket.cpp @@ -152,24 +152,23 @@ Status TCPSocket::Connect(llvm::StringRef name) { LLDB_LOGF(log, "TCPSocket::%s (host/port = %s)", __FUNCTION__, name.data()); Status error; - std::string host_str; - std::string port_str; - uint16_t port; - if (llvm::Error decode_error = - DecodeHostAndPort(name, host_str, port_str, port)) - return Status(std::move(decode_error)); + llvm::Expected host_port = DecodeHostAndPort(name); + if (!host_port) + return Status(host_port.takeError()); - std::vector addresses = SocketAddress::GetAddressInfo( - host_str.c_str(), nullptr, AF_UNSPEC, SOCK_STREAM, IPPROTO_TCP); + std::vector addresses = + SocketAddress::GetAddressInfo(host_port->hostname.c_str(), nullptr, + AF_UNSPEC, SOCK_STREAM, IPPROTO_TCP); for (SocketAddress &address : addresses) { error = CreateSocket(address.GetFamily()); if (error.Fail()) continue; - address.SetPort(port); + address.SetPort(host_port->port); - if (-1 == llvm::sys::RetryAfterSignal(-1, ::connect, - GetNativeSocket(), &address.sockaddr(), address.GetLength())) { + if (-1 == llvm::sys::RetryAfterSignal(-1, ::connect, GetNativeSocket(), + &address.sockaddr(), + address.GetLength())) { CLOSE_SOCKET(GetNativeSocket()); continue; } @@ -189,17 +188,14 @@ Status TCPSocket::Listen(llvm::StringRef name, int backlog) { LLDB_LOGF(log, "TCPSocket::%s (%s)", __FUNCTION__, name.data()); Status error; - std::string host_str; - std::string port_str; - uint16_t port; - if (llvm::Error decode_error = - DecodeHostAndPort(name, host_str, port_str, port)) - return Status(std::move(decode_error)); - - if (host_str == "*") - host_str = "0.0.0.0"; + llvm::Expected host_port = DecodeHostAndPort(name); + if (!host_port) + return Status(host_port.takeError()); + + if (host_port->hostname == "*") + host_port->hostname = "0.0.0.0"; std::vector addresses = SocketAddress::GetAddressInfo( - host_str.c_str(), nullptr, AF_UNSPEC, SOCK_STREAM, IPPROTO_TCP); + 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); @@ -215,9 +211,9 @@ Status TCPSocket::Listen(llvm::StringRef name, int backlog) { SocketAddress listen_address = address; if(!listen_address.IsLocalhost()) - listen_address.SetToAnyAddress(address.GetFamily(), port); + listen_address.SetToAnyAddress(address.GetFamily(), host_port->port); else - listen_address.SetPort(port); + listen_address.SetPort(host_port->port); int err = ::bind(fd, &listen_address.sockaddr(), listen_address.GetLength()); @@ -230,10 +226,10 @@ Status TCPSocket::Listen(llvm::StringRef name, int backlog) { continue; } - if (port == 0) { + if (host_port->port == 0) { socklen_t sa_len = address.GetLength(); if (getsockname(fd, &address.sockaddr(), &sa_len) == 0) - port = address.GetPort(); + host_port->port = address.GetPort(); } m_listen_sockets[fd] = address; } diff --git a/lldb/source/Host/common/UDPSocket.cpp b/lldb/source/Host/common/UDPSocket.cpp index ceab6f7..31266c9 100644 --- a/lldb/source/Host/common/UDPSocket.cpp +++ b/lldb/source/Host/common/UDPSocket.cpp @@ -58,12 +58,9 @@ UDPSocket::Connect(llvm::StringRef name, bool child_processes_inherit) { LLDB_LOG(log, "host/port = {0}", name); Status error; - std::string host_str; - std::string port_str; - uint16_t port; - if (llvm::Error decode_error = - DecodeHostAndPort(name, host_str, port_str, port)) - return std::move(decode_error); + llvm::Expected host_port = DecodeHostAndPort(name); + if (!host_port) + return host_port.takeError(); // At this point we have setup the receive port, now we need to setup the UDP // send socket @@ -74,16 +71,16 @@ UDPSocket::Connect(llvm::StringRef name, bool child_processes_inherit) { ::memset(&hints, 0, sizeof(hints)); hints.ai_family = kDomain; hints.ai_socktype = kType; - int err = ::getaddrinfo(host_str.c_str(), port_str.c_str(), &hints, + int err = ::getaddrinfo(host_port->hostname.c_str(), std::to_string(host_port->port).c_str(), &hints, &service_info_list); if (err != 0) { error.SetErrorStringWithFormat( #if defined(_WIN32) && defined(UNICODE) - "getaddrinfo(%s, %s, &hints, &info) returned error %i (%S)", + "getaddrinfo(%s, %d, &hints, &info) returned error %i (%S)", #else - "getaddrinfo(%s, %s, &hints, &info) returned error %i (%s)", + "getaddrinfo(%s, %d, &hints, &info) returned error %i (%s)", #endif - host_str.c_str(), port_str.c_str(), err, gai_strerror(err)); + host_port->hostname.c_str(), host_port->port, err, gai_strerror(err)); return error.ToError(); } @@ -110,9 +107,9 @@ UDPSocket::Connect(llvm::StringRef name, bool child_processes_inherit) { // Only bind to the loopback address if we are expecting a connection from // localhost to avoid any firewall issues. - const bool bind_addr_success = (host_str == "127.0.0.1" || host_str == "localhost") - ? bind_addr.SetToLocalhost(kDomain, port) - : bind_addr.SetToAnyAddress(kDomain, port); + const bool bind_addr_success = (host_port->hostname == "127.0.0.1" || host_port->hostname == "localhost") + ? bind_addr.SetToLocalhost(kDomain, host_port->port) + : bind_addr.SetToAnyAddress(kDomain, host_port->port); if (!bind_addr_success) { error.SetErrorString("Failed to get hostspec to bind for"); diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp index 27acf7e..da64531 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp @@ -3917,12 +3917,8 @@ lldb_private::process_gdb_remote::LLGSArgToURL(llvm::StringRef url_arg, if (url_arg.startswith(":")) host_port.insert(0, "localhost"); - std::string host_str; - std::string port_str; - uint16_t port; // Try parsing the (preprocessed) argument as host:port pair. - if (!llvm::errorToBool( - Socket::DecodeHostAndPort(host_port, host_str, port_str, port))) + if (!llvm::errorToBool(Socket::DecodeHostAndPort(host_port).takeError())) return (reverse_connect ? "connect://" : "listen://") + host_port; // If none of the above applied, interpret the argument as UNIX socket path. diff --git a/lldb/tools/lldb-server/Acceptor.cpp b/lldb/tools/lldb-server/Acceptor.cpp index a031a32..4714252 100644 --- a/lldb/tools/lldb-server/Acceptor.cpp +++ b/lldb/tools/lldb-server/Acceptor.cpp @@ -92,12 +92,8 @@ std::unique_ptr Acceptor::Create(StringRef name, else name = name.drop_front(res->scheme.size() + strlen("://")); } else { - std::string host_str; - std::string port_str; - uint16_t port; // Try to match socket name as $host:port - e.g., localhost:5555 - if (!llvm::errorToBool( - Socket::DecodeHostAndPort(name, host_str, port_str, port))) + if (!llvm::errorToBool(Socket::DecodeHostAndPort(name).takeError())) socket_protocol = Socket::ProtocolTcp; } diff --git a/lldb/unittests/Host/SocketTest.cpp b/lldb/unittests/Host/SocketTest.cpp index 15ccf64..6af0b3c 100644 --- a/lldb/unittests/Host/SocketTest.cpp +++ b/lldb/unittests/Host/SocketTest.cpp @@ -33,65 +33,40 @@ protected: }; TEST_P(SocketTest, DecodeHostAndPort) { - std::string host_str; - std::string port_str; - uint16_t port; - - EXPECT_THAT_ERROR( - Socket::DecodeHostAndPort("localhost:1138", host_str, port_str, port), - llvm::Succeeded()); - EXPECT_STREQ("localhost", host_str.c_str()); - EXPECT_STREQ("1138", port_str.c_str()); - EXPECT_EQ(1138, port); - - EXPECT_THAT_ERROR( - Socket::DecodeHostAndPort("google.com:65536", host_str, port_str, port), + EXPECT_THAT_EXPECTED(Socket::DecodeHostAndPort("localhost:1138"), + llvm::HasValue(Socket::HostAndPort{"localhost", 1138})); + + EXPECT_THAT_EXPECTED( + Socket::DecodeHostAndPort("google.com:65536"), llvm::FailedWithMessage( "invalid host:port specification: 'google.com:65536'")); - EXPECT_THAT_ERROR( - Socket::DecodeHostAndPort("google.com:-1138", host_str, port_str, port), + EXPECT_THAT_EXPECTED( + Socket::DecodeHostAndPort("google.com:-1138"), llvm::FailedWithMessage( "invalid host:port specification: 'google.com:-1138'")); - EXPECT_THAT_ERROR( - Socket::DecodeHostAndPort("google.com:65536", host_str, port_str, port), + EXPECT_THAT_EXPECTED( + Socket::DecodeHostAndPort("google.com:65536"), llvm::FailedWithMessage( "invalid host:port specification: 'google.com:65536'")); - EXPECT_THAT_ERROR( - Socket::DecodeHostAndPort("12345", host_str, port_str, port), - llvm::Succeeded()); - EXPECT_STREQ("", host_str.c_str()); - EXPECT_STREQ("12345", port_str.c_str()); - EXPECT_EQ(12345, port); - - EXPECT_THAT_ERROR(Socket::DecodeHostAndPort("*:0", host_str, port_str, port), - llvm::Succeeded()); - EXPECT_STREQ("*", host_str.c_str()); - EXPECT_STREQ("0", port_str.c_str()); - EXPECT_EQ(0, port); - - EXPECT_THAT_ERROR( - Socket::DecodeHostAndPort("*:65535", host_str, port_str, port), - llvm::Succeeded()); - EXPECT_STREQ("*", host_str.c_str()); - EXPECT_STREQ("65535", port_str.c_str()); - EXPECT_EQ(65535, port); - - EXPECT_THAT_ERROR( - Socket::DecodeHostAndPort("[::1]:12345", host_str, port_str, port), - llvm::Succeeded()); - EXPECT_STREQ("::1", host_str.c_str()); - EXPECT_STREQ("12345", port_str.c_str()); - EXPECT_EQ(12345, port); - - EXPECT_THAT_ERROR(Socket::DecodeHostAndPort("[abcd:12fg:AF58::1]:12345", - host_str, port_str, port), - llvm::Succeeded()); - EXPECT_STREQ("abcd:12fg:AF58::1", host_str.c_str()); - EXPECT_STREQ("12345", port_str.c_str()); - EXPECT_EQ(12345, port); + EXPECT_THAT_EXPECTED(Socket::DecodeHostAndPort("12345"), + llvm::HasValue(Socket::HostAndPort{"", 12345})); + + EXPECT_THAT_EXPECTED(Socket::DecodeHostAndPort("*:0"), + llvm::HasValue(Socket::HostAndPort{"*", 0})); + + EXPECT_THAT_EXPECTED(Socket::DecodeHostAndPort("*:65535"), + llvm::HasValue(Socket::HostAndPort{"*", 65535})); + + EXPECT_THAT_EXPECTED( + Socket::DecodeHostAndPort("[::1]:12345"), + llvm::HasValue(Socket::HostAndPort{"::1", 12345})); + + EXPECT_THAT_EXPECTED( + Socket::DecodeHostAndPort("[abcd:12fg:AF58::1]:12345"), + llvm::HasValue(Socket::HostAndPort{"abcd:12fg:AF58::1", 12345})); } #if LLDB_ENABLE_POSIX -- 2.7.4