]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
Improve cache_peer reporting and NetDB cache_peer search (#1174)
authorAlex Rousskov <rousskov@measurement-factory.com>
Wed, 9 Nov 2022 21:29:33 +0000 (21:29 +0000)
committerSquid Anubis <squid-anubis@squid-cache.org>
Wed, 9 Nov 2022 21:29:37 +0000 (21:29 +0000)
Squid used several cache_peer configuration components and CachePeer
fields to identify cache peers in code and cache.log messages: (the
lowercase version of) cache_peer hostname, cache_peer (hostname,
http_port) tuple, and cache_peer name=value (which uses cache_peer
hostname by default).

Inconsistent identification led to admin confusion (e.g., a message
about cache_peer A was misinterpreted as being about cache_peer B) and
bugs (e.g., indexing storage by component X, but searching using
component Y). This change cements a single component as a unique
cache_peer identifier: CachePeer::name.

Also reject configurations with cache_peer naming errors in
cache_peer_access and neighbor_type_domain directives (instead of
reporting but otherwise ignoring the problematic configuration lines).

The following CachePeer reporting principles were used:

* To identify a specific cache_peer p (among all the configured ones) in
  a cache.log message, print that peer (i.e. use `os << *p` or
  equivalent). CachePeer printing code will print that peer name, which
  is guaranteed to be unique across all configured cache_peers.

* After identifying a cache_peer, do not dump its various details like
  ICP port numbers, especially when they are not relevant to the message
  (i.e. provide no useful context-related information going beyond that
  cache_peer identification).

The following bugs were fixed:

* NetDB code is storing cache_peer hostname[1] but was searching for a
  cache_peer with a matching name[2] and, hence, usually failing to find
  explicitly named cache_peers. Probably broken since 2003 commit
  be75332 that added name=value support and updated peerFindByName() to
  use CachePeer::name but left storage code[1] unchanged.

    [1] netdbPeerAdd(): p->peername = netdbPeerName(e->host);
    [2] netdbClosestParent(): p = peerFindByName(h->peername);

* netdbClosestParent() could not find the closest cache_peer in certain
  cases involving multiple cache_peers with the same hostname because
  the code incorrectly stopped looking for eligible same-hostname
  cache_peers after correctly discarding the first one.

  The above NetDB problems may imply that NetDB code needs to be
  refactored to store CachePeer pointers instead of CachePeer hostnames,
  but that serious change deserves more analysis (and a dedicated PR).

* cache_peer name=value configurations with empty/missing name value
  could dereference a null name pointer while misleading developers into
  thinking that a previously set name could be erased/forgotten. Squid
  now rejects such configurations.

* neighborIgnoreNonPeer() was allocating CachePeer::host using new() but
  ~CachePeer is freeing its host data member using xfree().

Also removed peerFindByNameAndPort() unused since inception at db1cd23.

21 files changed:
doc/debug-messages.dox
src/CachePeer.cc
src/CachePeer.h
src/ConfigParser.cc
src/ConfigParser.h
src/HttpRequest.cc
src/Makefile.am
src/base/IoManip.h
src/cache_cf.cc
src/carp.cc
src/cf.data.pre
src/icmp/net_db.cc
src/icmp/net_db.h
src/neighbors.cc
src/neighbors.h
src/peer_digest.cc
src/peer_select.cc
src/peer_sourcehash.cc
src/peer_userhash.cc
src/tests/stub_CachePeer.cc
src/tests/stub_neighbors.cc

index f5713e964c00537f38520e996b6db96dc9f27123..f870823848c70736cde3737b7ecf4182754a326d 100644 (file)
@@ -38,7 +38,7 @@ ID Message gist
 26 Logfile: opening log ...
 27 Logfile: closing log ...
 28 Finished loading MIME types and icons.
-29 Configuring ... .../ .../ ...
+29 Configuring ... ...
 30 storeLateRelease: released ... objects
 31 Swap maxSize ... KB, estimated ... objects
 32 Target number of buckets: ...
index bad71c58c1b1cb7bb079e3448bf1b8d6010d9498..24bf56714f203ce30fd9fe0c32128c82f2c45a4d 100644 (file)
 #include "pconn.h"
 #include "PeerPoolMgr.h"
 #include "SquidConfig.h"
+#include "util.h"
 
 CBDATA_CLASS_INIT(CachePeer);
 
+CachePeer::CachePeer(const char * const hostname):
+    name(xstrdup(hostname)),
+    host(xstrdup(hostname))
+{
+    Tolower(host); // but .name preserves original spelling
+}
+
 CachePeer::~CachePeer()
 {
     xfree(name);
@@ -47,6 +55,16 @@ CachePeer::~CachePeer()
     xfree(domain);
 }
 
+void
+CachePeer::rename(const char * const newName)
+{
+    if (!newName || !*newName)
+        throw TextException("cache_peer name=value cannot be empty", Here());
+
+    xfree(name);
+    name = xstrdup(newName);
+}
+
 time_t
 CachePeer::connectTimeout() const
 {
@@ -55,3 +73,9 @@ CachePeer::connectTimeout() const
     return Config.Timeout.peer_connect;
 }
 
+std::ostream &
+operator <<(std::ostream &os, const CachePeer &p)
+{
+    return os << p.name;
+}
+
index 76d1e48193b7ba04477c5d6ffcaaef06d94fa2f5..2beb460982833ad1b675daaafcca70206c69dd2f 100644 (file)
@@ -16,6 +16,8 @@
 #include "ip/Address.h"
 #include "security/PeerOptions.h"
 
+#include <iosfwd>
+
 //TODO: remove, it is unconditionally defined and always used.
 #define PEER_MULTICAST_SIBLINGS 1
 
@@ -29,15 +31,32 @@ class CachePeer
     CBDATA_CLASS(CachePeer);
 
 public:
-    CachePeer() = default;
+    explicit CachePeer(const char *hostname);
     ~CachePeer();
 
+    /// (re)configure cache_peer name=value
+    void rename(const char *);
+
     /// \returns the effective connect timeout for the given peer
     time_t connectTimeout() const;
 
     u_int index = 0;
+
+    /// cache_peer name (if explicitly configured) or hostname (otherwise).
+    /// Unique across already configured cache_peers in the current configuration.
+    /// Not necessarily unique across discovered non-peers (see mgr:non_peers).
+    /// The value may change during CachePeer configuration.
+    /// The value affects various peer selection hashes (e.g., carp.hash).
+    /// Preserves configured spelling (i.e. does not lower letters case).
+    /// Never nil.
     char *name = nullptr;
+
+    /// The lowercase version of the configured cache_peer hostname or
+    /// the IP address of a non-peer (see mgr:non_peers).
+    /// May not be unique among cache_peers and non-peers.
+    /// Never nil.
     char *host = nullptr;
+
     peer_t type = PEER_NONE;
 
     Ip::Address in_addr;
@@ -199,5 +218,8 @@ public:
     int connection_auth = 2; ///< 0 - off, 1 - on, 2 - auto
 };
 
+/// identify the given cache peer in cache.log messages and such
+std::ostream &operator <<(std::ostream &, const CachePeer &);
+
 #endif /* SQUID_CACHEPEER_H_ */
 
index a2d51ab3b7ba670688e3d715c17b602230b97074..5dce42b5fe45b8174cad7988c7b7fb3db0a82487 100644 (file)
@@ -15,6 +15,7 @@
 #include "debug/Stream.h"
 #include "fatal.h"
 #include "globals.h"
+#include "neighbors.h"
 #include "sbuf/Stream.h"
 
 bool ConfigParser::RecognizeQuotedValues = true;
@@ -503,6 +504,22 @@ ConfigParser::regex(const char *expectedRegexDescription)
     return std::unique_ptr<RegexPattern>(new RegexPattern(pattern, flags));
 }
 
+CachePeer &
+ConfigParser::cachePeer(const char *peerNameTokenDescription)
+{
+    if (const auto name = NextToken()) {
+        debugs(3, 5, CurrentLocation() << ' ' << peerNameTokenDescription << ": " << name);
+
+        if (const auto p = findCachePeerByName(name))
+            return *p;
+
+        throw TextException(ToSBuf("Cannot find a previously declared cache_peer referred to by ",
+            peerNameTokenDescription, " as ", name), Here());
+    }
+
+    throw TextException(ToSBuf("Missing ", peerNameTokenDescription), Here());
+}
+
 char *
 ConfigParser::NextQuotedToken()
 {
index 8347dd478087f7ce84e527e3aa1a93ba4fd878c8..8f205639207f418ee012a1c5c7dfc2a7a7489e29 100644 (file)
@@ -19,6 +19,7 @@
 #include <stack>
 #include <string>
 
+class CachePeer;
 class wordlist;
 
 /**
@@ -76,6 +77,9 @@ public:
     /// extracts and returns a regex (including any optional flags)
     std::unique_ptr<RegexPattern> regex(const char *expectedRegexDescription);
 
+    /// extracts a cache_peer name token and returns the corresponding CachePeer
+    CachePeer &cachePeer(const char *peerNameTokenDescription);
+
     static void ParseUShort(unsigned short *var);
     static void ParseBool(bool *var);
     static const char *QuoteString(const String &var);
index 0920f08a0c441278e9f3eaf469e60fa09f9645ef..ed4c3e30f878a62fdb26632b3f0984cf17001172 100644 (file)
@@ -449,7 +449,7 @@ HttpRequest::prepForPeering(const CachePeer &peer)
     peer_login = peer.login;
     peer_domain = peer.domain;
     flags.auth_no_keytab = peer.options.auth_no_keytab;
-    debugs(11, 4, this << " to " << peer.host << (!peer.options.originserver ? " proxy" : " origin"));
+    debugs(11, 4, this << " to " << peer);
 }
 
 void
index 58c576dc29fab457155e118599dcd4bf0cffc362..d1498f260f484921fb77417a5615b2d2b2613ef0 100644 (file)
@@ -1354,6 +1354,7 @@ tests_testStore_SOURCES = \
        $(DELAY_POOL_SOURCE) \
        tests/stub_CacheDigest.cc \
        CacheDigest.h \
+       tests/stub_CachePeer.cc \
        ClientInfo.h \
        tests/stub_CollapsedForwarding.cc \
        ConfigOption.cc \
@@ -1444,6 +1445,7 @@ tests_testStore_SOURCES = \
        tests/stub_libformat.cc \
        tests/stub_libsecurity.cc \
        tests/stub_libsslsquid.cc \
+       tests/stub_neighbors.cc \
        log/access_log.h \
        mem_node.cc \
        tests/stub_mime.cc \
@@ -1692,6 +1694,7 @@ tests_testACLMaxUserIP_SOURCES = \
        tests/testACLMaxUserIP.h
 nodist_tests_testACLMaxUserIP_SOURCES = \
        ConfigParser.cc \
+       tests/stub_CachePeer.cc \
        tests/stub_HelperChildConfig.cc \
        tests/stub_HttpHeader.cc \
        tests/stub_HttpRequest.cc \
@@ -1709,9 +1712,11 @@ nodist_tests_testACLMaxUserIP_SOURCES = \
        tests/stub_fatal.cc \
        globals.cc \
        tests/stub_libauth.cc \
+       tests/stub_libcomm.cc \
        tests/stub_libhttp.cc \
        tests/stub_libmem.cc \
-       tests/stub_libsecurity.cc
+       tests/stub_libsecurity.cc \
+       tests/stub_neighbors.cc
 tests_testACLMaxUserIP_LDADD = \
        $(AUTH_ACL_LIBS) \
        acl/libapi.la \
@@ -2018,6 +2023,7 @@ tests_testHttp1Parser_LDFLAGS = $(LIBADD_DL)
 check_PROGRAMS += tests/testHttpReply
 tests_testHttpReply_SOURCES = \
        ConfigParser.cc \
+       tests/stub_CachePeer.cc \
        tests/stub_ETag.cc \
        tests/stub_HelperChildConfig.cc \
        HttpBody.cc \
@@ -2081,6 +2087,7 @@ tests_testHttpReply_SOURCES = \
        tests/stub_libmgr.cc \
        tests/stub_libsecurity.cc \
        tests/stub_libsslsquid.cc \
+       tests/stub_neighbors.cc \
        log/access_log.h \
        mime_header.cc \
        mime_header.h \
@@ -2706,7 +2713,8 @@ nodist_tests_testConfigParser_SOURCES = \
        tests/stub_cache_cf.cc \
        tests/stub_debug.cc \
        tests/stub_fatal.cc \
-       tests/stub_libmem.cc
+       tests/stub_libmem.cc \
+       tests/stub_neighbors.cc
 tests_testConfigParser_LDADD = \
        base/libbase.la \
        $(LIBCPPUNIT_LIBS) \
index f13c697b621c88b92e25c17fede7ac9f0fbdddab..bb60a8b67323a9821d3a08bd094afa5156662733 100644 (file)
@@ -25,12 +25,19 @@ public:
     /// Report the pointed-to-object on a dedicated Debug::Extra line.
     RawPointerT<Pointer> &asExtra() { onExtraLine = true; return *this; }
 
+    /// enable and, optionally, customize reporting of nil pointers
+    RawPointerT<Pointer> &orNil(const char *nilTextToUse = "[nil]") { nilText = nilTextToUse; return *this; }
+
     const char *label; /// the name or description of the being-debugged object
+
+    /// whether and how to report a nil pointer; use orNil() to enable
+    const char *nilText = nullptr;
+
     const Pointer &ptr; /// a possibly nil pointer to the being-debugged object
     bool onExtraLine = false;
 };
 
-/// convenience wrapper for creating  RawPointerT<> objects
+/// convenience wrapper for creating RawPointerT<> objects
 template <class Pointer>
 inline RawPointerT<Pointer>
 RawPointer(const char *label, const Pointer &ptr)
@@ -38,13 +45,24 @@ RawPointer(const char *label, const Pointer &ptr)
     return RawPointerT<Pointer>(label, ptr);
 }
 
+/// convenience wrapper for creating RawPointerT<> objects without a label
+template <class Pointer>
+inline RawPointerT<Pointer>
+RawPointer(const Pointer &ptr)
+{
+    return RawPointerT<Pointer>(nullptr, ptr);
+}
+
 /// prints RawPointerT<>, dereferencing the io_manip pointer if possible
 template <class Pointer>
 inline std::ostream &
 operator <<(std::ostream &os, const RawPointerT<Pointer> &pd)
 {
-    if (!pd.ptr)
+    if (!pd.ptr) {
+        if (pd.nilText)
+            os << pd.nilText;
         return os;
+    }
 
     if (pd.onExtraLine)
         os << Debug::Extra;
index 50392b5a0f6b844ffc6255c3ae3db08ef07a2e87..dbfb5c476ffdd1bef49395093e6e8e4c029c9090 100644 (file)
@@ -982,10 +982,10 @@ configDoConfigure(void)
             p->secure.sslDomain = p->host;
 
         if (p->secure.encryptTransport) {
-            debugs(3, DBG_IMPORTANT, "Initializing cache_peer " << p->name << " TLS context");
+            debugs(3, DBG_IMPORTANT, "Initializing TLS context for cache_peer " << *p);
             p->sslContext = p->secure.createClientContext(true);
             if (!p->sslContext) {
-                debugs(3, DBG_CRITICAL, "ERROR: Could not initialize cache_peer " << p->name << " TLS context");
+                debugs(3, DBG_CRITICAL, "ERROR: Could not initialize TLS context for cache_peer " << *p);
                 self_destruct();
                 return;
             }
@@ -2174,10 +2174,8 @@ parse_peer(CachePeer ** head)
         return;
     }
 
-    CachePeer *p = new CachePeer;
-    p->host = xstrdup(host_str);
-    Tolower(p->host);
-    p->name = xstrdup(host_str);
+    const auto p = new CachePeer(host_str);
+
     p->type = parseNeighborType(token);
 
     if (p->type == PEER_MULTICAST) {
@@ -2271,12 +2269,12 @@ parse_peer(CachePeer ** head)
 
         } else if (!strcmp(token, "carp")) {
             if (p->type != PEER_PARENT)
-                fatalf("parse_peer: non-parent carp peer %s/%d\n", p->host, p->http_port);
+                throw TextException(ToSBuf("non-parent carp cache_peer ", *p), Here());
 
             p->options.carp = true;
         } else if (!strncmp(token, "carp-key=", 9)) {
             if (p->options.carp != true)
-                fatalf("parse_peer: carp-key specified on non-carp peer %s/%d\n", p->host, p->http_port);
+                throw TextException(ToSBuf("carp-key specified on non-carp cache_peer ", *p), Here());
             p->options.carp_key.set = true;
             char *nextkey=token+strlen("carp-key="), *key=nextkey;
             for (; key; key = nextkey) {
@@ -2299,15 +2297,15 @@ parse_peer(CachePeer ** head)
         } else if (!strcmp(token, "userhash")) {
 #if USE_AUTH
             if (p->type != PEER_PARENT)
-                fatalf("parse_peer: non-parent userhash peer %s/%d\n", p->host, p->http_port);
+                throw TextException(ToSBuf("non-parent userhash cache_peer ", *p), Here());
 
             p->options.userhash = true;
 #else
-            fatalf("parse_peer: userhash requires authentication. peer %s/%d\n", p->host, p->http_port);
+            throw TextException(ToSBuf("missing authentication support; required for userhash cache_peer ", *p), Here());
 #endif
         } else if (!strcmp(token, "sourcehash")) {
             if (p->type != PEER_PARENT)
-                fatalf("parse_peer: non-parent sourcehash peer %s/%d\n", p->host, p->http_port);
+                throw TextException(ToSBuf("non-parent sourcehash cache_peer ", *p), Here());
 
             p->options.sourcehash = true;
 
@@ -2340,10 +2338,7 @@ parse_peer(CachePeer ** head)
         } else if (!strcmp(token, "originserver")) {
             p->options.originserver = true;
         } else if (!strncmp(token, "name=", 5)) {
-            safe_free(p->name);
-
-            if (token[5])
-                p->name = xstrdup(token + 5);
+            p->rename(token + 5);
         } else if (!strncmp(token, "forceddomain=", 13)) {
             safe_free(p->domain);
             if (token[13])
@@ -2381,11 +2376,12 @@ parse_peer(CachePeer ** head)
         }
     }
 
-    if (peerFindByName(p->name))
-        fatalf("ERROR: cache_peer %s specified twice\n", p->name);
+    if (findCachePeerByName(p->name))
+        throw TextException(ToSBuf("cache_peer ", *p, " specified twice"), Here());
 
     if (p->max_conn > 0 && p->max_conn < p->standby.limit)
-        fatalf("ERROR: cache_peer %s max-conn=%d is lower than its standby=%d\n", p->host, p->max_conn, p->standby.limit);
+        throw TextException(ToSBuf("cache_peer ", *p, " max-conn=", p->max_conn,
+                                   " is lower than its standby=", p->standby.limit), Here());
 
     if (p->weight < 1)
         p->weight = 1;
@@ -2509,31 +2505,16 @@ free_denyinfo(AclDenyInfoList ** list)
 static void
 parse_peer_access(void)
 {
-    char *host = ConfigParser::NextToken();
-    if (!host) {
-        self_destruct();
-        return;
-    }
-
-    CachePeer *p = peerFindByName(host);
-    if (!p) {
-        debugs(15, DBG_CRITICAL, "ERROR: " << cfg_filename << ", line " << config_lineno << ": No cache_peer '" << host << "'");
-        return;
-    }
-
+    auto &p = LegacyParser.cachePeer("cache_peer_access peer-name");
     std::string directive = "peer_access ";
-    directive += host;
-    aclParseAccessLine(directive.c_str(), LegacyParser, &p->access);
+    directive += p.name;
+    aclParseAccessLine(directive.c_str(), LegacyParser, &p.access);
 }
 
 static void
 parse_hostdomaintype(void)
 {
-    char *host = ConfigParser::NextToken();
-    if (!host) {
-        self_destruct();
-        return;
-    }
+    auto &p = LegacyParser.cachePeer("neighbor_type_domain peer-name");
 
     char *type = ConfigParser::NextToken();
     if (!type) {
@@ -2543,18 +2524,12 @@ parse_hostdomaintype(void)
 
     char *domain = nullptr;
     while ((domain = ConfigParser::NextToken())) {
-        CachePeer *p = peerFindByName(host);
-        if (!p) {
-            debugs(15, DBG_CRITICAL, "" << cfg_filename << ", line " << config_lineno << ": No cache_peer '" << host << "'");
-            return;
-        }
-
         auto *l = static_cast<NeighborTypeDomainList *>(xcalloc(1, sizeof(NeighborTypeDomainList)));
         l->type = parseNeighborType(type);
         l->domain = xstrdup(domain);
 
         NeighborTypeDomainList **L = nullptr;
-        for (L = &(p->typelist); *L; L = &((*L)->next));
+        for (L = &p.typelist; *L; L = &((*L)->next));
         *L = l;
     }
 }
index 562e8ab914facc4a2c088cd241f1abe8f67422dd..e699fcf38c9f68bd0aeca65f5d9c5f239bba60c9 100644 (file)
@@ -205,7 +205,7 @@ carpSelectParent(PeerSelector *ps)
         combined_hash += combined_hash * 0x62531965;
         combined_hash = ROTATE_LEFT(combined_hash, 21);
         score = combined_hash * tp->carp.load_multiplier;
-        debugs(39, 3, "carpSelectParent: key=" << key << " name=" << tp->name << " combined_hash=" << combined_hash  <<
+        debugs(39, 3, *tp << " key=" << key << " combined_hash=" << combined_hash  <<
                " score=" << std::setprecision(0) << score);
 
         if ((score > high_score) && peerHTTPOkay(tp, ps)) {
@@ -215,7 +215,7 @@ carpSelectParent(PeerSelector *ps)
     }
 
     if (p)
-        debugs(39, 2, "carpSelectParent: selected " << p->name);
+        debugs(39, 2, "selected " << *p);
 
     return p;
 }
index 5971483bea1a45e8b6174310312c8e2589f5252e..751d47cf9d5fc131536aa171a48870fe3be9e8ca 100644 (file)
@@ -3959,13 +3959,21 @@ DOC_START
                        configuration.
 
        name=xxx        Unique name for the peer.
-                       Required if you have multiple peers on the same host
-                       but different ports.
-                       This name can be used in cache_peer_access and similar
-                       directives to identify the peer.
+                       Required if you have multiple cache_peers with the same hostname.
+                       Defaults to cache_peer hostname when not explicitly specified.
+
+                       Other directives (e.g., cache_peer_access), cache manager reports,
+                       and cache.log messages use this name to refer to this cache_peer.
+
+                       The cache_peer name value affects hashing-based peer selection
+                       methods (e.g., carp and sourcehash).
+
                        Can be used by outgoing access controls through the
                        peername ACL type.
 
+                       The name value preserves configured spelling, but name uniqueness
+                       checks and name-based search are case-insensitive.
+
        no-tproxy       Do not use the client-spoof TPROXY support when forwarding
                        requests to this peer. Use normal address selection instead.
                        This overrides the spoof_client_ip ACL.
@@ -4026,7 +4034,11 @@ DOC_START
        about specific domains to the peer.
 
        Usage:
-                neighbor_type_domain neighbor parent|sibling domain domain ...
+               neighbor_type_domain peer-name parent|sibling domain...
+
+       For the required peer-name parameter, use either the value of the
+       cache_peer name=value parameter or, if name=value is missing, the
+       cache_peer hostname parameter.
 
        For example:
                cache_peer foo.example.com parent 3128 3130
index cda165551e84695b45c8b6c057a9746c28e91efc..ced1a37041b868b1b5b3f472325d0be9cd804cd4 100644 (file)
@@ -695,7 +695,7 @@ netdbExchangeHandleReply(void *data, StoreIOBuffer receivedData)
         return;
     }
 
-    debugs(38, 3, "netdbExchangeHandleReply: for '" << ex->p->host << ":" << ex->p->http_port << "'");
+    debugs(38, 3, "for " << *ex->p);
 
     if (receivedData.length == 0 && !receivedData.flags.error) {
         debugs(38, 3, "netdbExchangeHandleReply: Done");
@@ -1296,6 +1296,31 @@ netdbExchangeStart(void *data)
 #endif
 }
 
+#if USE_ICMP
+/// a netdbClosestParent() helper to find the first usable parent CachePeer
+/// responsible for the given hostname
+static CachePeer *
+findUsableParentAtHostname(PeerSelector *ps, const char * const hostname, const HttpRequest &request)
+{
+    for (auto p = Config.peers; p; p = p->next) {
+        // Both fields should be lowercase, but no code ensures that invariant.
+        // TODO: net_db_peer should point to CachePeer instead of its hostname!
+        if (strcasecmp(p->host, hostname) != 0)
+            continue;
+
+        if (neighborType(p, request.url) != PEER_PARENT)
+            continue;
+
+        if (!peerHTTPOkay(p, ps))
+            continue;
+
+        return p;
+    }
+
+    return nullptr;
+}
+#endif
+
 CachePeer *
 netdbClosestParent(PeerSelector *ps)
 {
@@ -1303,7 +1328,6 @@ netdbClosestParent(PeerSelector *ps)
     assert(ps);
     HttpRequest *request = ps->request;
 
-    CachePeer *p = nullptr;
     netdbEntry *n;
     const ipcache_addrs *ia;
     net_db_peer *h;
@@ -1338,18 +1362,8 @@ netdbClosestParent(PeerSelector *ps)
             if (n->rtt < h->rtt)
                 break;
 
-        p = peerFindByName(h->peername);
-
-        if (nullptr == p)      /* not found */
-            continue;
-
-        if (neighborType(p, request->url) != PEER_PARENT)
-            continue;
-
-        if (!peerHTTPOkay(p, ps))  /* not allowed */
-            continue;
-
-        return p;
+        if (const auto p = findUsableParentAtHostname(ps, h->peername, *request))
+            return p;
     }
 
 #else
index 68a1b5af54faf935db59284d23cc5df78c5d172d..16a98afa7978640fe9bcdba6cd1d011c125a4744 100644 (file)
@@ -37,7 +37,9 @@ public:
 class net_db_peer
 {
 public:
+    /// associated CachePeer::host (i.e. cache_peer hostname, not name=value!)
     const char *peername;
+
     double hops;
     double rtt;
     time_t expires;
index 0bc3a87c9a2d2b1f1727c70bae60b688d24cd0e1..2f9b25bf17dcbb951a2f39ed2df9c84d55601a82 100644 (file)
@@ -12,6 +12,7 @@
 #include "acl/FilledChecklist.h"
 #include "anyp/PortCfg.h"
 #include "base/EnumIterator.h"
+#include "base/IoManip.h"
 #include "CacheDigest.h"
 #include "CachePeer.h"
 #include "comm/Connection.h"
@@ -145,7 +146,7 @@ peerAllowedToUse(const CachePeer * p, PeerSelector * ps)
 #if PEER_MULTICAST_SIBLINGS
         if (p->type == PEER_MULTICAST && p->options.mcast_siblings &&
                 (request->flags.noCache || request->flags.refresh || request->flags.loopDetected || request->flags.needValidation))
-            debugs(15, 2, "peerAllowedToUse(" << p->name << ", " << request->url.authority() << ") : multicast-siblings optimization match");
+            debugs(15, 2, "multicast-siblings optimization match for " << *p << ", " << request->url.authority());
 #endif
         if (request->flags.noCache)
             return false;
@@ -305,7 +306,7 @@ getFirstUpParent(PeerSelector *ps)
         break;
     }
 
-    debugs(15, 3, "getFirstUpParent: returning " << (p ? p->host : "NULL"));
+    debugs(15, 3, "returning " << RawPointer(p).orNil());
     return p;
 }
 
@@ -346,7 +347,7 @@ getRoundRobinParent(PeerSelector *ps)
     if (q)
         ++ q->rr_count;
 
-    debugs(15, 3, "returning " << (q ? q->host : "NULL"));
+    debugs(15, 3, "returning " << RawPointer(q).orNil());
 
     return q;
 }
@@ -399,7 +400,7 @@ getWeightedRoundRobinParent(PeerSelector *ps)
         debugs(15, 3, "getWeightedRoundRobinParent: weighted_rtt " << weighted_rtt);
     }
 
-    debugs(15, 3, "getWeightedRoundRobinParent: returning " << (q ? q->host : "NULL"));
+    debugs(15, 3, "returning " << RawPointer(q).orNil());
     return q;
 }
 
@@ -459,7 +460,7 @@ static void
 peerAlive(CachePeer *p)
 {
     if (p->stats.logged_state == PEER_DEAD && p->tcp_up) {
-        debugs(15, DBG_IMPORTANT, "Detected REVIVED " << neighborTypeStr(p) << ": " << p->name);
+        debugs(15, DBG_IMPORTANT, "Detected REVIVED " << neighborTypeStr(p) << ": " << *p);
         p->stats.logged_state = PEER_ALIVE;
         peerClearRR();
         if (p->standby.mgr.valid())
@@ -488,12 +489,13 @@ getDefaultParent(PeerSelector *ps)
         if (!peerHTTPOkay(p, ps))
             continue;
 
-        debugs(15, 3, "getDefaultParent: returning " << p->host);
+        debugs(15, 3, "returning " << *p);
 
         return p;
     }
 
-    debugs(15, 3, "getDefaultParent: returning NULL");
+    // TODO: Refactor similar get*() functions to use our return/reporting style
+    debugs(15, 3, "none found");
     return nullptr;
 }
 
@@ -573,10 +575,7 @@ neighbors_init(void)
                     continue;
 
                 debugs(15, DBG_IMPORTANT, "WARNING: Peer looks like this host." <<
-                       Debug::Extra << "Ignoring " <<
-                       neighborTypeStr(thisPeer) << " " << thisPeer->host <<
-                       "/" << thisPeer->http_port << "/" <<
-                       thisPeer->icp.port);
+                       Debug::Extra << "Ignoring cache_peer " << *thisPeer);
 
                 neighborRemove(thisPeer);
             }
@@ -628,14 +627,14 @@ neighborsUdpPing(HttpRequest * request,
         if (p == nullptr)
             p = Config.peers;
 
-        debugs(15, 5, "neighborsUdpPing: Peer " << p->host);
+        debugs(15, 5, "candidate: " << *p);
 
         if (!peerWouldBePinged(p, ps))
             continue;       /* next CachePeer */
 
         ++peers_pinged;
 
-        debugs(15, 4, "neighborsUdpPing: pinging peer " << p->host << " for '" << url << "'");
+        debugs(15, 4, "pinging cache_peer " << *p << " for '" << url << "'");
 
         debugs(15, 3, "neighborsUdpPing: key = '" << entry->getMD5Text() << "'");
 
@@ -703,7 +702,7 @@ neighborsUdpPing(HttpRequest * request,
             /* log it once at the threshold */
 
             if (p->stats.logged_state == PEER_ALIVE) {
-                debugs(15, DBG_IMPORTANT, "Detected DEAD " << neighborTypeStr(p) << ": " << p->name);
+                debugs(15, DBG_IMPORTANT, "Detected DEAD " << neighborTypeStr(p) << ": " << *p);
                 p->stats.logged_state = PEER_DEAD;
             }
         }
@@ -764,7 +763,7 @@ peerDigestLookup(CachePeer * p, PeerSelector * ps)
     const cache_key *key = request ? storeKeyPublicByRequest(request) : nullptr;
     assert(p);
     assert(request);
-    debugs(15, 5, "peerDigestLookup: peer " << p->host);
+    debugs(15, 5, "cache_peer " << *p);
     /* does the peeer have a valid digest? */
 
     if (!p->digest) {
@@ -782,14 +781,14 @@ peerDigestLookup(CachePeer * p, PeerSelector * ps)
         return LOOKUP_NONE;
     }
 
-    debugs(15, 5, "peerDigestLookup: OK to lookup peer " << p->host);
+    debugs(15, 5, "OK to lookup cache_peer " << *p);
     assert(p->digest->cd);
     /* does digest predict a hit? */
 
     if (!p->digest->cd->contains(key))
         return LOOKUP_MISS;
 
-    debugs(15, 5, "peerDigestLookup: peer " << p->host << " says HIT!");
+    debugs(15, 5, "HIT for cache_peer " << *p);
 
     return LOOKUP_HIT;
 #else
@@ -842,7 +841,7 @@ neighborsDigestSelect(PeerSelector *ps)
 
         p_rtt = netdbHostRtt(p->host);
 
-        debugs(15, 5, "neighborsDigestSelect: peer " << p->host << " rtt: " << p_rtt);
+        debugs(15, 5, "cache_peer " << *p << " rtt: " << p_rtt);
 
         /* is this CachePeer better than others in terms of rtt ? */
         if (!best_p || (p_rtt && p_rtt < best_rtt)) {
@@ -852,7 +851,7 @@ neighborsDigestSelect(PeerSelector *ps)
             if (p_rtt)      /* informative choice (aka educated guess) */
                 ++ichoice_count;
 
-            debugs(15, 4, "neighborsDigestSelect: peer " << p->host << " leads with rtt " << best_rtt);
+            debugs(15, 4, "cache_peer " << *p << " leads with rtt " << best_rtt);
         }
     }
 
@@ -878,7 +877,7 @@ peerNoteDigestLookup(HttpRequest * request, CachePeer * p, lookup_t lookup)
         *request->hier.cd_host = '\0';
 
     request->hier.cd_lookup = lookup;
-    debugs(15, 4, "peerNoteDigestLookup: peer " << (p? p->host : "<none>") << ", lookup: " << lookup_t_str[lookup]  );
+    debugs(15, 4, "cache_peer " << RawPointer(p).orNil() << ", lookup: " << lookup_t_str[lookup]);
 #else
     (void)request;
     (void)p;
@@ -963,12 +962,12 @@ neighborIgnoreNonPeer(const Ip::Address &from, icp_opcode opcode)
     }
 
     if (np == nullptr) {
-        np = new CachePeer;
+        char fromStr[MAX_IPSTRLEN];
+        from.toStr(fromStr, sizeof(fromStr));
+        np = new CachePeer(fromStr);
         np->in_addr = from;
         np->icp.port = from.port();
         np->type = PEER_NONE;
-        np->host = new char[MAX_IPSTRLEN];
-        from.toStr(np->host,MAX_IPSTRLEN);
         np->next = non_peers;
         non_peers = np;
     }
@@ -976,7 +975,7 @@ neighborIgnoreNonPeer(const Ip::Address &from, icp_opcode opcode)
     ++ np->icp.counts[opcode];
 
     if (isPowTen(++np->stats.ignored_replies))
-        debugs(15, DBG_IMPORTANT, "WARNING: Ignored " << np->stats.ignored_replies << " replies from non-peer " << np->host);
+        debugs(15, DBG_IMPORTANT, "WARNING: Ignored " << np->stats.ignored_replies << " replies from non-peer " << *np);
 }
 
 /* ignoreMulticastReply
@@ -1071,7 +1070,7 @@ neighborsUdpAck(const cache_key * key, icp_common_t * header, const Ip::Address
         return;
     }
 
-    debugs(15, 3, "neighborsUdpAck: " << opcode_d << " for '" << storeKeyText(key) << "' from " << (p ? p->host : "source") << " ");
+    debugs(15, 3, opcode_d << " for " << storeKeyText(key) << " from " << RawPointer(p).orNil("source"));
 
     if (p) {
         ntype = neighborType(p, mem->request->url);
@@ -1103,7 +1102,7 @@ neighborsUdpAck(const cache_key * key, icp_common_t * header, const Ip::Address
         }
     } else if (opcode == ICP_SECHO) {
         if (p) {
-            debugs(15, DBG_IMPORTANT, "Ignoring SECHO from neighbor " << p->host);
+            debugs(15, DBG_IMPORTANT, "Ignoring SECHO from neighbor " << *p);
             neighborCountIgnored(p);
         } else {
             debugs(15, DBG_IMPORTANT, "Unsolicited SECHO from " << from);
@@ -1113,8 +1112,8 @@ neighborsUdpAck(const cache_key * key, icp_common_t * header, const Ip::Address
             neighborIgnoreNonPeer(from, opcode);
         } else if (p->stats.pings_acked > 100) {
             if (100 * p->icp.counts[ICP_DENIED] / p->stats.pings_acked > 95) {
-                debugs(15, DBG_CRITICAL, "95%% of replies from '" << p->host << "' are UDP_DENIED");
-                debugs(15, DBG_CRITICAL, "Disabling '" << p->host << "', please check your configuration.");
+                debugs(15, DBG_CRITICAL, "Disabling cache_peer " << *p <<
+                       " because over 95% of its replies are UDP_DENIED");
                 neighborRemove(p);
                 p = nullptr;
             } else {
@@ -1129,7 +1128,7 @@ neighborsUdpAck(const cache_key * key, icp_common_t * header, const Ip::Address
 }
 
 CachePeer *
-peerFindByName(const char *name)
+findCachePeerByName(const char * const name)
 {
     CachePeer *p = nullptr;
 
@@ -1141,24 +1140,6 @@ peerFindByName(const char *name)
     return p;
 }
 
-CachePeer *
-peerFindByNameAndPort(const char *name, unsigned short port)
-{
-    CachePeer *p = nullptr;
-
-    for (p = Config.peers; p; p = p->next) {
-        if (strcasecmp(name, p->name))
-            continue;
-
-        if (port != p->http_port)
-            continue;
-
-        break;
-    }
-
-    return p;
-}
-
 int
 neighborUp(const CachePeer * p)
 {
@@ -1172,22 +1153,22 @@ neighborUp(const CachePeer * p)
      * for it.
      */
     if (0 == p->n_addresses) {
-        debugs(15, 8, "neighborUp: DOWN (no-ip): " << p->host << " (" << p->in_addr << ")");
+        debugs(15, 8, "DOWN (no-ip): " << *p);
         return 0;
     }
 
     if (p->options.no_query) {
-        debugs(15, 8, "neighborUp: UP (no-query): " << p->host << " (" << p->in_addr << ")");
+        debugs(15, 8, "UP (no-query): " << *p);
         return 1;
     }
 
     if (p->stats.probe_start != 0 &&
             squid_curtime - p->stats.probe_start > Config.Timeout.deadPeer) {
-        debugs(15, 8, "neighborUp: DOWN (dead): " << p->host << " (" << p->in_addr << ")");
+        debugs(15, 8, "DOWN (dead): " << *p);
         return 0;
     }
 
-    debugs(15, 8, "neighborUp: UP: " << p->host << " (" << p->in_addr << ")");
+    debugs(15, 8, "UP: " << *p);
     return 1;
 }
 
@@ -1206,7 +1187,7 @@ peerDNSConfigure(const ipcache_addrs *ia, const Dns::LookupDetails &, void *data
     CachePeer *p = (CachePeer *)data;
 
     if (p->n_addresses == 0) {
-        debugs(15, Important(29), "Configuring " << neighborTypeStr(p) << " " << p->host << "/" << p->http_port << "/" << p->icp.port);
+        debugs(15, Important(29), "Configuring " << neighborTypeStr(p) << " " << *p);
 
         if (p->type == PEER_MULTICAST)
             debugs(15, DBG_IMPORTANT, "    Multicast TTL = " << p->mcast.ttl);
@@ -1215,12 +1196,12 @@ peerDNSConfigure(const ipcache_addrs *ia, const Dns::LookupDetails &, void *data
     p->n_addresses = 0;
 
     if (ia == nullptr) {
-        debugs(0, DBG_CRITICAL, "WARNING: DNS lookup for '" << p->host << "' failed!");
+        debugs(0, DBG_CRITICAL, "WARNING: DNS lookup for '" << *p << "' failed!");
         return;
     }
 
     if (ia->empty()) {
-        debugs(0, DBG_CRITICAL, "WARNING: No IP address found for '" << p->host << "'!");
+        debugs(0, DBG_CRITICAL, "WARNING: No IP address found for '" << *p << "'!");
         return;
     }
 
@@ -1289,18 +1270,18 @@ peerConnectFailed(CachePeer * const p)
 
     const auto consideredAliveByAdmin = p->stats.logged_state == PEER_ALIVE;
     const auto level = consideredAliveByAdmin ? DBG_IMPORTANT : 2;
-    debugs(15, level, "ERROR: TCP connection to " << p->host << "/" << p->http_port << " failed");
+    debugs(15, level, "ERROR: TCP connection to " << *p << " failed");
 
     if (consideredAliveByAdmin) {
         if (!p->tcp_up) {
-            debugs(15, DBG_IMPORTANT, "Detected DEAD " << neighborTypeStr(p) << ": " << p->name);
+            debugs(15, DBG_IMPORTANT, "Detected DEAD " << neighborTypeStr(p) << ": " << *p);
             p->stats.logged_state = PEER_DEAD;
         } else {
             debugs(15, 2, "additional failures needed to mark this cache_peer DEAD: " << p->tcp_up);
         }
     } else {
         assert(!p->tcp_up);
-        debugs(15, 2, "cache_peer " << p->host << "/" << p->http_port << " is still DEAD");
+        debugs(15, 2, "cache_peer " << *p << " is still DEAD");
     }
 }
 
@@ -1308,7 +1289,7 @@ void
 peerConnectSucceded(CachePeer * p)
 {
     if (!p->tcp_up) {
-        debugs(15, 2, "TCP connection to " << p->host << "/" << p->http_port << " succeeded");
+        debugs(15, 2, "TCP connection to " << *p << " succeeded");
         p->tcp_up = p->connect_fail_limit; // NP: so peerAlive(p) works properly.
         peerAlive(p);
         if (!p->n_addresses)
@@ -1474,7 +1455,7 @@ peerCountMcastPeersAbort(PeerSelector * const psstate)
         CachePeer *p = (CachePeer *)psstate->peerCountMcastPeerXXX;
         p->mcast.flags.counting = false;
         p->mcast.avg_n_members = Math::doubleAverage(p->mcast.avg_n_members, (double) psstate->ping.n_recv, ++p->mcast.n_times_counted, 10);
-        debugs(15, DBG_IMPORTANT, "Group " << p->host  << ": " << psstate->ping.n_recv  <<
+        debugs(15, DBG_IMPORTANT, "Group " << *p  << ": " << psstate->ping.n_recv  <<
                " replies, "<< std::setw(4)<< std::setprecision(2) <<
                p->mcast.avg_n_members <<" average, RTT " << p->stats.rtt);
         p->mcast.n_replies_expected = (int) p->mcast.avg_n_members;
index 0cb792404b0e764c8aeb157b6b7a943af00cdc90..2510f66c7571a1e78899a8ce5fc05bbc5d2f00bb 100644 (file)
@@ -42,8 +42,10 @@ void neighbors_init(void);
 #if USE_HTCP
 void neighborsHtcpClear(StoreEntry *, HttpRequest *, const HttpRequestMethod &, htcp_clr_reason);
 #endif
-CachePeer *peerFindByName(const char *);
-CachePeer *peerFindByNameAndPort(const char *, unsigned short);
+
+/// cache_peer with a given name (or nil)
+CachePeer *findCachePeerByName(const char *);
+
 CachePeer *getDefaultParent(PeerSelector*);
 CachePeer *getRoundRobinParent(PeerSelector*);
 CachePeer *getWeightedRoundRobinParent(PeerSelector*);
index 8f810ad87c5444b0f5b1de367afb2543f4c4f5ba..1be19f7d2c2acb98deeb87e8b2f104febd277c03 100644 (file)
@@ -249,7 +249,7 @@ peerDigestCheck(void *data)
         return;
     }
 
-    debugs(72, 3, "peerDigestCheck: peer " <<  pd->peer->host << ":" << pd->peer->http_port);
+    debugs(72, 3, "cache_peer " << *pd->peer);
     debugs(72, 3, "peerDigestCheck: time: " << squid_curtime <<
            ", last received: " << (long int) pd->times.received << "  (" <<
            std::showpos << (int) (squid_curtime - pd->times.received) << ")");
index 86c728770f931684d5b88e283f4df3d987fbf401..057e62dd5121cfa438283734094a0824bc6d6679 100644 (file)
@@ -94,7 +94,7 @@ operator <<(std::ostream &os, const PeerSelectionDumper &fsd)
     os << hier_code_str[fsd.code];
 
     if (fsd.peer)
-        os << '/' << fsd.peer->host;
+        os << '/' << *fsd.peer;
     else if (fsd.selector) // useful for DIRECT and gone PINNED destinations
         os << '#' << fsd.selector->request->url.host();
 
index 7f4da0270fdb695fc83842a687463153b07aac03..9ca982686f3c08b2658058396d60400bf48f5441 100644 (file)
@@ -177,7 +177,7 @@ peerSourceHashSelectParent(PeerSelector *ps)
         combined_hash += combined_hash * 0x62531965;
         combined_hash = ROTATE_LEFT(combined_hash, 21);
         score = combined_hash * tp->sourcehash.load_multiplier;
-        debugs(39, 3, "peerSourceHashSelectParent: " << tp->name << " combined_hash " << combined_hash  <<
+        debugs(39, 3, *tp << " combined_hash " << combined_hash  <<
                " score " << std::setprecision(0) << score);
 
         if ((score > high_score) && peerHTTPOkay(tp, ps)) {
@@ -187,7 +187,7 @@ peerSourceHashSelectParent(PeerSelector *ps)
     }
 
     if (p)
-        debugs(39, 2, "peerSourceHashSelectParent: selected " << p->name);
+        debugs(39, 2, "selected " << *p);
 
     return p;
 }
index ede9560c47ace19afdb9547d0a959055e40aaec9..944cdb33699fa9feffe1eccfc8021d7178eae513 100644 (file)
@@ -185,7 +185,7 @@ peerUserHashSelectParent(PeerSelector *ps)
         combined_hash += combined_hash * 0x62531965;
         combined_hash = ROTATE_LEFT(combined_hash, 21);
         score = combined_hash * tp->userhash.load_multiplier;
-        debugs(39, 3, "peerUserHashSelectParent: " << tp->name << " combined_hash " << combined_hash  <<
+        debugs(39, 3, *tp << " combined_hash " << combined_hash <<
                " score " << std::setprecision(0) << score);
 
         if ((score > high_score) && peerHTTPOkay(tp, ps)) {
@@ -195,7 +195,7 @@ peerUserHashSelectParent(PeerSelector *ps)
     }
 
     if (p)
-        debugs(39, 2, "peerUserHashSelectParent: selected " << p->name);
+        debugs(39, 2, "selected " << *p);
 
     return p;
 }
index abb4ae2ec1f95e2c07abfca9d8f0e7fb7ff1f7d9..25af852dc07c2aa56108d71f3f29749ee245f42d 100644 (file)
@@ -12,6 +12,7 @@
 #include "tests/STUB.h"
 
 #include "CachePeer.h"
-
+void CachePeer::rename(const char *) STUB
 time_t CachePeer::connectTimeout() const STUB_RETVAL(0)
+std::ostream &operator <<(std::ostream &os, const CachePeer &) STUB_RETVAL(os)
 
index f6efc00148caf808bb3b799ac25b92ad4bace990..20821f2719d898a216fccc4150b7ce7f146ac57c 100644 (file)
@@ -16,6 +16,7 @@
 
 void
 peerConnClosed(CachePeer *) STUB
+CachePeer *findCachePeerByName(const char *) STUB_RETVAL(nullptr)
 
 time_t
 FwdState::ForwardTimeout(const time_t) STUB_RETVAL(0)