From: Otto <otto.moerbeek@open-xchange.com>
Date: Wed, 15 Sep 2021 14:32:04 +0000 (+0200)
Subject: Process review comments, most importantly a simplification of the retry logic
X-Git-Tag: rec-4.6.0-alpha1~2^2~2
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=050b64acb87845df9978e30cccb41565dffaa829;p=thirdparty%2Fpdns.git

Process review comments, most importantly a simplification of the retry logic
---

diff --git a/pdns/lwres.cc b/pdns/lwres.cc
index cc15c18171..77a7f9c296 100644
--- a/pdns/lwres.cc
+++ b/pdns/lwres.cc
@@ -236,37 +236,34 @@ static bool tcpconnect(const struct timeval& now, const ComboAddress& ip, TCPOut
 {
   dnsOverTLS = SyncRes::s_dot_to_port_853 && ip.getPort() == 853;
 
+  connection = t_tcp_manager.get(ip);
+  if (connection.d_handler) {
+    return false;
+  }
 
-  while (true) {
-    connection = t_tcp_manager.get(ip);
-    if (connection.d_handler) {
-      return false;
-    }
-
-    const struct timeval timeout{ g_networkTimeoutMsec / 1000, static_cast<suseconds_t>(g_networkTimeoutMsec) % 1000 * 1000};
-    Socket s(ip.sin4.sin_family, SOCK_STREAM);
-    s.setNonBlocking();
-    ComboAddress localip = pdns::getQueryLocalAddress(ip.sin4.sin_family, 0);
-    s.bind(localip);
-
-    std::shared_ptr<TLSCtx> tlsCtx{nullptr};
-    if (dnsOverTLS) {
-      TLSContextParameters tlsParams;
-      tlsParams.d_provider = "openssl";
-      tlsParams.d_validateCertificates = false;
-      //tlsParams.d_caStore = caaStore;
-      tlsCtx = getTLSContext(tlsParams);
-      if (tlsCtx == nullptr) {
-        g_log << Logger::Error << "DoT to " << ip << " requested but not available" << endl;
-        dnsOverTLS = false;
-      }
+  const struct timeval timeout{ g_networkTimeoutMsec / 1000, static_cast<suseconds_t>(g_networkTimeoutMsec) % 1000 * 1000};
+  Socket s(ip.sin4.sin_family, SOCK_STREAM);
+  s.setNonBlocking();
+  ComboAddress localip = pdns::getQueryLocalAddress(ip.sin4.sin_family, 0);
+  s.bind(localip);
+
+  std::shared_ptr<TLSCtx> tlsCtx{nullptr};
+  if (dnsOverTLS) {
+    TLSContextParameters tlsParams;
+    tlsParams.d_provider = "openssl";
+    tlsParams.d_validateCertificates = false;
+    // tlsParams.d_caStore
+    tlsCtx = getTLSContext(tlsParams);
+    if (tlsCtx == nullptr) {
+      g_log << Logger::Error << "DoT to " << ip << " requested but not available" << endl;
+      dnsOverTLS = false;
     }
-    connection.d_handler = std::make_shared<TCPIOHandler>("", s.releaseHandle(), timeout, tlsCtx, now.tv_sec);
-    // Returned state ignored
-    // This can throw an excepion, retry will need to happen at higher level
-    connection.d_handler->tryConnect(SyncRes::s_tcp_fast_open_connect, ip);
-    return true;
   }
+  connection.d_handler = std::make_shared<TCPIOHandler>("", s.releaseHandle(), timeout, tlsCtx, now.tv_sec);
+  // Returned state ignored
+  // This can throw an exception, retry will need to happen at higher level
+  connection.d_handler->tryConnect(SyncRes::s_tcp_fast_open_connect, ip);
+  return true;
 }
 
 static LWResult::Result tcpsendrecv(const ComboAddress& ip, TCPOutConnectionManager::Connection& connection,
@@ -275,7 +272,6 @@ static LWResult::Result tcpsendrecv(const ComboAddress& ip, TCPOutConnectionMana
   socklen_t slen = ip.getSocklen();
   uint16_t tlen = htons(vpacket.size());
   const char *lenP = reinterpret_cast<const char*>(&tlen);
-  const char *msgP = reinterpret_cast<const char*>(&*vpacket.begin());
 
   localip.sin4.sin_family = ip.sin4.sin_family;
   getsockname(connection.d_handler->getDescriptor(), reinterpret_cast<sockaddr*>(&localip), &slen);
@@ -283,7 +279,7 @@ static LWResult::Result tcpsendrecv(const ComboAddress& ip, TCPOutConnectionMana
   PacketBuffer packet;
   packet.reserve(2 + vpacket.size());
   packet.insert(packet.end(), lenP, lenP + 2);
-  packet.insert(packet.end(), msgP, msgP + vpacket.size());
+  packet.insert(packet.end(), vpacket.begin(), vpacket.end());
 
   LWResult::Result ret = asendtcp(packet, connection.d_handler);
   if (ret != LWResult::Result::Success) {
@@ -567,9 +563,6 @@ LWResult::Result asyncresolve(const ComboAddress& ip, const DNSName& domain, int
   auto ret = asyncresolve(ip, domain, type, doTCP, sendRDQuery, EDNS0Level, now, srcmask, context, outgoingLoggers, fstrmLoggers, exportTypes, lwr, chained, connection);
 
   if (doTCP) {
-    if (!lwr->d_validpacket) {
-      ret = asyncresolve(ip, domain, type, doTCP, sendRDQuery, EDNS0Level, now, srcmask, context, outgoingLoggers, fstrmLoggers, exportTypes, lwr, chained, connection);
-    }
     if (connection.d_handler && lwr->d_validpacket) {
       t_tcp_manager.store(*now, ip, std::move(connection));
     }
diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc
index 9b0af4ef0c..385bd7a8a6 100644
--- a/pdns/pdns_recursor.cc
+++ b/pdns/pdns_recursor.cc
@@ -4506,7 +4506,7 @@ static void checkOrFixFDS()
 {
   unsigned int availFDs=getFilenumLimit(); 
   unsigned int wantFDs = g_maxMThreads * g_numWorkerThreads +25; // even healthier margin then before
-  wantFDs += g_numWorkerThreads * TCPOutConnectionManager::maxIdlePerThread;
+  wantFDs += g_numWorkerThreads * TCPOutConnectionManager::s_maxIdlePerThread;
 
   if(wantFDs > availFDs) {
     unsigned int hardlimit= getFilenumLimit(true);
@@ -4515,7 +4515,7 @@ static void checkOrFixFDS()
       g_log<<Logger::Warning<<"Raised soft limit on number of filedescriptors to "<<wantFDs<<" to match max-mthreads and threads settings"<<endl;
     }
     else {
-      int newval = (hardlimit - 25 - TCPOutConnectionManager::maxIdlePerThread) / g_numWorkerThreads;
+      int newval = (hardlimit - 25 - TCPOutConnectionManager::s_maxIdlePerThread) / g_numWorkerThreads;
       g_log<<Logger::Warning<<"Insufficient number of filedescriptors available for max-mthreads*threads setting! ("<<hardlimit<<" < "<<wantFDs<<"), reducing max-mthreads to "<<newval<<endl;
       g_maxMThreads = newval;
       setFilenumLimit(hardlimit);
@@ -5066,10 +5066,10 @@ static int serviceMain(int argc, char*argv[])
   }
 
   int64_t millis = ::arg().asNum("tcp-out-max-idle-ms");
-  TCPOutConnectionManager::maxIdleTime = timeval{millis / 1000, (millis % 1000) * 1000 };
-  TCPOutConnectionManager::maxIdlePerAuth = ::arg().asNum("tcp-out-max-idle-per-auth");
-  TCPOutConnectionManager::maxQueries = ::arg().asNum("tcp-out-max-queries");
-  TCPOutConnectionManager::maxIdlePerThread = ::arg().asNum("tcp-out-max-idle-per-thread");
+  TCPOutConnectionManager::s_maxIdleTime = timeval{millis / 1000, (millis % 1000) * 1000 };
+  TCPOutConnectionManager::s_maxIdlePerAuth = ::arg().asNum("tcp-out-max-idle-per-auth");
+  TCPOutConnectionManager::s_maxQueries = ::arg().asNum("tcp-out-max-queries");
+  TCPOutConnectionManager::s_maxIdlePerThread = ::arg().asNum("tcp-out-max-idle-per-thread");
 
   g_gettagNeedsEDNSOptions = ::arg().mustDo("gettag-needs-edns-options");
 
diff --git a/pdns/recursordist/rec-tcpout.cc b/pdns/recursordist/rec-tcpout.cc
index f9e7a6942f..c4b2e8560a 100644
--- a/pdns/recursordist/rec-tcpout.cc
+++ b/pdns/recursordist/rec-tcpout.cc
@@ -28,21 +28,21 @@
 
 #include "syncres.hh"
 
-timeval TCPOutConnectionManager::maxIdleTime;
-size_t TCPOutConnectionManager::maxQueries;
-size_t TCPOutConnectionManager::maxIdlePerAuth;
-size_t TCPOutConnectionManager::maxIdlePerThread;
+timeval TCPOutConnectionManager::s_maxIdleTime;
+size_t TCPOutConnectionManager::s_maxQueries;
+size_t TCPOutConnectionManager::s_maxIdlePerAuth;
+size_t TCPOutConnectionManager::s_maxIdlePerThread;
 
 void TCPOutConnectionManager::cleanup(const struct timeval& now)
 {
-  if (maxIdleTime.tv_sec == 0 && maxIdleTime.tv_usec == 0) {
+  if (s_maxIdleTime.tv_sec == 0 && s_maxIdleTime.tv_usec == 0) {
     // no maximum idle time
     return;
   }
 
   for (auto it = d_idle_connections.begin(); it != d_idle_connections.end();) {
     timeval idle = now - it->second.d_last_used;
-    if (maxIdleTime < idle) {
+    if (s_maxIdleTime < idle) {
       it = d_idle_connections.erase(it);
     }
     else {
@@ -53,19 +53,19 @@ void TCPOutConnectionManager::cleanup(const struct timeval& now)
 
 void TCPOutConnectionManager::store(const struct timeval& now, const ComboAddress& ip, Connection&& connection)
 {
-  if (d_idle_connections.size() >= maxIdlePerThread || d_idle_connections.count(ip) >= maxIdlePerAuth) {
+  if (d_idle_connections.size() >= s_maxIdlePerThread || d_idle_connections.count(ip) >= s_maxIdlePerAuth) {
     cleanup(now);
   }
 
-  if (d_idle_connections.size() >= maxIdlePerThread) {
+  if (d_idle_connections.size() >= s_maxIdlePerThread) {
     return;
   }
-  if (d_idle_connections.count(ip) >= maxIdlePerAuth) {
+  if (d_idle_connections.count(ip) >= s_maxIdlePerAuth) {
     return;
   }
 
   ++connection.d_numqueries;
-  if (maxQueries > 0 && connection.d_numqueries > maxQueries) {
+  if (s_maxQueries > 0 && connection.d_numqueries > s_maxQueries) {
     return;
   }
   gettimeofday(&connection.d_last_used, nullptr);
diff --git a/pdns/recursordist/rec-tcpout.hh b/pdns/recursordist/rec-tcpout.hh
index 91f67ae3a3..9c2cfa4d51 100644
--- a/pdns/recursordist/rec-tcpout.hh
+++ b/pdns/recursordist/rec-tcpout.hh
@@ -29,13 +29,13 @@ class TCPOutConnectionManager
 {
 public:
   // Max idle time for a connection, 0 is no timeout
-  static struct timeval maxIdleTime;
+  static struct timeval s_maxIdleTime;
   // Per thread maximum of idle connections for a specific destination, 0 means no idle connections will be kept open
-  static size_t maxIdlePerAuth;
+  static size_t s_maxIdlePerAuth;
   // Max total number of queries to handle per connection, 0 is no max
-  static size_t maxQueries;
+  static size_t s_maxQueries;
   // Per thread max # of idle connections, 0 means no idle connections will be kept open
-  static size_t maxIdlePerThread;
+  static size_t s_maxIdlePerThread;
 
   struct Connection
   {