[lldb] Add a MainLoop version of DomainSocket::Accept #108188
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
To go along with the existing TCPSocket implementation.
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesTo go along with the existing TCPSocket implementation. Full diff: https://github.com/llvm/llvm-project/pull/108188.diff 9 Files Affected:
diff --git a/lldb/include/lldb/Host/Socket.h b/lldb/include/lldb/Host/Socket.h
index 764a048976eb41..14468c98ac5a3a 100644
--- a/lldb/include/lldb/Host/Socket.h
+++ b/lldb/include/lldb/Host/Socket.h
@@ -12,6 +12,7 @@
#include <memory>
#include <string>
+#include "lldb/Host/MainLoopBase.h"
#include "lldb/lldb-private.h"
#include "lldb/Host/SocketAddress.h"
@@ -97,7 +98,17 @@ class Socket : public IOObject {
virtual Status Connect(llvm::StringRef name) = 0;
virtual Status Listen(llvm::StringRef name, int backlog) = 0;
- virtual Status Accept(Socket *&socket) = 0;
+
+ // Use the provided main loop instance to accept new connections. The callback
+ // will be called (from MainLoop::Run) for each new connection. This function
+ // does not block.
+ virtual llvm::Expected<std::vector<MainLoopBase::ReadHandleUP>>
+ Accept(MainLoopBase &loop,
+ std::function<void(std::unique_ptr<Socket> socket)> sock_cb) = 0;
+
+ // Accept a single connection and "return" it in the pointer argument. This
+ // function blocks until the connection arrives.
+ virtual Status Accept(Socket *&socket);
// Initialize a Tcp Socket object in listening mode. listen and accept are
// implemented separately because the caller may wish to manipulate or query
diff --git a/lldb/include/lldb/Host/common/TCPSocket.h b/lldb/include/lldb/Host/common/TCPSocket.h
index 78e80568e39967..eefe0240fe4a95 100644
--- a/lldb/include/lldb/Host/common/TCPSocket.h
+++ b/lldb/include/lldb/Host/common/TCPSocket.h
@@ -42,16 +42,10 @@ class TCPSocket : public Socket {
Status Connect(llvm::StringRef name) override;
Status Listen(llvm::StringRef name, int backlog) override;
- // Use the provided main loop instance to accept new connections. The callback
- // will be called (from MainLoop::Run) for each new connection. This function
- // does not block.
+ using Socket::Accept;
llvm::Expected<std::vector<MainLoopBase::ReadHandleUP>>
Accept(MainLoopBase &loop,
- std::function<void(std::unique_ptr<TCPSocket> socket)> sock_cb);
-
- // Accept a single connection and "return" it in the pointer argument. This
- // function blocks until the connection arrives.
- Status Accept(Socket *&conn_socket) override;
+ std::function<void(std::unique_ptr<Socket> socket)> sock_cb) override;
Status CreateSocket(int domain);
diff --git a/lldb/include/lldb/Host/common/UDPSocket.h b/lldb/include/lldb/Host/common/UDPSocket.h
index bae707e345d87c..7348010d02ada2 100644
--- a/lldb/include/lldb/Host/common/UDPSocket.h
+++ b/lldb/include/lldb/Host/common/UDPSocket.h
@@ -27,7 +27,13 @@ class UDPSocket : public Socket {
size_t Send(const void *buf, const size_t num_bytes) override;
Status Connect(llvm::StringRef name) override;
Status Listen(llvm::StringRef name, int backlog) override;
- Status Accept(Socket *&socket) override;
+
+ llvm::Expected<std::vector<MainLoopBase::ReadHandleUP>>
+ Accept(MainLoopBase &loop,
+ std::function<void(std::unique_ptr<Socket> socket)> sock_cb) override {
+ return llvm::errorCodeToError(
+ std::make_error_code(std::errc::operation_not_supported));
+ }
SocketAddress m_sockaddr;
};
diff --git a/lldb/include/lldb/Host/posix/DomainSocket.h b/lldb/include/lldb/Host/posix/DomainSocket.h
index 35c33811f60de6..983f43bd633719 100644
--- a/lldb/include/lldb/Host/posix/DomainSocket.h
+++ b/lldb/include/lldb/Host/posix/DomainSocket.h
@@ -14,11 +14,17 @@
namespace lldb_private {
class DomainSocket : public Socket {
public:
+ DomainSocket(NativeSocket socket, bool should_close,
+ bool child_processes_inherit);
DomainSocket(bool should_close, bool child_processes_inherit);
Status Connect(llvm::StringRef name) override;
Status Listen(llvm::StringRef name, int backlog) override;
- Status Accept(Socket *&socket) override;
+
+ using Socket::Accept;
+ llvm::Expected<std::vector<MainLoopBase::ReadHandleUP>>
+ Accept(MainLoopBase &loop,
+ std::function<void(std::unique_ptr<Socket> socket)> sock_cb) override;
std::string GetRemoteConnectionURI() const override;
diff --git a/lldb/source/Host/common/Socket.cpp b/lldb/source/Host/common/Socket.cpp
index 1a63571b94c6f1..d69eb608204033 100644
--- a/lldb/source/Host/common/Socket.cpp
+++ b/lldb/source/Host/common/Socket.cpp
@@ -10,6 +10,7 @@
#include "lldb/Host/Config.h"
#include "lldb/Host/Host.h"
+#include "lldb/Host/MainLoop.h"
#include "lldb/Host/SocketAddress.h"
#include "lldb/Host/common/TCPSocket.h"
#include "lldb/Host/common/UDPSocket.h"
@@ -459,6 +460,19 @@ NativeSocket Socket::CreateSocket(const int domain, const int type,
return sock;
}
+Status Socket::Accept(Socket *&socket) {
+ MainLoop accept_loop;
+ llvm::Expected<std::vector<MainLoopBase::ReadHandleUP>> expected_handles =
+ Accept(accept_loop,
+ [&accept_loop, &socket](std::unique_ptr<Socket> sock) {
+ socket = sock.release();
+ accept_loop.RequestTermination();
+ });
+ if (!expected_handles)
+ return Status::FromError(expected_handles.takeError());
+ return accept_loop.Run();
+}
+
NativeSocket Socket::AcceptSocket(NativeSocket sockfd, struct sockaddr *addr,
socklen_t *addrlen,
bool child_processes_inherit, Status &error) {
diff --git a/lldb/source/Host/common/TCPSocket.cpp b/lldb/source/Host/common/TCPSocket.cpp
index b28ba148ee1afa..2d16b605af9497 100644
--- a/lldb/source/Host/common/TCPSocket.cpp
+++ b/lldb/source/Host/common/TCPSocket.cpp
@@ -232,9 +232,9 @@ void TCPSocket::CloseListenSockets() {
m_listen_sockets.clear();
}
-llvm::Expected<std::vector<MainLoopBase::ReadHandleUP>> TCPSocket::Accept(
- MainLoopBase &loop,
- std::function<void(std::unique_ptr<TCPSocket> socket)> sock_cb) {
+llvm::Expected<std::vector<MainLoopBase::ReadHandleUP>>
+TCPSocket::Accept(MainLoopBase &loop,
+ std::function<void(std::unique_ptr<Socket> socket)> sock_cb) {
if (m_listen_sockets.size() == 0)
return llvm::createStringError("No open listening sockets!");
@@ -278,19 +278,6 @@ llvm::Expected<std::vector<MainLoopBase::ReadHandleUP>> TCPSocket::Accept(
return handles;
}
-Status TCPSocket::Accept(Socket *&conn_socket) {
- MainLoop accept_loop;
- llvm::Expected<std::vector<MainLoopBase::ReadHandleUP>> expected_handles =
- Accept(accept_loop,
- [&accept_loop, &conn_socket](std::unique_ptr<TCPSocket> sock) {
- conn_socket = sock.release();
- accept_loop.RequestTermination();
- });
- if (!expected_handles)
- return Status::FromError(expected_handles.takeError());
- return accept_loop.Run();
-}
-
int TCPSocket::SetOptionNoDelay() {
return SetOption(IPPROTO_TCP, TCP_NODELAY, 1);
}
diff --git a/lldb/source/Host/common/UDPSocket.cpp b/lldb/source/Host/common/UDPSocket.cpp
index 2a7a6cff414b14..05d7b2e6506027 100644
--- a/lldb/source/Host/common/UDPSocket.cpp
+++ b/lldb/source/Host/common/UDPSocket.cpp
@@ -47,10 +47,6 @@ Status UDPSocket::Listen(llvm::StringRef name, int backlog) {
return Status::FromErrorStringWithFormat("%s", g_not_supported_error);
}
-Status UDPSocket::Accept(Socket *&socket) {
- return Status::FromErrorStringWithFormat("%s", g_not_supported_error);
-}
-
llvm::Expected<std::unique_ptr<UDPSocket>>
UDPSocket::Connect(llvm::StringRef name, bool child_processes_inherit) {
std::unique_ptr<UDPSocket> socket;
diff --git a/lldb/source/Host/posix/DomainSocket.cpp b/lldb/source/Host/posix/DomainSocket.cpp
index 2d18995c3bb469..369123f2239302 100644
--- a/lldb/source/Host/posix/DomainSocket.cpp
+++ b/lldb/source/Host/posix/DomainSocket.cpp
@@ -7,11 +7,13 @@
//===----------------------------------------------------------------------===//
#include "lldb/Host/posix/DomainSocket.h"
+#include "lldb/Utility/LLDBLog.h"
#include "llvm/Support/Errno.h"
#include "llvm/Support/FileSystem.h"
#include <cstddef>
+#include <memory>
#include <sys/socket.h>
#include <sys/un.h>
@@ -57,7 +59,14 @@ static bool SetSockAddr(llvm::StringRef name, const size_t name_offset,
}
DomainSocket::DomainSocket(bool should_close, bool child_processes_inherit)
- : Socket(ProtocolUnixDomain, should_close, child_processes_inherit) {}
+ : DomainSocket(kInvalidSocketValue, should_close, child_processes_inherit) {
+}
+
+DomainSocket::DomainSocket(NativeSocket socket, bool should_close,
+ bool child_processes_inherit)
+ : Socket(ProtocolUnixDomain, should_close, child_processes_inherit) {
+ m_socket = socket;
+}
DomainSocket::DomainSocket(SocketProtocol protocol,
bool child_processes_inherit)
@@ -108,14 +117,31 @@ Status DomainSocket::Listen(llvm::StringRef name, int backlog) {
return error;
}
-Status DomainSocket::Accept(Socket *&socket) {
- Status error;
- auto conn_fd = AcceptSocket(GetNativeSocket(), nullptr, nullptr,
- m_child_processes_inherit, error);
- if (error.Success())
- socket = new DomainSocket(conn_fd, *this);
+llvm::Expected<std::vector<MainLoopBase::ReadHandleUP>> DomainSocket::Accept(
+ MainLoopBase &loop,
+ std::function<void(std::unique_ptr<Socket> socket)> sock_cb) {
+ // TODO: Refactor MainLoop to avoid the shared_ptr requirement.
+ auto io_sp = std::make_shared<DomainSocket>(GetNativeSocket(), false,
+ m_child_processes_inherit);
+ auto cb = [this, sock_cb](MainLoopBase &loop) {
+ Log *log = GetLog(LLDBLog::Host);
+ Status error;
+ auto conn_fd = AcceptSocket(GetNativeSocket(), nullptr, nullptr,
+ m_child_processes_inherit, error);
+ if (error.Fail()) {
+ LLDB_LOG(log, "AcceptSocket({0}): {1}", GetNativeSocket(), error);
+ return;
+ }
+ std::unique_ptr<DomainSocket> sock_up(new DomainSocket(conn_fd, *this));
+ sock_cb(std::move(sock_up));
+ };
- return error;
+ Status error;
+ std::vector<MainLoopBase::ReadHandleUP> handles;
+ handles.emplace_back(loop.RegisterReadObject(io_sp, cb, error));
+ if (error.Fail())
+ return error.ToError();
+ return handles;
}
size_t DomainSocket::GetNameOffset() const { return 0; }
diff --git a/lldb/unittests/Host/SocketTest.cpp b/lldb/unittests/Host/SocketTest.cpp
index 3a356d11ba1a51..a93b928e274d03 100644
--- a/lldb/unittests/Host/SocketTest.cpp
+++ b/lldb/unittests/Host/SocketTest.cpp
@@ -85,6 +85,42 @@ TEST_P(SocketTest, DomainListenConnectAccept) {
std::unique_ptr<DomainSocket> socket_b_up;
CreateDomainConnectedSockets(Path, &socket_a_up, &socket_b_up);
}
+
+TEST_P(SocketTest, DomainMainLoopAccept) {
+ llvm::SmallString<64> Path;
+ std::error_code EC = llvm::sys::fs::createUniqueDirectory("DomainListenConnectAccept", Path);
+ ASSERT_FALSE(EC);
+ llvm::sys::path::append(Path, "test");
+
+ // Skip the test if the $TMPDIR is too long to hold a domain socket.
+ if (Path.size() > 107u)
+ return;
+
+ auto listen_socket_up = std::make_unique<DomainSocket>(
+ /*should_close=*/true, /*child_process_inherit=*/false);
+ Status error = listen_socket_up->Listen(Path, 5);
+ ASSERT_THAT_ERROR(error.ToError(), llvm::Succeeded());
+ ASSERT_TRUE(listen_socket_up->IsValid());
+
+ MainLoop loop;
+ std::unique_ptr<Socket> accepted_socket_up;
+ auto expected_handles = listen_socket_up->Accept(
+ loop, [&accepted_socket_up, &loop](std::unique_ptr<Socket> sock_up) {
+ accepted_socket_up = std::move(sock_up);
+ loop.RequestTermination();
+ });
+ ASSERT_THAT_EXPECTED(expected_handles, llvm::Succeeded());
+
+ auto connect_socket_up = std::make_unique<DomainSocket>(
+ /*should_close=*/true, /*child_process_inherit=*/false);
+ ASSERT_THAT_ERROR(connect_socket_up->Connect(Path).ToError(),
+ llvm::Succeeded());
+ ASSERT_TRUE(connect_socket_up->IsValid());
+
+ loop.Run();
+ ASSERT_TRUE(accepted_socket_up);
+ ASSERT_TRUE(accepted_socket_up->IsValid());
+}
#endif
TEST_P(SocketTest, TCPListen0ConnectAccept) {
@@ -109,9 +145,9 @@ TEST_P(SocketTest, TCPMainLoopAccept) {
ASSERT_TRUE(listen_socket_up->IsValid());
MainLoop loop;
- std::unique_ptr<TCPSocket> accepted_socket_up;
+ std::unique_ptr<Socket> accepted_socket_up;
auto expected_handles = listen_socket_up->Accept(
- loop, [&accepted_socket_up, &loop](std::unique_ptr<TCPSocket> sock_up) {
+ loop, [&accepted_socket_up, &loop](std::unique_ptr<Socket> sock_up) {
accepted_socket_up = std::move(sock_up);
loop.RequestTermination();
});
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
slydiman
approved these changes
Sep 11, 2024
| @@ -85,6 +85,43 @@ TEST_P(SocketTest, DomainListenConnectAccept) { | |||
| std::unique_ptr<DomainSocket> socket_b_up; | |||
| CreateDomainConnectedSockets(Path, &socket_a_up, &socket_b_up); | |||
| } | |||
|
|
|||
| TEST_P(SocketTest, DomainMainLoopAccept) { | |||
There was a problem hiding this comment.
Add #if LLDB_ENABLE_POSIX. Otherwise it will fail on Windows.
There was a problem hiding this comment.
It's already in a ifdef block, next to the other domain socket test. That's why you don't see an ifdef in the default view.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/6215 Here is the relevant piece of the build log for the reference |
slydiman
added a commit
to slydiman/llvm-project
that referenced
this pull request
Sep 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To go along with the existing TCPSocket implementation.