From b339bfe01678c5a28b8c413ec11fa134eff0bf11 Mon Sep 17 00:00:00 2001 From: Nav Date: Mon, 28 Mar 2022 01:04:14 +0100 Subject: [PATCH] Using new RAII epoll instance wrapper in GDB server and Connection class --- src/DebugServers/GdbRsp/Connection.cpp | 100 ++++++------------ src/DebugServers/GdbRsp/Connection.hpp | 22 +++- src/DebugServers/GdbRsp/DebugSession.cpp | 4 +- src/DebugServers/GdbRsp/DebugSession.hpp | 2 +- src/DebugServers/GdbRsp/GdbRspDebugServer.cpp | 54 +++------- src/DebugServers/GdbRsp/GdbRspDebugServer.hpp | 9 +- 6 files changed, 77 insertions(+), 114 deletions(-) diff --git a/src/DebugServers/GdbRsp/Connection.cpp b/src/DebugServers/GdbRsp/Connection.cpp index cc7d1abf..0465ba61 100644 --- a/src/DebugServers/GdbRsp/Connection.cpp +++ b/src/DebugServers/GdbRsp/Connection.cpp @@ -38,19 +38,7 @@ namespace Bloom::DebugServers::Gdb fcntl(this->socketFileDescriptor, F_GETFL, 0) | O_NONBLOCK ); - // Create event FD - this->eventFileDescriptor = ::epoll_create(2); - struct epoll_event event = {}; - event.events = EPOLLIN; - event.data.fd = this->socketFileDescriptor; - - if (::epoll_ctl(this->eventFileDescriptor, EPOLL_CTL_ADD, this->socketFileDescriptor, &event) != 0) { - throw Exception( - "Failed to create event FD for GDB client connection - could not add client connection " - "socket FD to epoll FD" - ); - } - + this->epollInstance.addEntry(this->socketFileDescriptor, static_cast(EpollEvent::READ_READY)); this->enableReadInterrupts(); } @@ -157,7 +145,11 @@ namespace Bloom::DebugServers::Gdb } while (this->readSingleByte(false).value_or(0) != '+'); } - std::vector Connection::read(size_t bytes, bool interruptible, std::optional msTimeout) { + std::vector Connection::read( + size_t bytes, + bool interruptible, + std::optional timeout + ) { auto output = std::vector(); constexpr size_t bufferSize = 1024; std::array buffer = {}; @@ -177,53 +169,42 @@ namespace Bloom::DebugServers::Gdb this->disableReadInterrupts(); } - std::array events = {}; + const auto eventFileDescriptor = this->epollInstance.waitForEvent(timeout); - int eventCount = ::epoll_wait( - this->eventFileDescriptor, - events.data(), - 1, - msTimeout.value_or(-1) - ); + if ( + !eventFileDescriptor.has_value() + || eventFileDescriptor.value() == this->interruptEventNotifier.getFileDescriptor() + ) { + // Interrupted + this->interruptEventNotifier.clear(); + throw DebugServerInterrupted(); + } - if (eventCount > 0) { - for (size_t i = 0; i < eventCount; i++) { - auto fileDescriptor = events.at(i).data.fd; + size_t bytesToRead = (bytes > bufferSize || bytes == 0) ? bufferSize : bytes; + while ( + bytesToRead > 0 + && (bytesRead = ::read(this->socketFileDescriptor, buffer.data(), bytesToRead)) > 0 + ) { + output.insert(output.end(), buffer.begin(), buffer.begin() + bytesRead); - if (fileDescriptor == this->interruptEventNotifier.getFileDescriptor()) { - // Interrupted - this->interruptEventNotifier.clear(); - throw DebugServerInterrupted(); - } + if (bytesRead < bytesToRead) { + // No more data available + break; } - size_t bytesToRead = (bytes > bufferSize || bytes == 0) ? bufferSize : bytes; - while ( - bytesToRead > 0 - && (bytesRead = ::read(this->socketFileDescriptor, buffer.data(), bytesToRead)) > 0 - ) { - output.insert(output.end(), buffer.begin(), buffer.begin() + bytesRead); + bytesToRead = ((bytes - output.size()) > bufferSize || bytes == 0) ? bufferSize : (bytes - output.size()); + } - if (bytesRead < bytesToRead) { - // No more data available - break; - } - - bytesToRead = - ((bytes - output.size()) > bufferSize || bytes == 0) ? bufferSize : (bytes - output.size()); - } - - if (output.empty()) { - // EOF means the client has disconnected - throw ClientDisconnected(); - } + if (output.empty()) { + // EOF means the client has disconnected + throw ClientDisconnected(); } return output; } std::optional Connection::readSingleByte(bool interruptible) { - auto bytes = this->read(1, interruptible, 300); + auto bytes = this->read(1, interruptible, std::chrono::milliseconds(300)); if (!bytes.empty()) { return bytes.front(); @@ -247,27 +228,16 @@ namespace Bloom::DebugServers::Gdb } void Connection::disableReadInterrupts() { - if (::epoll_ctl( - this->eventFileDescriptor, - EPOLL_CTL_DEL, - this->interruptEventNotifier.getFileDescriptor(), - NULL) != 0 - ) { - throw Exception("Failed to disable GDB client connection read interrupts - epoll_ctl failed"); - } + this->epollInstance.removeEntry(this->interruptEventNotifier.getFileDescriptor()); this->readInterruptEnabled = false; } void Connection::enableReadInterrupts() { - auto interruptFileDescriptor = this->interruptEventNotifier.getFileDescriptor(); - struct epoll_event event = {}; - event.events = EPOLLIN; - event.data.fd = interruptFileDescriptor; - - if (::epoll_ctl(this->eventFileDescriptor, EPOLL_CTL_ADD, interruptFileDescriptor, &event) != 0) { - throw Exception("Failed to enable GDB client connection read interrupts - epoll_ctl failed"); - } + this->epollInstance.addEntry( + this->interruptEventNotifier.getFileDescriptor(), + static_cast(EpollEvent::READ_READY) + ); this->readInterruptEnabled = true; } diff --git a/src/DebugServers/GdbRsp/Connection.hpp b/src/DebugServers/GdbRsp/Connection.hpp index 48d81e53..17c9f001 100644 --- a/src/DebugServers/GdbRsp/Connection.hpp +++ b/src/DebugServers/GdbRsp/Connection.hpp @@ -8,8 +8,10 @@ #include #include #include +#include #include "src/Helpers/EventNotifier.hpp" +#include "src/Helpers/EpollInstance.hpp" #include "src/DebugServers/GdbRsp/Packet.hpp" #include "src/DebugServers/GdbRsp/ResponsePackets/ResponsePacket.hpp" @@ -22,12 +24,21 @@ namespace Bloom::DebugServers::Gdb class Connection { public: - Connection() = delete; - explicit Connection(EventNotifier& interruptEventNotifier) : interruptEventNotifier(interruptEventNotifier) {}; + Connection() = delete; + Connection(const Connection&) = delete; + Connection(Connection&& other) noexcept + : interruptEventNotifier(other.interruptEventNotifier) + , socketFileDescriptor(other.socketFileDescriptor) + , epollInstance(std::move(other.epollInstance)) + , readInterruptEnabled(other.readInterruptEnabled) + { + other.socketFileDescriptor = -1; + }; + /** * Accepts a connection on serverSocketFileDescriptor. * @@ -75,7 +86,8 @@ namespace Bloom::DebugServers::Gdb private: int socketFileDescriptor = -1; - int eventFileDescriptor = -1; + + EpollInstance epollInstance = EpollInstance(); struct sockaddr_in socketAddress = {}; int maxPacketSize = 1024; @@ -98,7 +110,7 @@ namespace Bloom::DebugServers::Gdb * the read (via means of this->interruptEventNotifier). This flag has no effect if this->readInterruptEnabled * is false. * - * @param msTimeout + * @param timeout * The timeout in milliseconds. If not supplied, no timeout will be applied. * * @return @@ -106,7 +118,7 @@ namespace Bloom::DebugServers::Gdb std::vector read( std::size_t bytes = 0, bool interruptible = true, - std::optional msTimeout = std::nullopt + std::optional timeout = std::nullopt ); /** diff --git a/src/DebugServers/GdbRsp/DebugSession.cpp b/src/DebugServers/GdbRsp/DebugSession.cpp index 8a6fffc0..9fc5b5de 100644 --- a/src/DebugServers/GdbRsp/DebugSession.cpp +++ b/src/DebugServers/GdbRsp/DebugSession.cpp @@ -4,8 +4,8 @@ namespace Bloom::DebugServers::Gdb { - DebugSession::DebugSession(const Connection& connection, const TargetDescriptor& targetDescriptor) - : connection(connection) + DebugSession::DebugSession(Connection&& connection, const TargetDescriptor& targetDescriptor) + : connection(std::move(connection)) , targetDescriptor(targetDescriptor) {} diff --git a/src/DebugServers/GdbRsp/DebugSession.hpp b/src/DebugServers/GdbRsp/DebugSession.hpp index 41f7ed2d..a4d74c3a 100644 --- a/src/DebugServers/GdbRsp/DebugSession.hpp +++ b/src/DebugServers/GdbRsp/DebugSession.hpp @@ -20,7 +20,7 @@ namespace Bloom::DebugServers::Gdb */ bool waitingForBreak = false; - DebugSession(const Connection& connection, const TargetDescriptor& targetDescriptor); + DebugSession(Connection&& connection, const TargetDescriptor& targetDescriptor); void terminate(); }; diff --git a/src/DebugServers/GdbRsp/GdbRspDebugServer.cpp b/src/DebugServers/GdbRsp/GdbRspDebugServer.cpp index 8c97420a..fb60c87e 100644 --- a/src/DebugServers/GdbRsp/GdbRspDebugServer.cpp +++ b/src/DebugServers/GdbRsp/GdbRspDebugServer.cpp @@ -92,22 +92,15 @@ namespace Bloom::DebugServers::Gdb this->serverSocketFileDescriptor = socketFileDescriptor; - this->eventFileDescriptor = ::epoll_create(2); - struct epoll_event event = {}; - event.events = EPOLLIN; - event.data.fd = this->serverSocketFileDescriptor; + this->epollInstance.addEntry( + this->serverSocketFileDescriptor, + static_cast(EpollEvent::READ_READY) + ); - if (::epoll_ctl(this->eventFileDescriptor, EPOLL_CTL_ADD, this->serverSocketFileDescriptor, &event) != 0) { - throw Exception("Failed epoll_ctl server socket"); - } - - const auto interruptFileDescriptor = this->interruptEventNotifier->getFileDescriptor(); - event.events = EPOLLIN; - event.data.fd = interruptFileDescriptor; - - if (::epoll_ctl(this->eventFileDescriptor, EPOLL_CTL_ADD, interruptFileDescriptor, &event) != 0) { - throw Exception("Failed epoll_ctl interrupt event fd"); - } + this->epollInstance.addEntry( + this->interruptEventNotifier->getFileDescriptor(), + static_cast(EpollEvent::READ_READY) + ); Logger::info("GDB RSP address: " + this->debugServerConfig->listeningAddress); Logger::info("GDB RSP port: " + std::to_string(this->debugServerConfig->listeningPortNumber)); @@ -145,7 +138,7 @@ namespace Bloom::DebugServers::Gdb Logger::info("Accepted GDP RSP connection from " + connection->getIpAddress()); this->activeDebugSession.emplace( - DebugSession(connection.value(), this->getGdbTargetDescriptor()) + DebugSession(std::move(connection.value()), this->getGdbTargetDescriptor()) ); EventManager::triggerEvent(std::make_shared()); @@ -203,30 +196,17 @@ namespace Bloom::DebugServers::Gdb throw Exception("Failed to listen on server socket"); } - constexpr int maxEvents = 5; - std::array events = {}; - int eventCount = ::epoll_wait( - this->eventFileDescriptor, - events.data(), - maxEvents, - -1 - ); + const auto eventFileDescriptor = this->epollInstance.waitForEvent(); - if (eventCount > 0) { - for (size_t i = 0; i < eventCount; i++) { - auto fileDescriptor = events.at(i).data.fd; - - if (fileDescriptor == this->interruptEventNotifier->getFileDescriptor()) { - // Interrupted - this->interruptEventNotifier->clear(); - return std::nullopt; - } - } - - return Connection(*(this->interruptEventNotifier)); + if ( + !eventFileDescriptor.has_value() + || eventFileDescriptor.value() == this->interruptEventNotifier->getFileDescriptor() + ) { + this->interruptEventNotifier->clear(); + return std::nullopt; } - return std::nullopt; + return std::make_optional(*(this->interruptEventNotifier)); } std::unique_ptr GdbRspDebugServer::waitForCommandPacket() { diff --git a/src/DebugServers/GdbRsp/GdbRspDebugServer.hpp b/src/DebugServers/GdbRsp/GdbRspDebugServer.hpp index 5e0407ec..11e6e5f1 100644 --- a/src/DebugServers/GdbRsp/GdbRspDebugServer.hpp +++ b/src/DebugServers/GdbRsp/GdbRspDebugServer.hpp @@ -11,6 +11,7 @@ #include "GdbDebugServerConfig.hpp" #include "src/EventManager/EventListener.hpp" +#include "src/Helpers/EpollInstance.hpp" #include "src/TargetController/TargetControllerConsole.hpp" #include "Connection.hpp" @@ -88,15 +89,15 @@ namespace Bloom::DebugServers::Gdb int serverSocketFileDescriptor = -1; /** - * We don't listen on the this->serverSocketFileDescriptor directly. Instead, we add it to an epoll set, along - * with the this->interruptEventNotifier FD. This allows us to interrupt any blocking socket IO calls when - * we have other things to do. + * We don't listen on the this->serverSocketFileDescriptor directly. Instead, we use an EpollInstance to + * monitor both this->serverSocketFileDescriptor and this->interruptEventNotifier. This allows us to interrupt + * any blocking socket IO calls when EventNotifier::notify() is called on this->interruptEventNotifier. * * See GdbRspDebugServer::init() * See DebugServer::interruptEventNotifier * See EventNotifier */ - int eventFileDescriptor = -1; + EpollInstance epollInstance = EpollInstance(); /** * SO_REUSEADDR option value for listening socket.