Add Socket::Create factory method which uses socket protocol to find an appropriate implementation class.

[ovyalov]

Summary

Add Socket::Create factory method which uses socket protocol to find an appropriate implementation class.

[zturner]

I'm not sure I agree with this change. The interface to programming with socket is by definition a property of the Host operating system. It seems to me like Host was already the correct place for this code.

[ovyalov]

In D14085#275442, @zturner wrote:

I'm not sure I agree with this change. The interface to programming with socket is by definition a property of the Host operating system. It seems to me like Host was already the correct place for this code.

Base Socket class which is utilized by users remains in Host interface - moving implementation classes into Plugins somewhat simplifies creation/registration process with PluginManager (instead of if (scheme=="tcp"){} else if (scheme=="unix"){}..).

[zturner]

I still think implementation should be in Host as well. If the idea is to simplify the creation scheme, then you could create an enum:

enum class SocketType {
    Tcp,
    Udp,
    UnixDomain
};

and provide a static method on SocketBase like this:

class SocketBase {
public:
    static std::unique_ptr<SocketBase>
    MakeSocket(SocketType type);
};

Although this is in the long term, eventually we will require a modules build of LLDB, so introducing more circular library dependencies should really be avoided, as they are incompatible with modules build and will have to be refactored again once we try to get a modules build working.

Let's see what Greg and others say, but personally I don't think the plugin system should be used solely to gain access to a factory-like creation interface.

[clayborg]

The plug-in interface I was thinking of was the Connection class. This would allow different platforms to support different Connection subclasses for things like network, serial, USB, Firewire, shared memory, IPC and others. Not sure if socket is stand alone enough to warrant its own plug-in interface. How are the socket subclasses used around LLDB? Are they used directly? Or are they place into a Connection class?

[zturner]

I think most of the time they are used in a Connection class, but I don't think it's necessarily guaranteed they will always be that way.

I can think of at least one use case in the future where we will need to open a socket to a server we don't control and stream some data down from the server. I don't see the advantage of using a Connection in that case.

So I would say: Right now it's probably always used in Connection, but I dont' want to lose the flexibility to use it standalone either.

That said, IMO low-level OS primitive abstractions are by definition what Host is for, but higher level abstractions built on top of those can use the plugin interface

[clayborg]

In D14085#275507, @zturner wrote:

I think most of the time they are used in a Connection class, but I don't think it's necessarily guaranteed they will always be that way.

I can think of at least one use case in the future where we will need to open a socket to a server we don't control and stream some data down from the server. I don't see the advantage of using a Connection in that case.

Definitely not.

So I would say: Right now it's probably always used in Connection, but I dont' want to lose the flexibility to use it standalone either.

Agreed. If they are mostly used in connection classes, I would say to do what you said to do: add an enum and factory static method on Socket that uses that enumeration and then make Connection class into plug-ins they themselves vend the URL prefixes and instantiate a connection class with any arguments. So:

1 - convert Socket class to use enum + factory class:

enum class SocketType {
    Tcp,
    Udp,
    UnixDomain
};

class SocketBase {
public:
    static std::unique_ptr<SocketBase>
    MakeSocket(SocketType type);
};

2 - Make Connection classes into plug-ins and have them parse the URLs in the "static Connection *Connection::GetConnectionForURL(const char *url);"

That said, IMO low-level OS primitive abstractions are by definition what Host is for, but higher level abstractions built on top of those can use the plugin interface

That is fine and the above two step solution would enforce that.

[ovyalov]

In D14085#275507, @zturner wrote:

I think most of the time they are used in a Connection class, but I don't think it's necessarily guaranteed they will always be that way.

I can think of at least one use case in the future where we will need to open a socket to a server we don't control and stream some data down from the server. I don't see the advantage of using a Connection in that case.

So I would say: Right now it's probably always used in Connection, but I dont' want to lose the flexibility to use it standalone either.

That said, IMO low-level OS primitive abstractions are by definition what Host is for, but higher level abstractions built on top of those can use the plugin interface

There are some corner cases that don't fit into regular model:

• Creation of UDP connection - LLDB uses pair of send/receive sockets and that's why we need to use UDPSocket::Connect instead of using factory method.
• ConnectionFileDescriptor::Connect creates a new TCP socket without giving up the ownership of a socket handle.

As an option, we can introduce static std::unique_ptr<Socket> Socket::Create(const char* scheme, bool child_processes_inherit, Error& error); as socket class creator method but without moving socket classes into plugins.

[zturner]

Do we need this GetScheme() method? And instead of having Create take a string, couldn't it just take a SocketProtocol instead? It seems like the logic of converting the string to the enum should be done in the Acceptor class, as this URI format is defined by the acceptor, and the socket itself is just a low-level host layer abstraction.

[ovyalov]

Thanks for suggestions - addressed review comments.

[ovyalov]

Files:

/lldb/trunk/include/lldb/Host/Socket.h
/lldb/trunk/include/lldb/Host/common/UDPSocket.h
/lldb/trunk/source/Host/common/Socket.cpp
/lldb/trunk/tools/lldb-server/Acceptor.cpp
/lldb/trunk/tools/lldb-server/Acceptor.h

Users:

ovyalov (Author)

http://reviews.llvm.org/rL251417

include/lldb/Host/Socket.h

 //===-- Socket.h ------------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef liblldb_Host_Socket_h_
 #define liblldb_Host_Socket_h_
 
+#include <memory>
 #include <string>
 
 #include "lldb/lldb-private.h"
 
 #include "lldb/Core/Error.h"
 #include "lldb/Host/IOObject.h"
 #include "lldb/Host/Predicate.h"
 #include "lldb/Host/SocketAddress.h"
public:
     typedef enum
     {
         ProtocolTcp,
         ProtocolUdp,
         ProtocolUnixDomain,
         ProtocolUnixAbstract
     } SocketProtocol;
 
+    static const char *TCP;
+    static const char *UDP;
+    static const char *UNIX;
+    static const char *UNIX_ABSTRACT;
+
     static const NativeSocket kInvalidSocketValue;
 
     ~Socket() override;
 
+    static std::unique_ptr<Socket> Create(const char *scheme, bool child_processes_inherit, Error &error);
+
     virtual Error Connect(llvm::StringRef name) = 0;
     virtual Error Listen(llvm::StringRef name, int backlog) = 0;
     virtual Error Accept(llvm::StringRef name, bool child_processes_inherit, Socket *&socket) = 0;
 
     // Initialize a Tcp Socket object in listening mode.  listen and accept are implemented
     // separately because the caller may wish to manipulate or query the socket after it is
     // initialized, but before entering a blocking accept.
     static Error TcpListen(
public:
     static Error UnixAbstractConnect(llvm::StringRef host_and_port, bool child_processes_inherit, Socket *&socket);
     static Error 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; }
+    virtual const char* GetScheme() const = 0;
 
     Error Read (void *buf, size_t &num_bytes) override;
     Error Write (const void *buf, size_t &num_bytes) override;
 
     virtual Error PreDisconnect ();
     Error Close() override;
 
     bool IsValid () const override { return m_socket != kInvalidSocketValue; }

include/lldb/Host/common/TCPSocket.h

public:
         std::string GetRemoteIPAddress () const;
 
         int SetOptionNoDelay();
         int SetOptionReuseAddress();
 
         Error Connect(llvm::StringRef name) override;
         Error Listen(llvm::StringRef name, int backlog) override;
         Error Accept(llvm::StringRef name, bool child_processes_inherit, Socket *&conn_socket) override;
+
+        const char * GetScheme() const override;
     };
 }
 
 #endif // ifndef liblldb_TCPSocket_h_

include/lldb/Host/common/UDPSocket.h

 
 #include "lldb/Host/Socket.h"
 
 namespace lldb_private
 {
     class UDPSocket: public Socket
     {
     public:
+        UDPSocket(bool child_processes_inherit, Error &error);
+
         static Error Connect(llvm::StringRef name, bool child_processes_inherit, Socket *&send_socket, Socket *&recv_socket);
+        const char * GetScheme() const override;
 
     private:
         UDPSocket(NativeSocket socket);
-        UDPSocket(bool child_processes_inherit, Error &error);
 
         size_t Send(const void *buf, const size_t num_bytes) override;
         Error Connect(llvm::StringRef name) override;
         Error Listen(llvm::StringRef name, int backlog) override;
         Error Accept(llvm::StringRef name, bool child_processes_inherit, Socket *&socket) override;
 
         SocketAddress m_send_sockaddr;
     };
 }
 
 #endif // ifndef liblldb_UDPSocket_h_

include/lldb/Host/linux/AbstractSocket.h

 
 namespace lldb_private
 {
     class AbstractSocket: public DomainSocket
     {
     public:
         AbstractSocket(bool child_processes_inherit, Error &error);
 
+        const char * GetScheme() const override;
+
     protected:
         size_t GetNameOffset() const override;
         void DeleteSocketFile(llvm::StringRef name) override;
     };
 }
 
 #endif // ifndef liblldb_AbstractSocket_h_

include/lldb/Host/posix/DomainSocket.h

namespace lldb_private
     {
     public:
         DomainSocket(bool child_processes_inherit, Error &error);
 
         Error Connect(llvm::StringRef name) override;
         Error Listen(llvm::StringRef name, int backlog) override;
         Error Accept(llvm::StringRef name, bool child_processes_inherit, Socket *&socket) override;
 
+        const char * GetScheme() const override;
+
     protected:
         DomainSocket(SocketProtocol protocol, bool child_processes_inherit, Error &error);
 
         virtual size_t GetNameOffset() const;
         virtual void DeleteSocketFile(llvm::StringRef name);
 
     private:
         DomainSocket(NativeSocket socket);
     };
 }
 
 #endif // ifndef liblldb_DomainSocket_h_

source/Host/common/Socket.cpp

#if defined(_WIN32)
     return ::WSAGetLastError() == WSAEINTR;
 #else
     return errno == EINTR;
 #endif
 }
 
 }
 
+const char * Socket::TCP = "tcp";
+const char * Socket::UDP = "udp";
+const char * Socket::UNIX = "unix";
+const char * Socket::UNIX_ABSTRACT = "unix-abstract";
+
 Socket::Socket(NativeSocket socket, SocketProtocol protocol, bool should_close)
     : IOObject(eFDTypeSocket, should_close)
     , m_protocol(protocol)
     , m_socket(socket)
 {
 
 }
 
 Socket::~Socket()
 {
     Close();
 }
 
+std::unique_ptr<Socket> Socket::Create(const char *scheme, bool child_processes_inherit, Error &error)
+{
+    error.Clear();
+
+    std::unique_ptr<Socket> socket_up;
+    if (!strcmp(scheme, TCP))
+        socket_up.reset(new TCPSocket(child_processes_inherit, error));
+    else if (!strcmp(scheme, UDP))
+        socket_up.reset(new UDPSocket(child_processes_inherit, error));
+    else if (!strcmp(scheme, UNIX))
+#ifndef LLDB_DISABLE_POSIX
+        socket_up.reset(new DomainSocket(child_processes_inherit, error));
+#else
+        error.SetErrorString("Unix domain sockets are not supported on this platform.");
+#endif
+    else if (!strcmp(scheme, UNIX_ABSTRACT))
+#ifdef __linux__
+        socket_up.reset(new AbstractSocket(child_processes_inherit, error));
+#else
+        error.SetErrorString("Abstract domain sockets are not supported on this platform.");
+#endif
+    else
+        error.SetErrorString("Unknown scheme");
+
+    if (error.Fail())
+        socket_up.reset();
+
+    return socket_up;
+}
+
 Error Socket::TcpConnect(llvm::StringRef host_and_port, bool child_processes_inherit, Socket *&socket)
 {
     Log *log(lldb_private::GetLogIfAnyCategoriesSet (LIBLLDB_LOG_COMMUNICATION));
     if (log)
         log->Printf ("Socket::%s (host/port = %s)", __FUNCTION__, host_and_port.data());
 
     Error error;
-    std::unique_ptr<TCPSocket> connect_socket(new TCPSocket(child_processes_inherit, error));
+    std::unique_ptr<Socket> connect_socket(Create(TCP, child_processes_inherit, error));
     if (error.Fail())
         return error;
 
     error = connect_socket->Connect(host_and_port);
     if (error.Success())
       socket = connect_socket.release();
 
     return error;
if (log)
         log->Printf ("Socket::%s (host/port = %s)", __FUNCTION__, host_and_port.data());
 
     return UDPSocket::Connect(host_and_port, child_processes_inherit, send_socket, recv_socket);
 }
 
 Error Socket::UnixDomainConnect(llvm::StringRef name, bool child_processes_inherit, Socket *&socket)
 {
     Error error;
-#ifndef LLDB_DISABLE_POSIX
-    std::unique_ptr<DomainSocket> connect_socket(new DomainSocket(child_processes_inherit, error));
+    std::unique_ptr<Socket> connect_socket(Create(UNIX, child_processes_inherit, error));
     if (error.Fail())
         return error;
 
     error = connect_socket->Connect(name);
     if (error.Success())
       socket = connect_socket.release();
-#else
-    error.SetErrorString("Unix domain sockets are not supported on this platform.");
-#endif
+
     return error;
 }
 
 Error Socket::UnixDomainAccept(llvm::StringRef name, bool child_processes_inherit, Socket *&socket)
 {
     Error error;
-#ifndef LLDB_DISABLE_POSIX
-    std::unique_ptr<DomainSocket> listen_socket(new DomainSocket(child_processes_inherit, error));
+    std::unique_ptr<Socket> listen_socket(Create(UNIX, child_processes_inherit, error));
     if (error.Fail())
         return error;
 
     error = listen_socket->Listen(name, 5);
     if (error.Fail())
         return error;
 
     error = listen_socket->Accept(name, child_processes_inherit, socket);
-#else
-    error.SetErrorString("Unix domain sockets are not supported on this platform.");
-#endif
     return error;
 }
 
 Error
 Socket::UnixAbstractConnect(llvm::StringRef name, bool child_processes_inherit, Socket *&socket)
 {
     Error error;
-#ifdef __linux__
-    std::unique_ptr<Socket> connect_socket(new AbstractSocket(child_processes_inherit, error));
+    std::unique_ptr<Socket> connect_socket(Create(UNIX_ABSTRACT, child_processes_inherit, error));
     if (error.Fail())
         return error;
 
     error = connect_socket->Connect(name);
     if (error.Success())
       socket = connect_socket.release();
-#else
-    error.SetErrorString("Abstract domain sockets are not supported on this platform.");
-#endif
     return error;
 }
 
 Error
 Socket::UnixAbstractAccept(llvm::StringRef name, bool child_processes_inherit, Socket *&socket)
 {
     Error error;
-#ifdef __linux__
-    std::unique_ptr<Socket> listen_socket(new AbstractSocket(child_processes_inherit, error));
+    std::unique_ptr<Socket> listen_socket(Create(UNIX_ABSTRACT,child_processes_inherit, error));
     if (error.Fail())
         return error;
 
     error = listen_socket->Listen(name, 5);
     if (error.Fail())
         return error;
 
     error = listen_socket->Accept(name, child_processes_inherit, socket);
-#else
-    error.SetErrorString("Abstract domain sockets are not supported on this platform.");
-#endif
     return error;
 }
 
 bool
 Socket::DecodeHostAndPort(llvm::StringRef host_and_port,
                           std::string &host_str,
                           std::string &port_str,
                           int32_t& port,

source/Host/common/TCPSocket.cpp

TCPSocket::SetOptionNoDelay()
     return SetOption (IPPROTO_TCP, TCP_NODELAY, 1);
 }
 
 int
 TCPSocket::SetOptionReuseAddress()
 {
     return SetOption(SOL_SOCKET, SO_REUSEADDR, 1);
 }
+
+const char *
+TCPSocket::GetScheme() const
+{
+    return TCP;
+}

source/Host/common/UDPSocket.cpp

UDPSocket::Connect(llvm::StringRef name, bool child_processes_inherit, Socket *&send_socket, Socket *&recv_socket)
     if (!final_send_socket)
         return error;
 
     send_socket = final_send_socket.release();
     recv_socket = final_recv_socket.release();
     error.Clear();
     return error;
 }
+
+const char *
+UDPSocket::GetScheme() const
+{
+    return UDP;
+}

source/Host/linux/AbstractSocket.cpp

 {
     return 1;
 }
 
 void
 AbstractSocket::DeleteSocketFile(llvm::StringRef name)
 {
 }
+
+const char *
+AbstractSocket::GetScheme() const
+{
+    return UNIX_ABSTRACT;
+}

source/Host/posix/DomainSocket.cpp

DomainSocket::GetNameOffset() const
     return 0;
 }
 
 void
 DomainSocket::DeleteSocketFile(llvm::StringRef name)
 {
     FileSystem::Unlink(FileSpec{name, true});
 }
+
+const char *
+DomainSocket::GetScheme() const
+{
+    return UNIX;
+}

tools/lldb-server/Acceptor.cpp

 //===-- Acceptor.cpp --------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Acceptor.h"
 
 #include "llvm/ADT/StringRef.h"
 
 #include "lldb/Core/StreamString.h"
 #include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/Host/common/TCPSocket.h"
-#include "lldb/Host/posix/DomainSocket.h"
+
+#include "Utility/UriParser.h"
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::lldb_server;
 using namespace llvm;
 
 Error
 Acceptor::Listen(int backlog)
Acceptor::GetLocalSocketId() const
     return m_local_socket_id();
 }
 
 std::unique_ptr<Acceptor>
 Acceptor::Create(StringRef name, const bool child_processes_inherit, Error &error)
 {
     error.Clear();
 
-    LocalSocketIdFunc local_socket_id;
-    std::unique_ptr<Socket> listener_socket = nullptr;
+    std::unique_ptr<Socket> listener_socket_up = nullptr;
+
+
+    int port;
+    std::string scheme, host, path;
+    // Try to match socket name as URL - e.g., tcp://localhost:5555
+    if (UriParser::Parse(name.str(), scheme, host, port, path))
+    {
+        listener_socket_up = Socket::Create(scheme.c_str(), child_processes_inherit, error);
+        name = name.drop_front(scheme.size() + strlen("://"));
+    }
+    else
+    {
         std::string host_str;
         std::string port_str;
         int32_t port = INT32_MIN;
+        // Try to match socket name as $host:port - e.g., localhost:5555
         if (Socket::DecodeHostAndPort (name, host_str, port_str, port, &error))
+            listener_socket_up = Socket::Create(Socket::TCP, child_processes_inherit, error);
+        else
+            listener_socket_up = Socket::Create(Socket::UNIX, child_processes_inherit, error);
+    }
+
+    LocalSocketIdFunc local_socket_id;
+    if (error.Success())
+    {
+        if (listener_socket_up->GetSocketProtocol() == Socket::ProtocolTcp)
         {
-        auto tcp_socket = new TCPSocket(child_processes_inherit, error);
+            TCPSocket* tcp_socket = static_cast<TCPSocket*>(listener_socket_up.get());
             local_socket_id = [tcp_socket]() {
                 auto local_port = tcp_socket->GetLocalPortNumber();
                 return (local_port != 0) ? std::to_string(local_port) : "";
             };
-        listener_socket.reset(tcp_socket);
         }
         else
         {
             const std::string socket_name = name;
             local_socket_id = [socket_name](){
                 return socket_name;
             };
-        listener_socket.reset(new DomainSocket(child_processes_inherit, error));
         }
 
-    if (error.Success())
         return std::unique_ptr<Acceptor>(
-            new Acceptor(std::move(listener_socket), name, local_socket_id));
+            new Acceptor(std::move(listener_socket_up), name, local_socket_id));
+    }
 
     return std::unique_ptr<Acceptor>();
 }
 
 Acceptor::Acceptor(std::unique_ptr<Socket> &&listener_socket,
                    StringRef name,
                    const LocalSocketIdFunc &local_socket_id)
     : m_listener_socket_up(std::move(listener_socket)),
       m_name(name.str()),
       m_local_socket_id(local_socket_id)
 {
 }