Process review comments from rgacogne

diff --git a/pdns/logger.cc b/pdns/logger.cc
index 0711b971b07c491ab19a0abf1792da9ab5946286..00d94906e073b8ebba581b2b7c9d1600422faf31 100644 (file)
--- a/pdns/logger.cc
+++ b/pdns/logger.cc
@@ -203,7 +203,6 @@ Logger& Logger::operator<<(const ComboAddress& ca)
   return *this;
 }
 
-#ifdef RECURSOR
 void addTraceTS(const timeval& start, ostringstream& str)
 {
   const auto& content = str.str();
@@ -215,4 +214,3 @@ void addTraceTS(const timeval& start, ostringstream& str)
     str << diff << ' ';
   }
 }
-#endif

diff --git a/pdns/logger.hh b/pdns/logger.hh
index 5e2f45dea55b47d1d2d560146873316e0ad3da1a..dbe7f278b2da5db1bdac9f4deb290b804e7fcf6c 100644 (file)
--- a/pdns/logger.hh
+++ b/pdns/logger.hh
@@ -183,11 +183,6 @@ struct LogVariant
 
 using OptLog = std::optional<LogVariant>;
 
-#ifndef RECURSOR
-// Originally there was a flag but it was never set from !RECURSOR
-#define VLOG(log, x) VLOG only works in recursor
-#else
-
 void addTraceTS(const timeval& start, ostringstream& str);
 
 #define VLOG(log, x)                                                       \
@@ -211,5 +206,3 @@ void addTraceTS(const timeval& start, ostringstream& str);
       *std::get<ostringstream*>((log)->v) << x;                      \
     }                                                                \
   }
-
-#endif

diff --git a/pdns/recursordist/pdns_recursor.cc b/pdns/recursordist/pdns_recursor.cc
index 1dbf5bb171eac548b46f614a445963ffae4b340f..a54f055fd3dba32bec12004a0505f40d308ca732 100644 (file)
--- a/pdns/recursordist/pdns_recursor.cc
+++ b/pdns/recursordist/pdns_recursor.cc
@@ -42,7 +42,7 @@
 
 thread_local std::shared_ptr<RecursorLua4> t_pdl;
 thread_local std::shared_ptr<Regex> t_traceRegex;
-thread_local int t_tracefd = -1;
+thread_local FDWrapper t_tracefd = -1;
 thread_local ProtobufServersInfo t_protobufServers;
 thread_local ProtobufServersInfo t_outgoingProtobufServers;
 
@@ -861,10 +861,10 @@ static void dumpTrace(const string& trace, const timeval& timev)
     return;
   }
   std::array<char, 64> timebuf;
-  timestamp(timev, timebuf.data(), timebuf.size());
+  isoDateTimeMillis(timev, timebuf.data(), timebuf.size());
   fprintf(filep.get(), " us === START OF TRACE %s ===\n", timebuf.data());
   fprintf(filep.get(), "%s", trace.c_str());
-  timestamp(now, timebuf.data(), timebuf.size());
+  isoDateTimeMillis(now, timebuf.data(), timebuf.size());
   fprintf(filep.get(), "=== END OF TRACE %s ===\n", timebuf.data());
   // fclose by unique_ptr does implicit flush
 }

diff --git a/pdns/recursordist/rec-main.cc b/pdns/recursordist/rec-main.cc
index 3acf1bfb62878ace714ae992220eca816980688d..31b2bf1101248558a5e454c850e326f21675adac 100644 (file)
--- a/pdns/recursordist/rec-main.cc
+++ b/pdns/recursordist/rec-main.cc
@@ -3096,21 +3096,17 @@ static string* pleaseUseNewTraceRegex(const std::string& newRegex, int file)
   try {
     if (newRegex.empty()) {
       t_traceRegex.reset();
-      close(t_tracefd);
-      t_tracefd = -1;
+      t_tracefd = FDWrapper();
       return new string("unset\n");
     }
     if (file == -1) {
       return new string("could not dup file\n");
     }
-    if (t_tracefd != -1) {
-      close(t_tracefd);
-    }
     t_traceRegex = std::make_shared<Regex>(newRegex);
     t_tracefd = file;
     return new string("ok\n");
   }
-  catch (PDNSException& ae) {
+  catch (const PDNSException& ae) {
     return new string(ae.reason + "\n");
   }
 }

diff --git a/pdns/recursordist/rec-main.hh b/pdns/recursordist/rec-main.hh
index 58ee029e2e3461f206c1442fd3691c6d7b564783..1a1c9ee63006433784eea535eb3adc66b9e9e7a9 100644 (file)
--- a/pdns/recursordist/rec-main.hh
+++ b/pdns/recursordist/rec-main.hh
@@ -227,7 +227,7 @@ extern std::shared_ptr<NetmaskGroup> g_initialAllowFrom; // new thread needs to
 extern std::shared_ptr<NetmaskGroup> g_initialAllowNotifyFrom; // new threads need this to be setup
 extern std::shared_ptr<notifyset_t> g_initialAllowNotifyFor; // new threads need this to be setup
 extern thread_local std::shared_ptr<Regex> t_traceRegex;
-extern thread_local int t_tracefd;
+extern thread_local FDWrapper t_tracefd;
 extern string g_programname;
 extern string g_pidfname;
 extern RecursorControlChannel g_rcc; // only active in the handler thread

diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc
index bed78220b00cc29c32f84494ae6edf56c3d7c169..386065628e2ba051afc20046e148fce0c0dcaa34 100644 (file)
--- a/pdns/recursordist/syncres.cc
+++ b/pdns/recursordist/syncres.cc
@@ -1035,7 +1035,7 @@ bool SyncRes::isForwardOrAuth(const DNSName& qname) const
   return iter != t_sstorage.domainmap->end();
 }
 
-const char* timestamp(const struct timeval& tv, char* buf, size_t sz)
+const char* isoDateTimeMillis(const struct timeval& tv, char* buf, size_t sz)
 {
   const std::string s_timestampFormat = "%Y-%m-%dT%T";
   struct tm tm;
@@ -1200,7 +1200,7 @@ uint64_t SyncRes::doDumpNSSpeeds(int fd)
 
     // an <empty> can appear hear in case of authoritative (hosted) zones
     char tmp[26];
-    fprintf(fp.get(), "%s\t%s\t", i.d_name.toLogString().c_str(), timestamp(i.d_lastget, tmp, sizeof(tmp)));
+    fprintf(fp.get(), "%s\t%s\t", i.d_name.toLogString().c_str(), isoDateTimeMillis(i.d_lastget, tmp, sizeof(tmp)));
     bool first = true;
     for (const auto& j : i.d_collection) {
       fprintf(fp.get(), "%s%s/%.3f/%.3f", first ? "" : "\t", j.first.toStringWithPortExcept(53).c_str(), j.second.peek() / 1000.0f, j.second.last() / 1000.0f);

diff --git a/pdns/recursordist/syncres.hh b/pdns/recursordist/syncres.hh
index b500c143ff9b0c5daf9875484301ef3dc191112c..1b99c277f02c4955d38f303e4ffecd7f0b545d28 100644 (file)
--- a/pdns/recursordist/syncres.hh
+++ b/pdns/recursordist/syncres.hh
@@ -568,6 +568,7 @@ public:
   unsigned int d_timeouts;
   unsigned int d_unreachables;
   unsigned int d_totUsec;
+  // Initialized ony once, as opposed to d_now which gets updated after outgoing requests
   const struct timeval d_fixednow;
 
 private:
@@ -692,6 +693,7 @@ private:
   std::shared_ptr<std::vector<std::unique_ptr<FrameStreamLogger>>> d_frameStreamServers;
   boost::optional<const boost::uuids::uuid&> d_initialRequestId;
   asyncresolve_t d_asyncResolve{nullptr};
+  // d_now is initialized in the constructor and updates after outgoing requests in lwres.cc:asyncresolve
   struct timeval d_now;
   /* if the client is asking for a DS that does not exist, we need to provide the SOA along with the NSEC(3) proof
      and we might not have it if we picked up the proof from a delegation */
@@ -922,7 +924,7 @@ uint64_t* pleaseGetPacketCacheHits();
 uint64_t* pleaseGetPacketCacheSize();
 void doCarbonDump(void*);
 bool primeHints(time_t now = time(nullptr));
-const char* timestamp(const struct timeval& tv, char* buf, size_t sz);
+const char* isoDateTimeMillis(const struct timeval& tv, char* buf, size_t sz);
 
 struct WipeCacheResult
 {