[lldb] [Host/ConnectionFileDescriptor] Refactor to improve code reuse

[mgorny]

Summary

Refactor ConnectionFileDescriptor to improve code reuse for different
types of sockets. Unify method naming.

While at it, remove some (now-)dead code from Socket.

[mib]

This patch broke TestPlatformSDK.py on GreenDragon (https://green.lab.llvm.org/green/job/lldb-cmake/37156/)

[mib]

Looking at the test backtrace, it seems that we couldn't connect to debugserver:

File "/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/API/commands/platform/sdk/TestPlatformSDK.py", line 113, in test_macos_sdk
  self.assertTrue(connected, "could not connect to debugserver")

I checked the SBCommandReturnObject.GetError output, which says Failed to connect port. The only occurence for this error is in :

lldb/source/Host/common/TCPSocket.cpp
183:  error.SetErrorString("error: Failed to connect port");

I reverted the patch to fix GreenDragon, I'll try to investigate more tomorrow.

[mib]

Build run #37166 (https://green.lab.llvm.org/green/job/lldb-cmake/37166/) confirms that this patch caused the failure.

lldb/include/lldb/Host/Socket.h

TcpListen(llvm::StringRef host_and_port, bool child_processes_inherit,
             int backlog = 5);
 
   static llvm::Expected<std::unique_ptr<Socket>>
   TcpConnect(llvm::StringRef host_and_port, bool child_processes_inherit);
 
   static llvm::Expected<std::unique_ptr<UDPSocket>>
   UdpConnect(llvm::StringRef host_and_port, bool child_processes_inherit);
 
-  static Status UnixDomainConnect(llvm::StringRef host_and_port,
-                                  bool child_processes_inherit,
-                                  Socket *&socket);
-  static Status UnixDomainAccept(llvm::StringRef host_and_port,
-                                 bool child_processes_inherit, Socket *&socket);
-  static Status UnixAbstractConnect(llvm::StringRef host_and_port,
-                                    bool child_processes_inherit,
-                                    Socket *&socket);
-  static Status UnixAbstractAccept(llvm::StringRef host_and_port,
-                                   bool child_processes_inherit,
-                                   Socket *&socket);
-
   int GetOption(int level, int option_name, int &option_value);
   int SetOption(int level, int option_name, int option_value);
 
   NativeSocket GetNativeSocket() const { return m_socket; }
   SocketProtocol GetSocketProtocol() const { return m_protocol; }
 
   Status Read(void *buf, size_t &num_bytes) override;
   Status Write(const void *buf, size_t &num_bytes) override;

lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h

 
 #include <atomic>
 #include <memory>
 #include <mutex>
 
 #include "lldb/lldb-forward.h"
 
 #include "lldb/Host/Pipe.h"
+#include "lldb/Host/Socket.h"
 #include "lldb/Utility/Connection.h"
 #include "lldb/Utility/IOObject.h"
 
 namespace lldb_private {
 
 class Status;
 class Socket;
 class SocketAddress;
public:
   void SetChildProcessesInherit(bool child_processes_inherit);
 
 protected:
   void OpenCommandPipe();
 
   void CloseCommandPipe();
 
   lldb::ConnectionStatus
-  SocketListenAndAccept(llvm::StringRef host_and_port,
+  AcceptSocket(Socket::SocketProtocol socket_protocol,
+               llvm::StringRef socket_name,
+               llvm::function_ref<void(Socket &)> post_listen_callback,
+               Status *error_ptr);
+
+  lldb::ConnectionStatus ConnectSocket(Socket::SocketProtocol socket_protocol,
+                                       llvm::StringRef socket_name,
+                                       Status *error_ptr);
+
+  lldb::ConnectionStatus AcceptTCP(llvm::StringRef host_and_port,
                                    socket_id_callback_type socket_id_callback,
                                    Status *error_ptr);
 
   lldb::ConnectionStatus ConnectTCP(llvm::StringRef host_and_port,
                                     socket_id_callback_type socket_id_callback,
                                     Status *error_ptr);
 
   lldb::ConnectionStatus ConnectUDP(llvm::StringRef args,
                                     socket_id_callback_type socket_id_callback,
                                     Status *error_ptr);
 
   lldb::ConnectionStatus
-  NamedSocketConnect(llvm::StringRef socket_name,
+  ConnectNamedSocket(llvm::StringRef socket_name,
                      socket_id_callback_type socket_id_callback,
                      Status *error_ptr);
 
   lldb::ConnectionStatus
-  NamedSocketAccept(llvm::StringRef socket_name,
+  AcceptNamedSocket(llvm::StringRef socket_name,
                     socket_id_callback_type socket_id_callback,
                     Status *error_ptr);
 
   lldb::ConnectionStatus
-  UnixAbstractSocketAccept(llvm::StringRef socket_name,
-                           socket_id_callback_type socket_id_callback,
-                           Status *error_ptr);
+  AcceptAbstractSocket(llvm::StringRef socket_name,
 
   lldb::ConnectionStatus
-  UnixAbstractSocketConnect(llvm::StringRef socket_name,
-                            socket_id_callback_type socket_id_callback,
-                            Status *error_ptr);
+  ConnectAbstractSocket(llvm::StringRef socket_name,
 
   lldb::ConnectionStatus ConnectFD(llvm::StringRef args,
                                    socket_id_callback_type socket_id_callback,
                                    Status *error_ptr);
 
   lldb::ConnectionStatus ConnectFile(llvm::StringRef args,
                                      socket_id_callback_type socket_id_callback,
                                      Status *error_ptr);

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::UnixDomainConnect(llvm::StringRef name,
-                                 bool child_processes_inherit,
-                                 Socket *&socket) {
-  Status error;
-  std::unique_ptr<Socket> connect_socket(
-      Create(ProtocolUnixDomain, child_processes_inherit, error));
-  if (error.Fail())
-    return error;
-
-  error = connect_socket->Connect(name);
-  if (error.Success())
-    socket = connect_socket.release();
-
-  return error;
-}
-
-Status Socket::UnixDomainAccept(llvm::StringRef name,
-                                bool child_processes_inherit, Socket *&socket) {
-  Status error;
-  std::unique_ptr<Socket> listen_socket(
-      Create(ProtocolUnixDomain, child_processes_inherit, error));
-  if (error.Fail())
-    return error;
-
-  error = listen_socket->Listen(name, 5);
-  if (error.Fail())
-    return error;
-
-  error = listen_socket->Accept(socket);
-  return error;
-}
-
-Status Socket::UnixAbstractConnect(llvm::StringRef name,
-                                   bool child_processes_inherit,
-                                   Socket *&socket) {
-  Status error;
-  std::unique_ptr<Socket> connect_socket(
-      Create(ProtocolUnixAbstract, child_processes_inherit, error));
-  if (error.Fail())
-    return error;
-
-  error = connect_socket->Connect(name);
-  if (error.Success())
-    socket = connect_socket.release();
-  return error;
-}
-
-Status Socket::UnixAbstractAccept(llvm::StringRef name,
-                                  bool child_processes_inherit,
-                                  Socket *&socket) {
-  Status error;
-  std::unique_ptr<Socket> listen_socket(
-      Create(ProtocolUnixAbstract, child_processes_inherit, error));
-  if (error.Fail())
-    return error;
-
-  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) {
   static llvm::Regex g_regex("([^:]+|\\[[0-9a-fA-F:]+.*\\]):([0-9]+)");
   llvm::SmallVector<llvm::StringRef, 3> matches;
   if (g_regex.match(host_and_port, &matches)) {
     host_str = matches[1].str();
     port_str = matches[2].str();

lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

ConnectionFileDescriptor::Connect(llvm::StringRef path,
 
   llvm::StringRef scheme;
   std::tie(scheme, path) = path.split("://");
 
   if (!path.empty()) {
     auto method =
         llvm::StringSwitch<ConnectionStatus (ConnectionFileDescriptor::*)(
             llvm::StringRef, socket_id_callback_type, Status *)>(scheme)
-            .Case("listen", &ConnectionFileDescriptor::SocketListenAndAccept)
+            .Case("listen", &ConnectionFileDescriptor::AcceptTCP)
             .Cases("accept", "unix-accept",
-                   &ConnectionFileDescriptor::NamedSocketAccept)
+                   &ConnectionFileDescriptor::AcceptNamedSocket)
             .Case("unix-abstract-accept",
-                  &ConnectionFileDescriptor::UnixAbstractSocketAccept)
+                  &ConnectionFileDescriptor::AcceptAbstractSocket)
             .Cases("connect", "tcp-connect",
                    &ConnectionFileDescriptor::ConnectTCP)
             .Case("udp", &ConnectionFileDescriptor::ConnectUDP)
-            .Case("unix-connect", &ConnectionFileDescriptor::NamedSocketConnect)
+            .Case("unix-connect", &ConnectionFileDescriptor::ConnectNamedSocket)
             .Case("unix-abstract-connect",
-                  &ConnectionFileDescriptor::UnixAbstractSocketConnect)
+                  &ConnectionFileDescriptor::ConnectAbstractSocket)
 #if LLDB_ENABLE_POSIX
             .Case("fd", &ConnectionFileDescriptor::ConnectFD)
             .Case("file", &ConnectionFileDescriptor::ConnectFile)
             .Case("serial", &ConnectionFileDescriptor::ConnectSerialPort)
 #endif
             .Default(nullptr);
 
     if (method)
#endif
     }
   }
 
   if (error_ptr)
     error_ptr->SetErrorString("not connected");
   return eConnectionStatusLostConnection;
 }
 
-ConnectionStatus ConnectionFileDescriptor::NamedSocketAccept(
-    llvm::StringRef socket_name, socket_id_callback_type socket_id_callback,
+lldb::ConnectionStatus ConnectionFileDescriptor::AcceptSocket(
+    Socket::SocketProtocol socket_protocol, llvm::StringRef socket_name,
+    llvm::function_ref<void(Socket &)> post_listen_callback,
     Status *error_ptr) {
   Status error;
-  std::unique_ptr<Socket> listen_socket = Socket::Create(
-      Socket::ProtocolUnixDomain, m_child_processes_inherit, error);
-  Socket *socket = nullptr;
+  std::unique_ptr<Socket> listening_socket =
+      Socket::Create(socket_protocol, m_child_processes_inherit, error);
+  Socket *accepted_socket;
 
   if (!error.Fail())
-    error = listen_socket->Listen(socket_name, 5);
+    error = listening_socket->Listen(socket_name, 5);
 
   if (!error.Fail()) {
-    socket_id_callback(socket_name);
-    error = listen_socket->Accept(socket);
+    post_listen_callback(*listening_socket);
+    error = listening_socket->Accept(accepted_socket);
   }
 
   if (!error.Fail()) {
-    m_io_sp.reset(socket);
+    m_io_sp.reset(accepted_socket);
     m_uri.assign(socket_name.str());
     return eConnectionStatusSuccess;
   }
 
   if (error_ptr)
     *error_ptr = error;
   return eConnectionStatusError;
 }
 
-ConnectionStatus ConnectionFileDescriptor::NamedSocketConnect(
-    llvm::StringRef socket_name, socket_id_callback_type socket_id_callback,
-    Status *error_ptr) {
-  Socket *socket = nullptr;
-  Status error =
-      Socket::UnixDomainConnect(socket_name, m_child_processes_inherit, socket);
-  if (error_ptr)
-    *error_ptr = error;
-  m_io_sp.reset(socket);
-  if (error.Fail())
-    return eConnectionStatusError;
-  m_uri.assign(std::string(socket_name));
-  return eConnectionStatusSuccess;
-}
-
-ConnectionStatus ConnectionFileDescriptor::UnixAbstractSocketAccept(
-    llvm::StringRef socket_name, socket_id_callback_type socket_id_callback,
+lldb::ConnectionStatus
+ConnectionFileDescriptor::ConnectSocket(Socket::SocketProtocol socket_protocol,
+                                        llvm::StringRef socket_name,
                                         Status *error_ptr) {
   Status error;
-  std::unique_ptr<Socket> listen_socket = Socket::Create(
-      Socket::ProtocolUnixAbstract, m_child_processes_inherit, error);
-  Socket *socket = nullptr;
-
-  if (!error.Fail())
-    error = listen_socket->Listen(socket_name, 5);
+  std::unique_ptr<Socket> socket =
+      Socket::Create(socket_protocol, m_child_processes_inherit, error);
 
   if (!error.Fail())
-    socket_id_callback(socket_name);
-
-  if (!error.Fail())
-    error = listen_socket->Accept(socket);
+    error = socket->Connect(socket_name);
 
   if (!error.Fail()) {
-    m_io_sp.reset(socket);
+    m_io_sp = std::move(socket);
     m_uri.assign(socket_name.str());
     return eConnectionStatusSuccess;
   }
 
   if (error_ptr)
     *error_ptr = error;
   return eConnectionStatusError;
 }
 
-lldb::ConnectionStatus ConnectionFileDescriptor::UnixAbstractSocketConnect(
+ConnectionStatus ConnectionFileDescriptor::AcceptNamedSocket(
     llvm::StringRef socket_name, socket_id_callback_type socket_id_callback,
     Status *error_ptr) {
-  Socket *socket = nullptr;
-  Status error = Socket::UnixAbstractConnect(socket_name,
-                                             m_child_processes_inherit, socket);
-  if (error_ptr)
-    *error_ptr = error;
-  m_io_sp.reset(socket);
-  if (error.Fail())
-    return eConnectionStatusError;
-  m_uri.assign(std::string(socket_name));
-  return eConnectionStatusSuccess;
+  return AcceptSocket(
+      Socket::ProtocolUnixDomain, socket_name,
+      [socket_id_callback, socket_name](Socket &listening_socket) {
+        socket_id_callback(socket_name);
+      },
+      error_ptr);
 }
 
-ConnectionStatus ConnectionFileDescriptor::SocketListenAndAccept(
-    llvm::StringRef s, socket_id_callback_type socket_id_callback,
+ConnectionStatus ConnectionFileDescriptor::ConnectNamedSocket(
+    llvm::StringRef socket_name, socket_id_callback_type socket_id_callback,
     Status *error_ptr) {
-  if (error_ptr)
-    *error_ptr = Status();
-
-  llvm::Expected<std::unique_ptr<TCPSocket>> listening_socket =
-      Socket::TcpListen(s, m_child_processes_inherit);
-  if (!listening_socket) {
-    if (error_ptr)
-      *error_ptr = listening_socket.takeError();
-    else
-      LLDB_LOG_ERROR(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION),
-                     listening_socket.takeError(), "tcp listen failed: {0}");
-    return eConnectionStatusError;
-  }
+  return ConnectSocket(Socket::ProtocolUnixDomain, socket_name, error_ptr);
 
-  uint16_t port = listening_socket.get()->GetLocalPortNumber();
-  socket_id_callback(std::to_string(port));
-
-  Socket *accepted_socket;
-  Status error = listening_socket.get()->Accept(accepted_socket);
-  if (error_ptr)
-    *error_ptr = error;
-  if (error.Fail())
-    return eConnectionStatusError;
+ConnectionStatus ConnectionFileDescriptor::AcceptAbstractSocket(
+    llvm::StringRef socket_name, socket_id_callback_type socket_id_callback,
+    Status *error_ptr) {
+  return AcceptSocket(
+      Socket::ProtocolUnixAbstract, socket_name,
+      [socket_id_callback, socket_name](Socket &listening_socket) {
+        socket_id_callback(socket_name);
+      },
+      error_ptr);
+}
 
-  InitializeSocket(accepted_socket);
-  return eConnectionStatusSuccess;
+lldb::ConnectionStatus ConnectionFileDescriptor::ConnectAbstractSocket(
+    llvm::StringRef socket_name, socket_id_callback_type socket_id_callback,
+    Status *error_ptr) {
+  return ConnectSocket(Socket::ProtocolUnixAbstract, socket_name, error_ptr);
 }
 
 ConnectionStatus
-ConnectionFileDescriptor::ConnectTCP(llvm::StringRef s,
-                                     socket_id_callback_type socket_id_callback,
-                                     Status *error_ptr) {
-  if (error_ptr)
-    *error_ptr = Status();
-
-  llvm::Expected<std::unique_ptr<Socket>> socket =
-      Socket::TcpConnect(s, m_child_processes_inherit);
-  if (!socket) {
-    if (error_ptr)
-      *error_ptr = socket.takeError();
-    else
-      LLDB_LOG_ERROR(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION),
-                     socket.takeError(), "tcp connect failed: {0}");
-    return eConnectionStatusError;
-  }
-  m_io_sp = std::move(*socket);
-  m_uri.assign(std::string(s));
-  return eConnectionStatusSuccess;
+ConnectionFileDescriptor::AcceptTCP(llvm::StringRef socket_name,
+  ConnectionStatus ret = AcceptSocket(
+      Socket::ProtocolTcp, socket_name,
+      [socket_id_callback](Socket &listening_socket) {
+        uint16_t port =
+            static_cast<TCPSocket &>(listening_socket).GetLocalPortNumber();
+        socket_id_callback(std::to_string(port));
+      },
+      error_ptr);
+  if (ret == eConnectionStatusSuccess)
+    m_uri.assign(
+        static_cast<TCPSocket *>(m_io_sp.get())->GetRemoteConnectionURI());
+  return ret;
+
+ConnectionStatus
+ConnectionFileDescriptor::ConnectTCP(llvm::StringRef socket_name,
+                                     socket_id_callback_type socket_id_callback,
+                                     Status *error_ptr) {
+  return ConnectSocket(Socket::ProtocolTcp, socket_name, error_ptr);
 }
 
 ConnectionStatus
 ConnectionFileDescriptor::ConnectUDP(llvm::StringRef s,
                                      socket_id_callback_type socket_id_callback,
                                      Status *error_ptr) {
   if (error_ptr)
     *error_ptr = Status();