[lldb] [ConnectionFileDescriptorPosix] Refactor scheme matching

[mgorny]

Summary

Move the POSIX-specific fd:// and file:// scheme handling into
separate methods. Replace the custom GetURLAddress() matching with
splitting into scheme and path, and matching scheme via
llvm::StringSwitch.

[mgorny]

Rebase to match the original termios code without other of my patches.

[labath]

thanks

lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

140–141

Maybe just drop std::function?

161–171

Could you also please convert this to early returns, while you're inside. I spent 10 minutes looking at this, and I'm still not sure whether the error strings are correctly related to the individual checks.

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

 namespace lldb_private {
 
 class Status;
 class Socket;
 class SocketAddress;
 
 class ConnectionFileDescriptor : public Connection {
 public:
-  static const char *LISTEN_SCHEME;
-  static const char *ACCEPT_SCHEME;
-  static const char *UNIX_ACCEPT_SCHEME;
-  static const char *CONNECT_SCHEME;
-  static const char *TCP_CONNECT_SCHEME;
-  static const char *UDP_SCHEME;
-  static const char *UNIX_CONNECT_SCHEME;
-  static const char *UNIX_ABSTRACT_CONNECT_SCHEME;
-  static const char *FD_SCHEME;
-  static const char *FILE_SCHEME;
-
   ConnectionFileDescriptor(bool child_processes_inherit = false);
 
   ConnectionFileDescriptor(int fd, bool owns_fd);
 
   ConnectionFileDescriptor(Socket *socket);
 
   ~ConnectionFileDescriptor() override;
 
lldb::ConnectionStatus NamedSocketConnect(llvm::StringRef socket_name,
                                             Status *error_ptr);
 
   lldb::ConnectionStatus NamedSocketAccept(llvm::StringRef socket_name,
                                            Status *error_ptr);
 
   lldb::ConnectionStatus UnixAbstractSocketConnect(llvm::StringRef socket_name,
                                                    Status *error_ptr);
 
+  lldb::ConnectionStatus ConnectFD(llvm::StringRef args, Status *error_ptr);
+
+  lldb::ConnectionStatus ConnectFile(llvm::StringRef args, Status *error_ptr);
+
   lldb::IOObjectSP m_read_sp;
   lldb::IOObjectSP m_write_sp;
 
   Predicate<uint16_t>
       m_port_predicate; // Used when binding to port zero to wait for the thread
                         // that creates the socket, binds and listens to
                         // resolve the port number.
 

lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

 #include "lldb/Host/common/UDPSocket.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/Timer.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
-const char *ConnectionFileDescriptor::LISTEN_SCHEME = "listen";
-const char *ConnectionFileDescriptor::ACCEPT_SCHEME = "accept";
-const char *ConnectionFileDescriptor::UNIX_ACCEPT_SCHEME = "unix-accept";
-const char *ConnectionFileDescriptor::CONNECT_SCHEME = "connect";
-const char *ConnectionFileDescriptor::TCP_CONNECT_SCHEME = "tcp-connect";
-const char *ConnectionFileDescriptor::UDP_SCHEME = "udp";
-const char *ConnectionFileDescriptor::UNIX_CONNECT_SCHEME = "unix-connect";
-const char *ConnectionFileDescriptor::UNIX_ABSTRACT_CONNECT_SCHEME =
-    "unix-abstract-connect";
-const char *ConnectionFileDescriptor::FD_SCHEME = "fd";
-const char *ConnectionFileDescriptor::FILE_SCHEME = "file";
-
-static llvm::Optional<llvm::StringRef> GetURLAddress(llvm::StringRef url,
-                                                     llvm::StringRef scheme) {
-  if (!url.consume_front(scheme))
-    return llvm::None;
-  if (!url.consume_front("://"))
-    return llvm::None;
-  return url;
-}
-
 ConnectionFileDescriptor::ConnectionFileDescriptor(bool child_processes_inherit)
     : Connection(), m_pipe(), m_mutex(), m_shutting_down(false),
 
       m_child_processes_inherit(child_processes_inherit) {
   Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION |
                                                   LIBLLDB_LOG_OBJECT));
   LLDB_LOGF(log, "%p ConnectionFileDescriptor::ConnectionFileDescriptor ()",
             static_cast<void *>(this));
ConnectionStatus ConnectionFileDescriptor::Connect(llvm::StringRef path,
                                                    Status *error_ptr) {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
   Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION));
   LLDB_LOGF(log, "%p ConnectionFileDescriptor::Connect (url = '%s')",
             static_cast<void *>(this), path.str().c_str());
 
   OpenCommandPipe();
 
-  if (!path.empty()) {
-    llvm::Optional<llvm::StringRef> addr;
-    if ((addr = GetURLAddress(path, LISTEN_SCHEME))) {
-      // listen://HOST:PORT
-      return SocketListenAndAccept(*addr, error_ptr);
-    } else if ((addr = GetURLAddress(path, ACCEPT_SCHEME))) {
-      // unix://SOCKNAME
-      return NamedSocketAccept(*addr, error_ptr);
-    } else if ((addr = GetURLAddress(path, UNIX_ACCEPT_SCHEME))) {
-      // unix://SOCKNAME
-      return NamedSocketAccept(*addr, error_ptr);
-    } else if ((addr = GetURLAddress(path, CONNECT_SCHEME))) {
-      return ConnectTCP(*addr, error_ptr);
-    } else if ((addr = GetURLAddress(path, TCP_CONNECT_SCHEME))) {
-      return ConnectTCP(*addr, error_ptr);
-    } else if ((addr = GetURLAddress(path, UDP_SCHEME))) {
-      return ConnectUDP(*addr, error_ptr);
-    } else if ((addr = GetURLAddress(path, UNIX_CONNECT_SCHEME))) {
-      // unix-connect://SOCKNAME
-      return NamedSocketConnect(*addr, error_ptr);
-    } else if ((addr = GetURLAddress(path, UNIX_ABSTRACT_CONNECT_SCHEME))) {
-      // unix-abstract-connect://SOCKNAME
-      return UnixAbstractSocketConnect(*addr, error_ptr);
-    }
-#if LLDB_ENABLE_POSIX
-    else if ((addr = GetURLAddress(path, FD_SCHEME))) {
-      // Just passing a native file descriptor within this current process that
-      // is already opened (possibly from a service or other source).
-      int fd = -1;
-
-      if (!addr->getAsInteger(0, fd)) {
-        // We have what looks to be a valid file descriptor, but we should make
-        // sure it is. We currently are doing this by trying to get the flags
-        // from the file descriptor and making sure it isn't a bad fd.
-        errno = 0;
-        int flags = ::fcntl(fd, F_GETFL, 0);
-        if (flags == -1 || errno == EBADF) {
-          if (error_ptr)
-            error_ptr->SetErrorStringWithFormat("stale file descriptor: %s",
-                                                path.str().c_str());
-          m_read_sp.reset();
-          m_write_sp.reset();
-          return eConnectionStatusError;
-        } else {
-          // Don't take ownership of a file descriptor that gets passed to us
-          // since someone else opened the file descriptor and handed it to us.
-          // TODO: Since are using a URL to open connection we should
-          // eventually parse options using the web standard where we have
-          // "fd://123?opt1=value;opt2=value" and we can have an option be
-          // "owns=1" or "owns=0" or something like this to allow us to specify
-          // this. For now, we assume we must assume we don't own it.
-
-          std::unique_ptr<TCPSocket> tcp_socket;
-          tcp_socket = std::make_unique<TCPSocket>(fd, false, false);
-          // Try and get a socket option from this file descriptor to see if
-          // this is a socket and set m_is_socket accordingly.
-          int resuse;
-          bool is_socket =
-              !!tcp_socket->GetOption(SOL_SOCKET, SO_REUSEADDR, resuse);
-          if (is_socket) {
-            m_read_sp = std::move(tcp_socket);
-            m_write_sp = m_read_sp;
-          } else {
-            m_read_sp = std::make_shared<NativeFile>(
-                fd, File::eOpenOptionReadWrite, false);
-            m_write_sp = m_read_sp;
-          }
-          m_uri = std::string(*addr);
-          return eConnectionStatusSuccess;
-        }
-      }
-
-      if (error_ptr)
-        error_ptr->SetErrorStringWithFormat("invalid file descriptor: \"%s\"",
-                                            path.str().c_str());
-      m_read_sp.reset();
-      m_write_sp.reset();
-      return eConnectionStatusError;
-    } else if ((addr = GetURLAddress(path, FILE_SCHEME))) {
-      std::string addr_str = addr->str();
-      // file:///PATH
-      int fd = llvm::sys::RetryAfterSignal(-1, ::open, addr_str.c_str(), O_RDWR);
-      if (fd == -1) {
-        if (error_ptr)
-          error_ptr->SetErrorToErrno();
-        return eConnectionStatusError;
-      }
+  if (path.empty()) {
+      error_ptr->SetErrorString("invalid connect arguments");
 
-      if (::isatty(fd)) {
-        // Set up serial terminal emulation
-        struct termios options;
-        ::tcgetattr(fd, &options);
+  llvm::StringRef scheme;
+  std::tie(scheme, path) = path.split("://");
 
-        // Set port speed to maximum
-        ::cfsetospeed(&options, B115200);
-        ::cfsetispeed(&options, B115200);
-
-        // Raw input, disable echo and signals
-        options.c_lflag &= ~(ICANON | ECHO | ECHOE | ISIG);
-
-        // Make sure only one character is needed to return from a read
-        options.c_cc[VMIN] = 1;
-        options.c_cc[VTIME] = 0;
+  if (!path.empty()) {
+    auto method =
+        llvm::StringSwitch<ConnectionStatus (ConnectionFileDescriptor::*)(
+            llvm::StringRef, Status *)>(scheme)
+            .Case("listen", &ConnectionFileDescriptor::SocketListenAndAccept)
+            .Cases("accept", "unix-accept",
+                   &ConnectionFileDescriptor::NamedSocketAccept)
+            .Cases("connect", "tcp-connect",
+                   &ConnectionFileDescriptor::ConnectTCP)
+            .Case("udp", &ConnectionFileDescriptor::ConnectUDP)
+            .Case("unix-connect", &ConnectionFileDescriptor::NamedSocketConnect)
+            .Case("unix-abstract-connect",
+                  &ConnectionFileDescriptor::UnixAbstractSocketConnect)
+#if LLDB_ENABLE_POSIX
+            .Case("fd", &ConnectionFileDescriptor::ConnectFD)
+            .Case("file", &ConnectionFileDescriptor::ConnectFile)
+#endif
+            .Default(nullptr);
 
-        llvm::sys::RetryAfterSignal(-1, ::tcsetattr, fd, TCSANOW, &options);
-      }
+    if (method)
+      return (this->*method)(path, error_ptr);
 
-      int flags = ::fcntl(fd, F_GETFL, 0);
-      if (flags >= 0) {
-        if ((flags & O_NONBLOCK) == 0) {
-          flags |= O_NONBLOCK;
-          ::fcntl(fd, F_SETFL, flags);
-        }
-      }
-      m_read_sp =
-          std::make_shared<NativeFile>(fd, File::eOpenOptionReadWrite, true);
-      m_write_sp = m_read_sp;
-      return eConnectionStatusSuccess;
-    }
-#endif
-    if (error_ptr)
-      error_ptr->SetErrorStringWithFormat("unsupported connection URL: '%s'",
-                                          path.str().c_str());
-    return eConnectionStatusError;
-  }
-  if (error_ptr)
-    error_ptr->SetErrorString("invalid connect arguments");
-  return eConnectionStatusError;
-}
 
 bool ConnectionFileDescriptor::InterruptRead() {
   size_t bytes_written = 0;
   Status result = m_pipe.Write("i", 1, bytes_written);
   return result.Success();
 }
 
 ConnectionStatus ConnectionFileDescriptor::Disconnect(Status *error_ptr) {
if (!socket) {
     return eConnectionStatusError;
   }
   m_write_sp = std::move(*socket);
   m_read_sp = m_write_sp;
   m_uri.assign(std::string(s));
   return eConnectionStatusSuccess;
 }
 
+ConnectionStatus ConnectionFileDescriptor::ConnectFD(llvm::StringRef s,
+                                                     Status *error_ptr) {
+#if LLDB_ENABLE_POSIX
+  // Just passing a native file descriptor within this current process that
+  // is already opened (possibly from a service or other source).
+  int fd = -1;
+
+  if (!s.getAsInteger(0, fd)) {
+    // We have what looks to be a valid file descriptor, but we should make
+    // sure it is. We currently are doing this by trying to get the flags
+    // from the file descriptor and making sure it isn't a bad fd.
+    errno = 0;
+    int flags = ::fcntl(fd, F_GETFL, 0);
+    if (flags == -1 || errno == EBADF) {
+      if (error_ptr)
+        error_ptr->SetErrorStringWithFormat("stale file descriptor: %s",
+                                            s.str().c_str());
+      m_read_sp.reset();
+      m_write_sp.reset();
+      return eConnectionStatusError;
+    } else {
+      // Don't take ownership of a file descriptor that gets passed to us
+      // since someone else opened the file descriptor and handed it to us.
+      // TODO: Since are using a URL to open connection we should
+      // eventually parse options using the web standard where we have
+      // "fd://123?opt1=value;opt2=value" and we can have an option be
+      // "owns=1" or "owns=0" or something like this to allow us to specify
+      // this. For now, we assume we must assume we don't own it.
+
+      std::unique_ptr<TCPSocket> tcp_socket;
+      tcp_socket = std::make_unique<TCPSocket>(fd, false, false);
+      // Try and get a socket option from this file descriptor to see if
+      // this is a socket and set m_is_socket accordingly.
+      int resuse;
+      bool is_socket =
+          !!tcp_socket->GetOption(SOL_SOCKET, SO_REUSEADDR, resuse);
+      if (is_socket) {
+        m_read_sp = std::move(tcp_socket);
+        m_write_sp = m_read_sp;
+      } else {
+        m_read_sp =
+            std::make_shared<NativeFile>(fd, File::eOpenOptionReadWrite, false);
+        m_write_sp = m_read_sp;
+      }
+      m_uri = s.str();
+      return eConnectionStatusSuccess;
+    }
+  }
+
+  if (error_ptr)
+    error_ptr->SetErrorStringWithFormat("invalid file descriptor: \"%s\"",
+                                        s.str().c_str());
+  m_read_sp.reset();
+  m_write_sp.reset();
+  return eConnectionStatusError;
+#endif // LLDB_ENABLE_POSIX
+  llvm_unreachable("this function should be only called w/ LLDB_ENABLE_POSIX");
+}
+
+ConnectionStatus ConnectionFileDescriptor::ConnectFile(llvm::StringRef s,
+                                                       Status *error_ptr) {
+#if LLDB_ENABLE_POSIX
+  std::string addr_str = s.str();
+  // file:///PATH
+  int fd = llvm::sys::RetryAfterSignal(-1, ::open, addr_str.c_str(), O_RDWR);
+  if (fd == -1) {
+    if (error_ptr)
+      error_ptr->SetErrorToErrno();
+    return eConnectionStatusError;
+  }
+
+  if (::isatty(fd)) {
+    // Set up serial terminal emulation
+    struct termios options;
+    ::tcgetattr(fd, &options);
+
+    // Set port speed to maximum
+    ::cfsetospeed(&options, B115200);
+    ::cfsetispeed(&options, B115200);
+
+    // Raw input, disable echo and signals
+    options.c_lflag &= ~(ICANON | ECHO | ECHOE | ISIG);
+
+    // Make sure only one character is needed to return from a read
+    options.c_cc[VMIN] = 1;
+    options.c_cc[VTIME] = 0;
+
+    llvm::sys::RetryAfterSignal(-1, ::tcsetattr, fd, TCSANOW, &options);
+  }
+
+  int flags = ::fcntl(fd, F_GETFL, 0);
+  if (flags >= 0) {
+    if ((flags & O_NONBLOCK) == 0) {
+      flags |= O_NONBLOCK;
+      ::fcntl(fd, F_SETFL, flags);
+    }
+  }
+  m_read_sp =
+      std::make_shared<NativeFile>(fd, File::eOpenOptionReadWrite, true);
+  m_write_sp = m_read_sp;
+  return eConnectionStatusSuccess;
+#endif // LLDB_ENABLE_POSIX
+  llvm_unreachable("this function should be only called w/ LLDB_ENABLE_POSIX");
+}
+
 uint16_t
 ConnectionFileDescriptor::GetListeningPort(const Timeout<std::micro> &timeout) {
   auto Result = m_port_predicate.WaitForValueNotEqualTo(0, timeout);
   return Result ? *Result : 0;
 }
 
 bool ConnectionFileDescriptor::GetChildProcessesInherit() const {
   return m_child_processes_inherit;

lldb/source/Plugins/Platform/Android/PlatformAndroidRemoteGDBServer.cpp

Status PlatformAndroidRemoteGDBServer::ConnectRemote(Args &args) {
   if (!url)
     return Status("URL is null.");
   if (!UriParser::Parse(url, scheme, host, remote_port, path))
     return Status("Invalid URL: %s", url);
   if (host != "localhost")
     m_device_id = std::string(host);
 
   m_socket_namespace.reset();
-  if (scheme == ConnectionFileDescriptor::UNIX_CONNECT_SCHEME)
+  if (scheme == "unix-connect")
     m_socket_namespace = AdbClient::UnixSocketNamespaceFileSystem;
-  else if (scheme == ConnectionFileDescriptor::UNIX_ABSTRACT_CONNECT_SCHEME)
+  else if (scheme == "unix-abstract-connect")
     m_socket_namespace = AdbClient::UnixSocketNamespaceAbstract;
 
   std::string connect_url;
   auto error =
       MakeConnectURL(g_remote_platform_pid, (remote_port < 0) ? 0 : remote_port,
                      path, connect_url);
 
   if (error.Fail())