[Lldb-commits] [PATCH] D31823: Update LLDB Host to support IPv6 over TCP

[Zachary Turner via Phabricator via lldb-commits]

zturner added a comment.

Networking isn't my area of domain expertise, so these are mostly just general comments.



================
Comment at: include/lldb/Host/common/TCPSocket.h:55
+
+  std::map<int, SocketAddress> m_listen_sockets;
 };
----------------
Any particular reason you're using a `std::map` instead of a `DenseMap` or other similar LLVM structure?  I was under the impression that the number of times where you should actually use `std::map` are pretty small.


================
Comment at: source/Host/common/Socket.cpp:86
   case ProtocolTcp:
-    socket_up.reset(new TCPSocket(child_processes_inherit, error));
+    socket_up.reset(new TCPSocket(true, child_processes_inherit));
     break;
----------------
Is this ever set to `false` anywhere?  If not, just delete the parameter.

Also, since you're in this code anyway, maybe you could use `llvm::make_unique`.


================
Comment at: source/Host/common/Socket.cpp:150
   std::unique_ptr<TCPSocket> listen_socket(
-      new TCPSocket(child_processes_inherit, error));
+      new TCPSocket(child_processes_inherit, true));
   if (error.Fail())
----------------
The parameters look reversed here.


================
Comment at: source/Host/common/Socket.cpp:260
+      if (host_str.front() == '[' && host_str.back() == ']')
+        host_str = host_str.substr(1, host_str.size() - 2);
       bool ok = false;
----------------
Makes me wish we had `StringRef::shrink(int N)`.  Oh well, this is fine.


================
Comment at: source/Host/common/TCPSocket.cpp:29
+#include <sys/event.h>
+#elif defined(_WIN32)
+#include <winsock2.h>
----------------
Can you use `LLVM_ON_WIN32`?


================
Comment at: source/Host/common/TCPSocket.cpp:60
+bool TCPSocket::IsValid() const {
+  return m_socket != kInvalidSocketValue || m_listen_sockets.size() != 0;
+}
----------------
`!m_listen_sockets.empty()`


================
Comment at: source/Host/common/TCPSocket.cpp:116
+  Error error;
+  if (IsValid())
+    error = Close();
----------------
Should we perhaps assert here?  Seems like creating 2 sockets back to back without cleaning up after yourself is not intended behavior.  The owner of a socket should probably close it before they try to create a new one, and an `assert(!IsValid())` would catch that.


================
Comment at: source/Host/common/TCPSocket.cpp:194
+    if (-1 == err) {
+#if defined(_WIN32)
+      closesocket(fd);
----------------
`LLVM_ON_WIN32`.

Also can you raise this up into something like this:

```
#if defined(LLVM_ON_WIN32)
  #define CLOSE_SOCKET ::closesocket
#else
  #define CLOSE_SOCKET ::close
#endif
```

Then just write `CLOSE_SOCKET(fd);`


================
Comment at: source/Host/common/TCPSocket.cpp:217
+  for (auto socket : m_listen_sockets)
+#if defined(_WIN32)
+    closesocket(socket.first);
----------------
Use `CLOSE_SOCKET` macro from earlier.


================
Comment at: source/Host/common/TCPSocket.cpp:250
+#else
+  std::vector<struct pollfd> sock_set;
+
----------------
Can you add `sock_set.reserve(m_listen_sockets.size())`?


================
Comment at: source/Host/common/TCPSocket.cpp:284
+#if defined(_WIN32)
+    int retval = WSAPoll(sock_set.data(), sock_set.size(), 1000);
+#else
----------------
labath wrote:
> Why not simply use infinite timeout if that's the behaviour you desire?
Similar to before, you could add a `#define POLL_SOCKET ...`


================
Comment at: source/Host/common/TCPSocket.cpp:318
+    if (!AddrIn.IsAnyAddr() && AccptAddr != AddrIn) {
+#if defined(_WIN32)
+      closesocket(sock);
----------------
`CLOSE_SOCKET(fd)`


================
Comment at: source/Host/common/TCPSocket.cpp:323-326
+      ::fprintf(
+          stderr,
+          "error: rejecting incoming connection from %s (expecting %s)\n",
+          AccptAddr.GetIPAddress().c_str(), AddrIn.GetIPAddress().c_str());
----------------
`llvm::errs() << formatv("error: rejecting incoming connection from {0} (expecting {1})", AcceptAddr.GetIPAddress(), AddrIn.GetIPAddress());`


================
Comment at: source/Host/common/TCPSocket.cpp:330
+    accept_connection = true;
+    accepted_socket.reset(new TCPSocket(sock, *this));
   }
----------------
`make_unique`


================
Comment at: source/Host/common/UDPSocket.cpp:125
+Error UDPSocket::Listen(llvm::StringRef name, int backlog) {
+  return Error("%s", g_not_supported_error);
+}
----------------
This seems odd.  Does `return Error(g_not_supported_error)` not work?


================
Comment at: source/Host/common/UDPSocket.cpp:145-146
+                         Socket *&socket) {
+  std::unique_ptr<UDPSocket> final_socket(
+      new UDPSocket(true, child_processes_inherit));
+  Error error = final_socket->Connect(name);
----------------
`make_unique` again.


https://reviews.llvm.org/D31823