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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
