Re: [Lldb-commits] [PATCH] D14085: Make Socket to support plugin interface

2015-10-26 Thread Oleksiy Vyalov via lldb-commits
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

2015-10-26 Thread Greg Clayton via lldb-commits
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

2015-10-26 Thread Zachary Turner via lldb-commits
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

2015-10-26 Thread Greg Clayton via lldb-commits
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

2015-10-26 Thread Zachary Turner via lldb-commits
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

2015-10-26 Thread Oleksiy Vyalov via lldb-commits
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

2015-10-26 Thread Zachary Turner via lldb-commits
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