From cce416f263ab6519a0621c2d0af8adf90f20a351 Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Mon, 15 Jan 2024 15:44:31 +0100 Subject: [PATCH] dnsdist: Format and delint the XSK code --- pdns/dnsdist.cc | 1 - pdns/dnsdist.hh | 2 +- pdns/dnsdistdist/dnsdist-xsk.cc | 9 ++++---- pdns/xsk.cc | 41 ++++++++++++++++++++++++--------- pdns/xsk.hh | 18 +++++++-------- 5 files changed, 44 insertions(+), 27 deletions(-) diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc index 81ad234bfe..7bc864dbb5 100644 --- a/pdns/dnsdist.cc +++ b/pdns/dnsdist.cc @@ -3203,7 +3203,6 @@ int main(int argc, char** argv) g_hashperturb = dnsdist::getRandomValue(0xffffffff); #ifdef HAVE_XSK -#warning FIXME: we need to provide a way to clear the map from Lua, as well as a way to change the map path try { dnsdist::xsk::clearDestinationAddresses(); } diff --git a/pdns/dnsdist.hh b/pdns/dnsdist.hh index 254e64af61..1a92f25dab 100644 --- a/pdns/dnsdist.hh +++ b/pdns/dnsdist.hh @@ -987,7 +987,7 @@ public: void handleUDPTimeouts(); void reportTimeoutOrError(); void reportResponse(uint8_t rcode); - void submitHealthCheckResult(bool initial, bool newState); + void submitHealthCheckResult(bool initial, bool newResult); time_t getNextLazyHealthCheck(); uint16_t saveState(InternalQueryState&&); void restoreState(uint16_t id, InternalQueryState&&); diff --git a/pdns/dnsdistdist/dnsdist-xsk.cc b/pdns/dnsdistdist/dnsdist-xsk.cc index 4d1bec33cd..fbc0313ae7 100644 --- a/pdns/dnsdistdist/dnsdist-xsk.cc +++ b/pdns/dnsdistdist/dnsdist-xsk.cc @@ -150,7 +150,7 @@ void XskRouter(std::shared_ptr xsk) if ((fds.at(0).revents & POLLIN) != 0) { auto packets = xsk->recv(64, &failed); dnsdist::metrics::g_stats.nonCompliantQueries += failed; - for (auto &packet : packets) { + for (auto& packet : packets) { const auto dest = packet.getToAddr(); auto worker = xsk->getWorkerByDestination(dest); if (!worker) { @@ -172,7 +172,6 @@ void XskRouter(std::shared_ptr xsk) needNotify.clear(); ready--; } - const auto backup = ready; for (size_t fdIndex = 1; fdIndex < fds.size() && ready > 0; fdIndex++) { if ((fds.at(fdIndex).revents & POLLIN) != 0) { ready--; @@ -199,7 +198,6 @@ void XskRouter(std::shared_ptr xsk) xsk->recycle(4096); xsk->fillFq(); xsk->send(fillInTx); - ready = backup; } catch (...) { vinfolog("Exception in XSK router loop"); @@ -238,8 +236,9 @@ void XskClientThread(ClientState* clientState) } } -static std::string getDestinationMap(bool isV6) { - return !isV6 ? "/sys/fs/bpf/dnsdist/xsk-destinations-v4" : "/sys/fs/bpf/dnsdist/xsk-destinations-v6"; +static std::string getDestinationMap(bool isV6) +{ + return !isV6 ? "/sys/fs/bpf/dnsdist/xsk-destinations-v4" : "/sys/fs/bpf/dnsdist/xsk-destinations-v6"; } void addDestinationAddress(const ComboAddress& addr) diff --git a/pdns/xsk.cc b/pdns/xsk.cc index b656757a2d..cfa6912419 100644 --- a/pdns/xsk.cc +++ b/pdns/xsk.cc @@ -59,10 +59,17 @@ extern "C" #include "xsk.hh" #ifdef DEBUG_UMEM -namespace { +namespace +{ struct UmemEntryStatus { - enum class Status: uint8_t { Free, FillQueue, Received, TXQueue }; + enum class Status : uint8_t + { + Free, + FillQueue, + Received, + TXQueue + }; Status status{Status::Free}; }; @@ -144,7 +151,6 @@ XskSocket::XskSocket(size_t frameNum_, std::string ifName_, uint32_t queue_id, c uniqueEmptyFrameOffset.reserve(frameNum); { for (uint64_t i = 0; i < frameNum; i++) { - //uniqueEmptyFrameOffset.push_back(i * frameSize); uniqueEmptyFrameOffset.push_back(i * frameSize + XDP_PACKET_HEADROOM); #ifdef DEBUG_UMEM { @@ -261,11 +267,14 @@ void XskSocket::removeDestinationAddress(const std::string& mapPath, const Combo void XskSocket::fillFq(uint32_t fillSize) noexcept { { -#warning why are we collecting frames from unique into shared here, even though we need unique ones? + // if we have less than holdThreshold frames in the shared queue (which might be an issue + // when the XskWorker needs empty frames), move frames from the unique container into the + // shared one. This might not be optimal right now. auto frames = sharedEmptyFrameOffset->lock(); if (frames->size() < holdThreshold) { const auto moveSize = std::min(holdThreshold - frames->size(), uniqueEmptyFrameOffset.size()); if (moveSize > 0) { + // NOLINTNEXTLINE(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions) frames->insert(frames->end(), std::make_move_iterator(uniqueEmptyFrameOffset.end() - moveSize), std::make_move_iterator(uniqueEmptyFrameOffset.end())); uniqueEmptyFrameOffset.resize(uniqueEmptyFrameOffset.size() - moveSize); } @@ -304,7 +313,8 @@ int XskSocket::wait(int timeout) return packet.getFrameOffsetFrom(umem.bufBase); } -[[nodiscard]] int XskSocket::xskFd() const noexcept { +[[nodiscard]] int XskSocket::xskFd() const noexcept +{ return xsk_socket__fd(socket.get()); } @@ -359,7 +369,7 @@ std::vector XskSocket::recv(uint32_t recvSizeMax, uint32_t* failedCou for (; processed < recvSize; processed++) { try { const auto* desc = xsk_ring_cons__rx_desc(&rx, idx++); - // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) + // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast,performance-no-int-to-ptr) XskPacket packet = XskPacket(reinterpret_cast(desc->addr + baseAddr), desc->len, frameSize); #ifdef DEBUG_UMEM checkUmemIntegrity(__PRETTY_FUNCTION__, __LINE__, frameOffset(packet), {UmemEntryStatus::Status::Free, UmemEntryStatus::Status::FillQueue}, UmemEntryStatus::Status::Received); @@ -565,6 +575,7 @@ void XskPacket::setIPv6Header(const ipv6hdr& ipv6Header) noexcept void XskPacket::setUDPHeader(const udphdr& udpHeader) noexcept { assert(frameLength >= (sizeof(ethhdr) + (v6 ? sizeof(ipv6hdr) : sizeof(iphdr)) + sizeof(udpHeader))); + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-pointer-arithmetic) memcpy(frame + getL4HeaderOffset(), &udpHeader, sizeof(udpHeader)); } @@ -661,11 +672,14 @@ void XskPacket::changeDirectAndUpdateChecksum() noexcept { auto ethHeader = getEthernetHeader(); { - std::array tmp; + std::array tmp{}; static_assert(tmp.size() == sizeof(ethHeader.h_dest), "Size Error"); static_assert(tmp.size() == sizeof(ethHeader.h_source), "Size Error"); + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-array-to-pointer-decay) memcpy(tmp.data(), ethHeader.h_dest, tmp.size()); + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-array-to-pointer-decay) memcpy(ethHeader.h_dest, ethHeader.h_source, tmp.size()); + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-array-to-pointer-decay) memcpy(ethHeader.h_source, tmp.data(), tmp.size()); } if (ethHeader.h_proto == htons(ETH_P_IPV6)) { @@ -841,7 +855,9 @@ const void* XskPacket::getPayloadData() const void XskPacket::setAddr(const ComboAddress& from_, MACAddr fromMAC, const ComboAddress& to_, MACAddr toMAC) noexcept { auto ethHeader = getEthernetHeader(); + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-array-to-pointer-decay) memcpy(ethHeader.h_dest, toMAC.data(), toMAC.size()); + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-array-to-pointer-decay) memcpy(ethHeader.h_source, fromMAC.data(), fromMAC.size()); setEthernetHeader(ethHeader); to = to_; @@ -993,7 +1009,7 @@ void XskPacket::rewrite() noexcept uint32_t words[3]; }; }; - struct ipv4_pseudo_header_t pseudo_header; + struct ipv4_pseudo_header_t pseudo_header{}; static_assert(sizeof(pseudo_header) == 12, "IPv4 pseudo-header size is incorrect"); /* Fill in the pseudo-header. */ @@ -1027,7 +1043,7 @@ void XskPacket::rewrite() noexcept uint32_t words[10]; }; }; - struct ipv6_pseudo_header_t pseudo_header; + struct ipv6_pseudo_header_t pseudo_header{}; static_assert(sizeof(pseudo_header) == 40, "IPv6 pseudo-header size is incorrect"); /* Fill in the pseudo-header. */ @@ -1070,7 +1086,7 @@ int XskWorker::createEventfd() void XskWorker::waitForXskSocket() const noexcept { - uint64_t x = read(workerWaker, &x, sizeof(x)); + uint64_t value = read(workerWaker, &value, sizeof(value)); } void XskWorker::notifyXskSocket() const @@ -1109,7 +1125,7 @@ uint64_t XskWorker::frameOffset(const XskPacket& packet) const noexcept return packet.getFrameOffsetFrom(umemBufBase); } -void XskWorker::notifyWorker() noexcept +void XskWorker::notifyWorker() const { notify(workerWaker); } @@ -1126,11 +1142,13 @@ void XskSocket::getMACFromIfName() throw std::runtime_error("Unable to get MAC address for interface " + ifName + ": name too long"); } + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-array-to-pointer-decay) strncpy(ifr.ifr_name, ifName.c_str(), ifName.length() + 1); if (ioctl(desc.getHandle(), SIOCGIFHWADDR, &ifr) < 0 || ifr.ifr_hwaddr.sa_family != ARPHRD_ETHER) { throw std::runtime_error("Error getting MAC address for interface " + ifName); } static_assert(sizeof(ifr.ifr_hwaddr.sa_data) >= std::tuple_size{}, "The size of an ARPHRD_ETHER MAC address is smaller than expected"); + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-array-to-pointer-decay) memcpy(source.data(), ifr.ifr_hwaddr.sa_data, source.size()); } @@ -1175,6 +1193,7 @@ void XskWorker::fillUniqueEmptyOffset() auto frames = sharedEmptyFrameOffset->lock(); const auto moveSize = std::min(static_cast(32), frames->size()); if (moveSize > 0) { + // NOLINTNEXTLINE(bugprone-narrowing-conversions,cppcoreguidelines-narrowing-conversions) uniqueEmptyFrameOffset.insert(uniqueEmptyFrameOffset.end(), std::make_move_iterator(frames->end() - moveSize), std::make_move_iterator(frames->end())); frames->resize(frames->size() - moveSize); } diff --git a/pdns/xsk.hh b/pdns/xsk.hh index a2cc6f036b..4ea36e38b3 100644 --- a/pdns/xsk.hh +++ b/pdns/xsk.hh @@ -29,12 +29,12 @@ #include #include #include +#include #include #include #include #include #include -//#include #include #include #include @@ -120,7 +120,7 @@ class XskSocket static constexpr uint32_t txCapacity = XSK_RING_PROD__DEFAULT_NUM_DESCS * 2; constexpr static bool isPowOfTwo(uint32_t value) noexcept; - [[nodiscard]] static int timeDifference(const timespec& t1, const timespec& t2) noexcept; + [[nodiscard]] static int timeDifference(const timespec& lhs, const timespec& rhs) noexcept; [[nodiscard]] uint64_t frameOffset(const XskPacket& packet) const noexcept; [[nodiscard]] int firstTimeout(); @@ -224,7 +224,7 @@ private: [[nodiscard]] __be16 tcp_udp_v4_checksum(const struct iphdr*) const noexcept; // You must set l4Header.check = 0 before calling this method [[nodiscard]] __be16 tcp_udp_v6_checksum(const struct ipv6hdr*) const noexcept; - /* offset of the L4 (udphdr) header (after ethhdr and iphdr/ipv6hdr) */ + /* offset of the L4 (udphdr) header (after ethhdr and iphdr/ipv6hdr) */ [[nodiscard]] size_t getL4HeaderOffset() const noexcept; /* offset of the data after the UDP header */ [[nodiscard]] size_t getDataOffset() const noexcept; @@ -271,7 +271,7 @@ public: return frame - base; } }; -bool operator<(const XskPacket& s1, const XskPacket& s2) noexcept; +bool operator<(const XskPacket& lhs, const XskPacket& rhs) noexcept; /* g++ defines __SANITIZE_THREAD__ clang++ supports the nice __has_feature(thread_sanitizer), @@ -288,9 +288,9 @@ bool operator<(const XskPacket& s1, const XskPacket& s2) noexcept; class XskWorker { #if defined(__SANITIZE_THREAD__) - using XskPacketRing = LockGuarded>>; + using XskPacketRing = LockGuarded>>; #else - using XskPacketRing = boost::lockfree::spsc_queue>; + using XskPacketRing = boost::lockfree::spsc_queue>; #endif public: @@ -299,7 +299,7 @@ public: // queue of packets processed by this worker (to be sent, or discarded) XskPacketRing outgoingPacketsQueue; - uint8_t* umemBufBase; + uint8_t* umemBufBase{nullptr}; // list of frames that are shared with the XskRouter std::shared_ptr>> sharedEmptyFrameOffset; // list of frames that we own, used to generate new packets (health-check) @@ -310,13 +310,13 @@ public: XskWorker(); static int createEventfd(); - static void notify(int fd); + static void notify(int desc); static std::shared_ptr create(); void pushToProcessingQueue(XskPacket& packet); void pushToSendQueue(XskPacket& packet); void markAsFree(const XskPacket& packet); // notify worker that at least one packet is available for processing - void notifyWorker() noexcept; + void notifyWorker() const; // notify the router that packets are ready to be sent void notifyXskSocket() const; void waitForXskSocket() const noexcept; -- 2.47.2