[Lldb-commits] [PATCH] D111964: [lldb] [lldb-gdbserver] Unify listen/connect code to use ConnectionFileDescriptor

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp:583-610
+ConnectionStatus ConnectionFileDescriptor::UnixAbstractSocketAccept(
+    llvm::StringRef socket_name,
+    std::function<void(llvm::StringRef local_socket_id)> socket_id_callback,
+    Status *error_ptr) {
+  Status error;
+  std::unique_ptr<Socket> listen_socket = Socket::Create(
+      Socket::ProtocolUnixAbstract, m_child_processes_inherit, error);
----------------
mgorny wrote:
> labath wrote:
> > This is identical to `NamedSocketAccept` modulo the socket type constant, right? Can you merge these?
> Yes; in fact, most of the functions are pretty similar. However, I'd prefer to defer merging them until after the port predicate removal patch, as that should also open the possibility of combining them with the TCP socket methods too.
Ok, sounds good.


================
Comment at: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp:647
 
+  uint16_t port = listening_socket.get()->GetLocalPortNumber();
+  socket_id_callback(port != 0 ? std::to_string(port) : "");
----------------
mgorny wrote:
> labath wrote:
> > Can this ever be zero ?
> Yes, when `getsockname()` fails. I suppose we could modify it to use `llvm::Optional<...>` or even `llvm::Expected<...>` but this is one place where I don't think we need to care much about proper error handling.
But how can it fail? This is a socket that we've just created and its totally under our control...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111964/new/

https://reviews.llvm.org/D111964