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

[Michał Górny via Phabricator via lldb-commits]

mgorny marked 4 inline comments as done.
mgorny 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);
----------------
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.


================
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) : "");
----------------
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.


================
Comment at: lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp:898
+  // expect the remainder to be the port.
+  if (host_port[0] == ':')
+    host_port = std::string("localhost") + host_port;
----------------
labath wrote:
> Can this string be empty? Maybe use `url_arg.startswith()`, just in case.
I don't think it can, or at least I wasn't able to get an empty string in (the argument parser seems to reject them) but sure, no harm in being extra secure.


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

https://reviews.llvm.org/D111964