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

[mgorny]

Fix missing return in operator<<.

[labath]

lldb/source/Host/common/Socket.cpp

262–264

I guess this isn't true anymore.

lldb/include/lldb/Host/Socket.h

 public:
   enum SocketProtocol {
     ProtocolTcp,
     ProtocolUdp,
     ProtocolUnixDomain,
     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;
 
   static llvm::Error Initialize();
   static void Terminate();
 
   static std::unique_ptr<Socket> Create(const SocketProtocol protocol,
public:
   Status Write(const void *buf, size_t &num_bytes) override;
 
   virtual Status PreDisconnect();
   Status Close() override;
 
   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 ""; };
 
 protected:
   Socket(SocketProtocol protocol, bool should_close,
          bool m_child_process_inherit);
 
   virtual size_t Send(const void *buf, const size_t num_bytes);
 
   static void SetLastError(Status &error);
   static NativeSocket CreateSocket(const int domain, const int type,
                                    const int protocol,
                                    bool child_processes_inherit, Status &error);
   static NativeSocket AcceptSocket(NativeSocket sockfd, struct sockaddr *addr,
                                    socklen_t *addrlen,
                                    bool child_processes_inherit, Status &error);
 
   SocketProtocol m_protocol;
   NativeSocket m_socket;
   bool m_child_processes_inherit;
   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

 }
 
 llvm::Expected<std::unique_ptr<TCPSocket>>
 Socket::TcpListen(llvm::StringRef host_and_port, bool child_processes_inherit,
                   int backlog) {
   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));
 
   Status error = listen_socket->Listen(host_and_port, backlog);
   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);
 }
 
 llvm::Expected<std::unique_ptr<UDPSocket>>
 Socket::UdpConnect(llvm::StringRef host_and_port,
                    bool child_processes_inherit) {
   return UDPSocket::Connect(host_and_port, child_processes_inherit);
 }
Status Socket::UnixAbstractAccept(llvm::StringRef name,
   error = listen_socket->Listen(name, 5);
   if (error.Fail())
     return error;
 
   error = listen_socket->Accept(socket);
   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(),
                                  "invalid host:port specification: '%s'",
                                  host_and_port.str().c_str());
 }
 
 IOObject::WaitableHandle Socket::GetWaitableHandle() {

 #else
   NativeSocket fd = llvm::sys::RetryAfterSignal(
       static_cast<NativeSocket>(-1), ::accept, sockfd, addr, addrlen);
 #endif
   if (fd == kInvalidSocketValue)
     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

 }
 
 Status TCPSocket::Connect(llvm::StringRef name) {
 
   Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_COMMUNICATION));
   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));
-
-  std::vector<SocketAddress> addresses = SocketAddress::GetAddressInfo(
-      host_str.c_str(), nullptr, AF_UNSPEC, SOCK_STREAM, IPPROTO_TCP);
+  llvm::Expected<HostAndPort> host_port = DecodeHostAndPort(name);
+  if (!host_port)
+    return Status(host_port.takeError());
+
+  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;
     }
 
     SetOptionNoDelay();
 
     error.Clear();
     return error;
   }
 
   error.SetErrorString("Failed to connect port");
   return error;
 }
 
 Status TCPSocket::Listen(llvm::StringRef name, int backlog) {
   Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION));
   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);
     if (error.Fail())
       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);
     ::setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, option_value_p,
                  sizeof(option_value));
 
     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());
     if (-1 != err)
       err = ::listen(fd, backlog);
 
     if (-1 == err) {
       error = GetLastSocketError();
       CLOSE_SOCKET(fd);
       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;
   }
 
   if (m_listen_sockets.empty()) {
     assert(error.Fail());
     return error;
   }

lldb/source/Host/common/UDPSocket.cpp

 llvm::Expected<std::unique_ptr<UDPSocket>>
 UDPSocket::Connect(llvm::StringRef name, bool child_processes_inherit) {
   std::unique_ptr<UDPSocket> socket;
 
   Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION));
   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
 
   struct addrinfo hints;
   struct addrinfo *service_info_list = nullptr;
 
   ::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();
   }
 
   for (struct addrinfo *service_info_ptr = service_info_list;
        service_info_ptr != nullptr;
        service_info_ptr = service_info_ptr->ai_next) {
     auto send_fd = CreateSocket(
         service_info_ptr->ai_family, service_info_ptr->ai_socktype,
#endif
 
   if (!socket)
     return error.ToError();
 
   SocketAddress bind_addr;
 
   // 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");
     return error.ToError();
   }
 
   bind_addr.SetPort(0); // Let the source port # be determined dynamically
 

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

lldb_private::process_gdb_remote::LLGSArgToURL(llvm::StringRef url_arg,
   }
 
   std::string host_port = url_arg.str();
   // If host_and_port starts with ':', default the host to be "localhost" and
   // expect the remainder to be the port.
   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.
   return (reverse_connect ? "unix-connect://" : "unix-accept://") +
          url_arg.str();
 }

lldb/tools/lldb-server/Acceptor.cpp

std::unique_ptr<Acceptor> Acceptor::Create(StringRef name,
   // Try to match socket name as URL - e.g., tcp://localhost:5555
   if (llvm::Optional<URI> res = URI::Parse(name)) {
     if (!FindProtocolByScheme(res->scheme.str().c_str(), socket_protocol))
       error.SetErrorStringWithFormat("Unknown protocol scheme \"%s\"",
                                      res->scheme.str().c_str());
     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;
   }
 
   if (error.Fail())
     return std::unique_ptr<Acceptor>();
 
   std::unique_ptr<Socket> listener_socket_up =
       Socket::Create(socket_protocol, child_processes_inherit, error);

lldb/unittests/Host/SocketTest.cpp

protected:
   bool HostSupportsProtocol() const {
     if (GetParam().is_ipv6)
       return HostSupportsIPv6();
     return HostSupportsIPv4();
   }
 };
 
 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_EXPECTED(Socket::DecodeHostAndPort("localhost:1138"),
+                       llvm::HasValue(Socket::HostAndPort{"localhost", 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("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
 TEST_P(SocketTest, DomainListenConnectAccept) {
   llvm::SmallString<64> Path;
   std::error_code EC = llvm::sys::fs::createUniqueDirectory("DomainListenConnectAccept", Path);
   ASSERT_FALSE(EC);
   llvm::sys::path::append(Path, "test");