Improve error message storage in Dns::LookupDetails (#1241)

Removed one more deprecated String usage and optimized handling of a
common "no error, no lookup" case as well as portions of the "no-error
lookup" code paths. Further optimizations need similar ipcache_entry and
fqdncache_entry error storage changes (at least).

Co-authored-by: Alex Rousskov <rousskov@measurement-factory.com>

diff --git a/src/dns/LookupDetails.cc b/src/dns/LookupDetails.cc
index 01c15abddb9e3ba91b51ffdb074590c39f9a151e..743b6744a98393361bd08fe65f4771b629a9566d 100644 (file)
--- a/src/dns/LookupDetails.cc
+++ b/src/dns/LookupDetails.cc
@@ -16,8 +16,8 @@ Dns::LookupDetails::print(std::ostream &os) const
 {
     if (wait > 0)
         os << "lookup_wait=" << wait;
-    if (error.size())
-        os << " lookup_err=" << error;
+    if (error)
+        os << " lookup_err=" << *error;
     return os;
 }
 

diff --git a/src/dns/LookupDetails.h b/src/dns/LookupDetails.h
index 1e28dcdcba2e98a087145afbf6d95c067b80b410..fc7c36eb4f901db812b3b5018d5ef3256b305fce 100644 (file)
--- a/src/dns/LookupDetails.h
+++ b/src/dns/LookupDetails.h
@@ -11,7 +11,9 @@
 #ifndef SQUID_DNS_LOOKUPDETAILS_H
 #define SQUID_DNS_LOOKUPDETAILS_H
 
-#include "SquidString.h"
+#include "sbuf/SBuf.h"
+
+#include <optional>
 
 namespace Dns
 {
@@ -20,13 +22,21 @@ namespace Dns
 class LookupDetails
 {
 public:
-    LookupDetails() : wait(-1) {} ///< no error, no lookup delay (i.e., no lookup)
-    LookupDetails(const String &anError, int aWait) : error(anError), wait(aWait) {}
+    /// no lookup attempt: no error and no lookup delay
+    LookupDetails(): wait(-1) {}
+
+    /// details a possible lookup attempt
+    /// \param anError either a failed attempt error message or an empty string
+    /// \param aWait \copydoc wait
+    LookupDetails(const SBuf &anError, const int aWait):
+        error(anError.isEmpty() ? std::nullopt : std::make_optional(anError)),
+        wait(aWait)
+    {}
 
     std::ostream &print(std::ostream &os) const;
 
 public:
-    String error; ///< error message for unsuccessful lookups; empty otherwise
+    const std::optional<SBuf> error; ///< error message (if any)
     int wait; ///< msecs spent waiting for the lookup (if any) or -1 (if none)
 };
 

diff --git a/src/errorpage.cc b/src/errorpage.cc
index 4d257bc8c6ce2971489b23164c5ae9d50468ed1f..c4287f4140db0047f1a3a937cb8b45965d8e8094 100644 (file)
--- a/src/errorpage.cc
+++ b/src/errorpage.cc
@@ -828,8 +828,8 @@ ErrorState::Dump(MemBuf * mb)
     if (auth_user_request.getRaw() && auth_user_request->denyMessage())
         str.appendf("Auth ErrMsg: %s\r\n", auth_user_request->denyMessage());
 #endif
-    if (dnsError.size() > 0)
-        str.appendf("DNS ErrMsg: %s\r\n", dnsError.termedBuf());
+    if (dnsError)
+        str.appendf("DNS ErrMsg: " SQUIDSBUFPH "\r\n", SQUIDSBUFPRINT(*dnsError));
 
     /* - TimeStamp */
     str.appendf("TimeStamp: %s\r\n\r\n", Time::FormatRfc1123(squid_curtime));
@@ -1212,8 +1212,8 @@ ErrorState::compileLegacyCode(Build &build)
 
     case 'z':
         if (building_deny_info_url) break;
-        if (dnsError.size() > 0)
-            p = dnsError.termedBuf();
+        if (dnsError)
+            p = dnsError->c_str();
         else if (ftp.cwd_msg)
             p = ftp.cwd_msg;
         else

diff --git a/src/errorpage.h b/src/errorpage.h
index d711a252992edc54be0992c1f013e05cbfd42ea6..1d3b53045173c763856365dee39d6248c74c647e 100644 (file)
--- a/src/errorpage.h
+++ b/src/errorpage.h
@@ -24,6 +24,8 @@
 /* auth/UserRequest.h is empty unless USE_AUTH is defined */
 #include "auth/UserRequest.h"
 
+#include <optional>
+
 /// error page callback
 typedef void ERCB(int fd, void *, size_t);
 
@@ -176,7 +178,7 @@ public:
     char *url = nullptr;
     int xerrno = 0;
     unsigned short port = 0;
-    String dnsError; ///< DNS lookup error message
+    std::optional<SBuf> dnsError; ///< DNS lookup error message
     time_t ttl = 0;
 
     Ip::Address src_addr;

diff --git a/src/fqdncache.cc b/src/fqdncache.cc
index 1f9978bd56cd3688afc279da8c3094b6263972b5..78670fb23526feb2227aca8cd9e816f8300ff356 100644 (file)
--- a/src/fqdncache.cc
+++ b/src/fqdncache.cc
@@ -304,7 +304,7 @@ fqdncacheCallback(fqdncache_entry * f, int wait)
     f->handler = nullptr;
 
     if (cbdataReferenceValidDone(f->handlerData, &cbdata)) {
-        const Dns::LookupDetails details(f->error_message, wait);
+        const Dns::LookupDetails details(SBuf(f->error_message), wait);
         callback(f->name_count ? f->names[0] : nullptr, details, cbdata);
     }
 
@@ -422,7 +422,7 @@ fqdncache_nbgethostbyaddr(const Ip::Address &addr, FQDNH * handler, void *handle
 
     if (name[0] == '\0') {
         debugs(35, 4, "fqdncache_nbgethostbyaddr: Invalid name!");
-        const Dns::LookupDetails details("Invalid hostname", -1); // error, no lookup
+        static const Dns::LookupDetails details(SBuf("Invalid hostname"), -1); // error, no lookup
         if (handler)
             handler(nullptr, details, handlerData);
         return;

diff --git a/src/ipcache.cc b/src/ipcache.cc
index fbb0a217c04b9b4922ee7d58f20874836b49c681..4a0630b48d797b73e308ffc84cf6f6cea4a4fd4b 100644 (file)
--- a/src/ipcache.cc
+++ b/src/ipcache.cc
@@ -280,7 +280,7 @@ IpCacheLookupForwarder::forwardLookup(const char *error)
     // are sequential. Give it just the new, yet-unaccounted-for delay.
     if (receiverObj.set()) {
         if (auto receiver = receiverObj.valid()) {
-            receiver->noteLookup(Dns::LookupDetails(error, additionalLookupDelay()));
+            receiver->noteLookup(Dns::LookupDetails(SBuf(error), additionalLookupDelay()));
             lastLookupEnd = current_time;
         }
     }
@@ -445,7 +445,7 @@ ipcacheCallback(ipcache_entry *i, const bool hit, const int wait)
 
     if (hit)
         i->handler.forwardHits(i->addrs);
-    const Dns::LookupDetails details(i->error_message, wait);
+    const Dns::LookupDetails details(SBuf(i->error_message), wait);
     i->handler.finalCallback(&i->addrs, details);
 
     ipcacheUnlockEntry(i);
@@ -620,7 +620,7 @@ ipcache_nbgethostbyname_(const char *name, IpCacheLookupForwarder handler)
     if (name == nullptr || name[0] == '\0') {
         debugs(14, 4, "ipcache_nbgethostbyname: Invalid name!");
         ++IpcacheStats.invalid;
-        const Dns::LookupDetails details("Invalid hostname", -1); // error, no lookup
+        static const Dns::LookupDetails details(SBuf("Invalid hostname"), -1); // error, no lookup
         handler.finalCallback(nullptr, details);
         return;
     }