From: Remi Gacogne Date: Fri, 8 Apr 2022 15:17:00 +0000 (+0200) Subject: dnsdist: Refactoring of the SNMP code with channels X-Git-Tag: rec-5.0.0-alpha1~161^2~19 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=790a6a604eaca8cb40a1fe40ed82867394e0812b;p=thirdparty%2Fpdns.git dnsdist: Refactoring of the SNMP code with channels --- diff --git a/pdns/dnsdist-snmp.cc b/pdns/dnsdist-snmp.cc index d853a32d04..149acbc511 100644 --- a/pdns/dnsdist-snmp.cc +++ b/pdns/dnsdist-snmp.cc @@ -411,7 +411,7 @@ bool DNSDistSNMPAgent::sendBackendStatusChangeTrap(const DownstreamState& dss) backendStatus.c_str(), backendStatus.size()); - return sendTrap(d_trapPipe[1], varList); + return sendTrap(d_sender, varList); #else return true; #endif /* HAVE_NET_SNMP */ @@ -436,7 +436,7 @@ bool DNSDistSNMPAgent::sendCustomTrap(const std::string& reason) reason.c_str(), reason.size()); - return sendTrap(d_trapPipe[1], varList); + return sendTrap(d_sender, varList); #else return true; #endif /* HAVE_NET_SNMP */ @@ -542,7 +542,7 @@ bool DNSDistSNMPAgent::sendDNSTrap(const DNSQuestion& dq, const std::string& rea reason.c_str(), reason.size()); - return sendTrap(d_trapPipe[1], varList); + return sendTrap(d_sender, varList); #else return true; #endif /* HAVE_NET_SNMP */ diff --git a/pdns/dnsdistdist/channel.hh b/pdns/dnsdistdist/channel.hh index 5698fb7ba3..5c352870ed 100644 --- a/pdns/dnsdistdist/channel.hh +++ b/pdns/dnsdistdist/channel.hh @@ -34,7 +34,7 @@ namespace channel * * A sender can be used by several threads in a safe way. */ - template + template > class Sender { public: @@ -56,7 +56,7 @@ namespace channel * * \throw runtime_error if the channel is broken, for example if the other end has been closed. */ - bool send(std::unique_ptr&&) const; + bool send(std::unique_ptr&&) const; private: FDWrapper d_fd; @@ -67,7 +67,7 @@ namespace channel * * A receiver can be used by several threads in a safe way, but in that case spurious wake up might happen. */ - template + template > class Receiver { public: @@ -89,7 +89,8 @@ namespace channel * * \throw runtime_error if the channel is broken, for example if the other end has been closed. */ - std::optional> receive() const; + std::optional> receive() const; + std::optional> receive(D deleter) const; /** * \brief Get a descriptor that can be used with an I/O multiplexer to wait for an object to become available. @@ -112,8 +113,8 @@ namespace channel * * \throw runtime_error if the channel creation failed. */ - template - std::pair, Receiver> createObjectQueue(bool nonBlocking = true, size_t pipeBufferSize = 0); + template > + std::pair, Receiver> createObjectQueue(bool nonBlocking = true, size_t pipeBufferSize = 0); /** * The notifier's end of a channel used to communicate between threads. @@ -183,15 +184,19 @@ namespace channel */ std::pair createNotificationQueue(bool nonBlocking = true, size_t pipeBufferSize = 0); - template - bool Sender::send(std::unique_ptr&& object) const + template + bool Sender::send(std::unique_ptr&& object) const { - auto ptr = object.release(); + // we do not release right away because we might need the custom deleter later + auto ptr = object.get(); static_assert(sizeof(ptr) <= PIPE_BUF, "Writes up to PIPE_BUF are guaranted not to interleaved and to either fully succeed or fail"); ssize_t sent = write(d_fd.getHandle(), &ptr, sizeof(ptr)); - if (sent != sizeof(ptr)) { - delete ptr; + if (sent == sizeof(ptr)) { + // we cannot touch it anymore + object.release(); + } + else { if (errno == EAGAIN || errno == EWOULDBLOCK) { return false; } @@ -203,14 +208,37 @@ namespace channel return true; } - template - std::optional> Receiver::receive() const + template + std::optional> Receiver::receive() const + { + std::optional> result; + T* obj{nullptr}; + ssize_t got = read(d_fd.getHandle(), &obj, sizeof(obj)); + if (got == sizeof(obj)) { + return std::unique_ptr(obj); + } + else if (got == 0) { + throw std::runtime_error("EOF while reading from Channel receiver"); + } + else if (got == -1) { + if (errno == EAGAIN || errno == EINTR) { + return result; + } + throw std::runtime_error("Error while reading from Channel receiver: " + stringerror()); + } + else { + throw std::runtime_error("Partial read from Channel receiver"); + } + } + + template + std::optional> Receiver::receive(D deleter) const { - std::optional> result; + std::optional> result; T* obj{nullptr}; ssize_t got = read(d_fd.getHandle(), &obj, sizeof(obj)); if (got == sizeof(obj)) { - return std::unique_ptr(obj); + return std::unique_ptr(obj, deleter); } else if (got == 0) { throw std::runtime_error("EOF while reading from Channel receiver"); @@ -226,8 +254,8 @@ namespace channel } } - template - std::pair, Receiver> createObjectQueue(bool nonBlocking, size_t pipeBufferSize) + template + std::pair, Receiver> createObjectQueue(bool nonBlocking, size_t pipeBufferSize) { int fds[2] = {-1, -1}; if (pipe(fds) < 0) { @@ -251,7 +279,7 @@ namespace channel setPipeBufferSize(receiver.getHandle(), pipeBufferSize); } - return std::pair(Sender(std::move(sender)), Receiver(std::move(receiver))); + return std::pair(Sender(std::move(sender)), Receiver(std::move(receiver))); } } } diff --git a/pdns/recursordist/rec-snmp.cc b/pdns/recursordist/rec-snmp.cc index 8773a158f1..7a0fbd5f88 100644 --- a/pdns/recursordist/rec-snmp.cc +++ b/pdns/recursordist/rec-snmp.cc @@ -268,7 +268,7 @@ bool RecursorSNMPAgent::sendCustomTrap(const std::string& reason) reason.c_str(), reason.size()); - return sendTrap(d_trapPipe[1], varList); + return sendTrap(d_sender, varList); #endif /* HAVE_NET_SNMP */ return true; } diff --git a/pdns/snmp-agent.cc b/pdns/snmp-agent.cc index 18a088ea43..b70a5b280d 100644 --- a/pdns/snmp-agent.cc +++ b/pdns/snmp-agent.cc @@ -41,32 +41,31 @@ int SNMPAgent::setCounter64Value(netsnmp_request_info* request, return SNMP_ERR_NOERROR; } -bool SNMPAgent::sendTrap(int fd, +bool SNMPAgent::sendTrap(pdns::channel::Sender& sender, netsnmp_variable_list* varList) { - ssize_t written = write(fd, &varList, sizeof(varList)); - - if (written != sizeof(varList)) { - snmp_free_varbind(varList); + try { + auto obj = std::unique_ptr(varList, snmp_free_varbind); + return sender.send(std::move(obj)); + } + catch (...) { return false; } - return true; } void SNMPAgent::handleTrapsEvent() { - netsnmp_variable_list* varList = nullptr; - ssize_t got = 0; - - do { - got = read(d_trapPipe[0], &varList, sizeof(varList)); - - if (got == sizeof(varList)) { - send_v2trap(varList); - snmp_free_varbind(varList); + try { + while (true) { + auto obj = d_receiver.receive(snmp_free_varbind); + if (!obj) { + break; + } + send_v2trap(obj->get()); } } - while (got > 0); + catch (const std::exception& e) { + } } void SNMPAgent::handleSNMPQueryEvent(int fd) @@ -121,7 +120,7 @@ void SNMPAgent::worker() /* we want to be notified if a trap is waiting to be sent */ - mplexer->addReadFD(d_trapPipe[0], &handleTrapsCB, this); + mplexer->addReadFD(d_receiver.getDescriptor(), &handleTrapsCB, this); while(true) { netsnmp_large_fd_set_init(&fdset, FD_SETSIZE); @@ -187,20 +186,8 @@ SNMPAgent::SNMPAgent(const std::string& name, const std::string& daemonSocket) init_snmp(name.c_str()); - if (pipe(d_trapPipe) < 0) - unixDie("Creating pipe"); - - if (!setNonBlocking(d_trapPipe[0])) { - close(d_trapPipe[0]); - close(d_trapPipe[1]); - unixDie("Setting pipe non-blocking"); - } - - if (!setNonBlocking(d_trapPipe[1])) { - close(d_trapPipe[0]); - close(d_trapPipe[1]); - unixDie("Setting pipe non-blocking"); - } - + auto [sender, receiver] = pdns::channel::createObjectQueue(true); + d_sender = std::move(sender); + d_receiver = std::move(receiver); #endif /* HAVE_NET_SNMP */ } diff --git a/pdns/snmp-agent.hh b/pdns/snmp-agent.hh index e4ba13420d..f5df62ba74 100644 --- a/pdns/snmp-agent.hh +++ b/pdns/snmp-agent.hh @@ -16,6 +16,7 @@ #endif /* HAVE_NET_SNMP */ #include "mplexer.hh" +#include "channel.hh" class SNMPAgent { @@ -23,11 +24,6 @@ public: SNMPAgent(const std::string& name, const std::string& daemonSocket); virtual ~SNMPAgent() { -#ifdef HAVE_NET_SNMP - - close(d_trapPipe[0]); - close(d_trapPipe[1]); -#endif /* HAVE_NET_SNMP */ } void run() @@ -48,10 +44,11 @@ protected: static const oid snmpTrapOID[]; static const size_t snmpTrapOIDLen; - static bool sendTrap(int fd, + static bool sendTrap(pdns::channel::Sender& sender, netsnmp_variable_list* varList); - int d_trapPipe[2] = { -1, -1}; + pdns::channel::Sender d_sender; + pdns::channel::Receiver d_receiver; #endif /* HAVE_NET_SNMP */ private: void worker();