]> git.ipfire.org Git - thirdparty/squid.git/commit - src/ConfigParser.h
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)
commita555a85bc12e5a50db3305b70556d2807a42a7bb
tree1fced9f6b3ae438a0f6b90718d07962945f8096c
parentfab62eff0e238b5ca8b764ec0496324b299b39b8
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.
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