From f3493699c5487343aa5e58180f7bdae30d49dfaf Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Tue, 31 Jan 2023 13:29:43 +0100 Subject: [PATCH] Process review comments from rgacogne --- pdns/logger.cc | 2 -- pdns/logger.hh | 7 ------- pdns/recursordist/pdns_recursor.cc | 6 +++--- pdns/recursordist/rec-main.cc | 8 ++------ pdns/recursordist/rec-main.hh | 2 +- pdns/recursordist/syncres.cc | 4 ++-- pdns/recursordist/syncres.hh | 4 +++- 7 files changed, 11 insertions(+), 22 deletions(-) diff --git a/pdns/logger.cc b/pdns/logger.cc index 0711b971b0..00d94906e0 100644 --- 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 5e2f45dea5..dbe7f278b2 100644 --- a/pdns/logger.hh +++ b/pdns/logger.hh @@ -183,11 +183,6 @@ struct LogVariant using OptLog = std::optional; -#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((log)->v) << x; \ } \ } - -#endif diff --git a/pdns/recursordist/pdns_recursor.cc b/pdns/recursordist/pdns_recursor.cc index 1dbf5bb171..a54f055fd3 100644 --- a/pdns/recursordist/pdns_recursor.cc +++ b/pdns/recursordist/pdns_recursor.cc @@ -42,7 +42,7 @@ thread_local std::shared_ptr t_pdl; thread_local std::shared_ptr 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 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 3acf1bfb62..31b2bf1101 100644 --- 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(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 58ee029e2e..1a1c9ee630 100644 --- a/pdns/recursordist/rec-main.hh +++ b/pdns/recursordist/rec-main.hh @@ -227,7 +227,7 @@ extern std::shared_ptr g_initialAllowFrom; // new thread needs to extern std::shared_ptr g_initialAllowNotifyFrom; // new threads need this to be setup extern std::shared_ptr g_initialAllowNotifyFor; // new threads need this to be setup extern thread_local std::shared_ptr 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 bed78220b0..386065628e 100644 --- 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 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 b500c143ff..1b99c277f0 100644 --- 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>> d_frameStreamServers; boost::optional 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 { -- 2.47.2