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.