[lldb] [Host/Socket] Make DecodeHostAndPort() return a dedicated struct

Differential Revision: https://reviews.llvm.org/D112629

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<HostAndPort>
+  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

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<TCPSocket> 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::HostAndPort> Socket::DecodeHostAndPort(llvm::StringRef host_and_port) {
   static llvm::Regex g_regex("([^:]+|\\[[0-9a-fA-F:]+.*\\]):([0-9]+)");
+  HostAndPort ret;
   llvm::SmallVector<llvm::StringRef, 3> 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;
+}

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<HostAndPort> host_port = DecodeHostAndPort(name);
+  if (!host_port)
+    return Status(host_port.takeError());
 
-  std::vector<SocketAddress> addresses = SocketAddress::GetAddressInfo(
-      host_str.c_str(), nullptr, AF_UNSPEC, SOCK_STREAM, IPPROTO_TCP);
+  std::vector<SocketAddress> 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));
+  llvm::Expected<HostAndPort> host_port = DecodeHostAndPort(name);
+  if (!host_port)
+    return Status(host_port.takeError());
 
-  if (host_str == "*")
-    host_str = "0.0.0.0";
+  if (host_port->hostname == "*")
+    host_port->hostname = "0.0.0.0";
   std::vector<SocketAddress> 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;
   }

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<HostAndPort> 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");

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.

lldb/tools/lldb-server/Acceptor.cpp

@@ -92,12 +92,8 @@ std::unique_ptr<Acceptor> 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;
   }
 

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_EXPECTED(Socket::DecodeHostAndPort("localhost:1138"),
+                       llvm::HasValue(Socket::HostAndPort{"localhost", 1138}));
 
-  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("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_EXPECTED(Socket::DecodeHostAndPort("12345"),
+                       llvm::HasValue(Socket::HostAndPort{"", 12345}));
 
-  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_EXPECTED(Socket::DecodeHostAndPort("*:0"),
+                       llvm::HasValue(Socket::HostAndPort{"*", 0}));
 
-  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_EXPECTED(Socket::DecodeHostAndPort("*:65535"),
+                       llvm::HasValue(Socket::HostAndPort{"*", 65535}));
 
-  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_EXPECTED(
+      Socket::DecodeHostAndPort("[::1]:12345"),
+      llvm::HasValue(Socket::HostAndPort{"::1", 12345}));
 
-  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("[abcd:12fg:AF58::1]:12345"),
+      llvm::HasValue(Socket::HostAndPort{"abcd:12fg:AF58::1", 12345}));
 }
 
 #if LLDB_ENABLE_POSIX