From: Alex Rousskov Date: Wed, 9 Nov 2022 21:29:33 +0000 (+0000) Subject: Improve cache_peer reporting and NetDB cache_peer search (#1174) X-Git-Tag: SQUID_6_0_1~80 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=a555a85bc12e5a50db3305b70556d2807a42a7bb;p=thirdparty%2Fsquid.git Improve cache_peer reporting and NetDB cache_peer search (#1174) 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. --- diff --git a/doc/debug-messages.dox b/doc/debug-messages.dox index f5713e964c..f870823848 100644 --- a/doc/debug-messages.dox +++ b/doc/debug-messages.dox @@ -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: ... diff --git a/src/CachePeer.cc b/src/CachePeer.cc index bad71c58c1..24bf56714f 100644 --- a/src/CachePeer.cc +++ b/src/CachePeer.cc @@ -14,9 +14,17 @@ #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; +} + diff --git a/src/CachePeer.h b/src/CachePeer.h index 76d1e48193..2beb460982 100644 --- a/src/CachePeer.h +++ b/src/CachePeer.h @@ -16,6 +16,8 @@ #include "ip/Address.h" #include "security/PeerOptions.h" +#include + //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_ */ diff --git a/src/ConfigParser.cc b/src/ConfigParser.cc index a2d51ab3b7..5dce42b5fe 100644 --- a/src/ConfigParser.cc +++ b/src/ConfigParser.cc @@ -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(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() { diff --git a/src/ConfigParser.h b/src/ConfigParser.h index 8347dd4780..8f20563920 100644 --- a/src/ConfigParser.h +++ b/src/ConfigParser.h @@ -19,6 +19,7 @@ #include #include +class CachePeer; class wordlist; /** @@ -76,6 +77,9 @@ public: /// extracts and returns a regex (including any optional flags) std::unique_ptr 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); diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc index 0920f08a0c..ed4c3e30f8 100644 --- a/src/HttpRequest.cc +++ b/src/HttpRequest.cc @@ -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 diff --git a/src/Makefile.am b/src/Makefile.am index 58c576dc29..d1498f260f 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -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) \ diff --git a/src/base/IoManip.h b/src/base/IoManip.h index f13c697b62..bb60a8b673 100644 --- a/src/base/IoManip.h +++ b/src/base/IoManip.h @@ -25,12 +25,19 @@ public: /// Report the pointed-to-object on a dedicated Debug::Extra line. RawPointerT &asExtra() { onExtraLine = true; return *this; } + /// enable and, optionally, customize reporting of nil pointers + RawPointerT &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 inline RawPointerT RawPointer(const char *label, const Pointer &ptr) @@ -38,13 +45,24 @@ RawPointer(const char *label, const Pointer &ptr) return RawPointerT(label, ptr); } +/// convenience wrapper for creating RawPointerT<> objects without a label +template +inline RawPointerT +RawPointer(const Pointer &ptr) +{ + return RawPointerT(nullptr, ptr); +} + /// prints RawPointerT<>, dereferencing the io_manip pointer if possible template inline std::ostream & operator <<(std::ostream &os, const RawPointerT &pd) { - if (!pd.ptr) + if (!pd.ptr) { + if (pd.nilText) + os << pd.nilText; return os; + } if (pd.onExtraLine) os << Debug::Extra; diff --git a/src/cache_cf.cc b/src/cache_cf.cc index 50392b5a0f..dbfb5c476f 100644 --- a/src/cache_cf.cc +++ b/src/cache_cf.cc @@ -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(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; } } diff --git a/src/carp.cc b/src/carp.cc index 562e8ab914..e699fcf38c 100644 --- a/src/carp.cc +++ b/src/carp.cc @@ -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; } diff --git a/src/cf.data.pre b/src/cf.data.pre index 5971483bea..751d47cf9d 100644 --- a/src/cf.data.pre +++ b/src/cf.data.pre @@ -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 diff --git a/src/icmp/net_db.cc b/src/icmp/net_db.cc index cda165551e..ced1a37041 100644 --- a/src/icmp/net_db.cc +++ b/src/icmp/net_db.cc @@ -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 diff --git a/src/icmp/net_db.h b/src/icmp/net_db.h index 68a1b5af54..16a98afa79 100644 --- a/src/icmp/net_db.h +++ b/src/icmp/net_db.h @@ -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; diff --git a/src/neighbors.cc b/src/neighbors.cc index 0bc3a87c9a..2f9b25bf17 100644 --- a/src/neighbors.cc +++ b/src/neighbors.cc @@ -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 : "") << ", 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; diff --git a/src/neighbors.h b/src/neighbors.h index 0cb792404b..2510f66c75 100644 --- a/src/neighbors.h +++ b/src/neighbors.h @@ -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*); diff --git a/src/peer_digest.cc b/src/peer_digest.cc index 8f810ad87c..1be19f7d2c 100644 --- a/src/peer_digest.cc +++ b/src/peer_digest.cc @@ -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) << ")"); diff --git a/src/peer_select.cc b/src/peer_select.cc index 86c728770f..057e62dd51 100644 --- a/src/peer_select.cc +++ b/src/peer_select.cc @@ -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(); diff --git a/src/peer_sourcehash.cc b/src/peer_sourcehash.cc index 7f4da0270f..9ca982686f 100644 --- a/src/peer_sourcehash.cc +++ b/src/peer_sourcehash.cc @@ -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; } diff --git a/src/peer_userhash.cc b/src/peer_userhash.cc index ede9560c47..944cdb3369 100644 --- a/src/peer_userhash.cc +++ b/src/peer_userhash.cc @@ -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; } diff --git a/src/tests/stub_CachePeer.cc b/src/tests/stub_CachePeer.cc index abb4ae2ec1..25af852dc0 100644 --- a/src/tests/stub_CachePeer.cc +++ b/src/tests/stub_CachePeer.cc @@ -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) diff --git a/src/tests/stub_neighbors.cc b/src/tests/stub_neighbors.cc index f6efc00148..20821f2719 100644 --- a/src/tests/stub_neighbors.cc +++ b/src/tests/stub_neighbors.cc @@ -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)