Re: [Lldb-commits] [PATCH] D14085: Make Socket to support plugin interface
ovyalov added a comment. In http://reviews.llvm.org/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::Create(const char* scheme, bool child_processes_inherit, Error& error); as socket class creator method but without moving socket classes into plugins. http://reviews.llvm.org/D14085 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D14085: Make Socket to support plugin interface
clayborg added a comment. In http://reviews.llvm.org/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 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. http://reviews.llvm.org/D14085 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D14085: Make Socket to support plugin interface
zturner added a comment. 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 http://reviews.llvm.org/D14085 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D14085: Make Socket to support plugin interface
clayborg added a comment. 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? http://reviews.llvm.org/D14085 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D14085: Make Socket to support plugin interface
zturner added a comment. 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 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. http://reviews.llvm.org/D14085 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D14085: Make Socket to support plugin interface
ovyalov added a comment. In http://reviews.llvm.org/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"){}..). http://reviews.llvm.org/D14085 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D14085: Make Socket to support plugin interface
zturner added a subscriber: zturner. zturner requested changes to this revision. zturner added a reviewer: zturner. zturner added a comment. This revision now requires changes to proceed. 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. http://reviews.llvm.org/D14085 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits