Do not use raw pointers to index CARP CachePeers (#1381)

Simplified and improved code safety by using CbcPointers instead.

Also fixed mgr:carp Cache Manager reports to detail relevant
cache_peers instead of all cache_peers. When mgr:carp report was added
in 2000 commit 8ee9b49, Squid did not index (or even distinguish!)
CARP cache_peers, and the reporting loop naturally iterated through all
cache_peers. 2002 commit b399543 added identification and indexing of
CARP peers but forgot to adjust the reporting loop.

Very similar changes will be applied to userhash and sourcehash
cache_peers. 2008 commit 63104e2 simply copied problematic CARP code to
add userhash and sourcehash cache_peer support. This change adds a few
reusable types with those upcoming improvements in mind.

diff --git a/src/CachePeers.h b/src/CachePeers.h
new file mode 100644 (file)
index 0000000..e15d6d0
--- /dev/null
+++ b/src/CachePeers.h
@@ -0,0 +1,26 @@
+/*
+ * Copyright (C) 1996-2023 The Squid Software Foundation and contributors
+ *
+ * Squid software is distributed under GPLv2+ license and includes
+ * contributions from numerous individuals and organizations.
+ * Please see the COPYING and CONTRIBUTORS files for details.
+ */
+
+#ifndef SQUID_CACHEPEERS_H
+#define SQUID_CACHEPEERS_H
+
+#include "base/forward.h"
+#include "CachePeer.h"
+#include "mem/PoolingAllocator.h"
+
+#include <vector>
+
+/// Weak pointers to zero or more Config.peers.
+/// Users must specify the selection algorithm and the order of entries.
+using SelectedCachePeers = std::vector< CbcPointer<CachePeer>, PoolingAllocator< CbcPointer<CachePeer> > >;
+
+/// Temporary, local storage of raw pointers to zero or more Config.peers.
+using RawCachePeers = std::vector<CachePeer *, PoolingAllocator<CachePeer*> >;
+
+#endif /* SQUID_CACHEPEERS_H */
+

diff --git a/src/Makefile.am b/src/Makefile.am
index a29a69bf7b0b4e6aa83b4cacff282ac6c686c397..f2eee16555605293c2ecc51524cb34b32b14f709 100644 (file)
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -199,6 +199,7 @@ squid_SOURCES = \
        CacheManager.h \
        CachePeer.cc \
        CachePeer.h \
+       CachePeers.h \
        ClientInfo.h \
        ClientRequestContext.h \
        CollapsedForwarding.cc \

diff --git a/src/carp.cc b/src/carp.cc
index 126e443b09ef384a5670d31c16908a01486d1dd8..3313a0b93dd1b9163ba927970c6c089c0caf5162 100644 (file)
--- a/src/carp.cc
+++ b/src/carp.cc
@@ -10,6 +10,7 @@
 
 #include "squid.h"
 #include "CachePeer.h"
+#include "CachePeers.h"
 #include "carp.h"
 #include "HttpRequest.h"
 #include "mgr/Registration.h"
@@ -22,8 +23,9 @@
 
 #define ROTATE_LEFT(x, n) (((x) << (n)) | ((x) >> (32-(n))))
 
-static int n_carp_peers = 0;
-static CachePeer **carp_peers = nullptr;
+/// CARP cache_peers ordered by their CARP weight
+static SelectedCachePeers TheCarpPeers;
+
 static OBJH carpCachemgr;
 
 static int
@@ -44,27 +46,22 @@ void
 carpInit(void)
 {
     int W = 0;
-    int K;
-    int k;
     double P_last, X_last, Xn;
-    CachePeer *p;
-    CachePeer **P;
     char *t;
     /* Clean up */
 
-    for (k = 0; k < n_carp_peers; ++k) {
-        cbdataReferenceDone(carp_peers[k]);
-    }
-
-    safe_free(carp_peers);
-    n_carp_peers = 0;
+    TheCarpPeers.clear();
 
     /* initialize cache manager before we have a chance to leave the execution path */
     carpRegisterWithCacheManager();
 
     /* find out which peers we have */
 
-    for (p = Config.peers; p; p = p->next) {
+    RawCachePeers rawCarpPeers;
+    for (auto p = Config.peers; p; p = p->next) {
+        if (!cbdataReferenceValid(p))
+            continue;
+
         if (!p->options.carp)
             continue;
 
@@ -73,24 +70,16 @@ carpInit(void)
         if (p->weight == 0)
             continue;
 
-        ++n_carp_peers;
+        rawCarpPeers.push_back(p);
 
         W += p->weight;
     }
 
-    if (n_carp_peers == 0)
+    if (rawCarpPeers.empty())
         return;
 
-    carp_peers = (CachePeer **)xcalloc(n_carp_peers, sizeof(*carp_peers));
-
-    /* Build a list of the found peers and calculate hashes and load factors */
-    for (P = carp_peers, p = Config.peers; p; p = p->next) {
-        if (!p->options.carp)
-            continue;
-
-        if (p->weight == 0)
-            continue;
-
+    /* calculate hashes and load factors */
+    for (const auto p: rawCarpPeers) {
         /* calculate this peers hash */
         p->carp.hash = 0;
 
@@ -106,14 +95,10 @@ carpInit(void)
 
         if (floor(p->carp.load_factor * 1000.0) == 0.0)
             p->carp.load_factor = 0.0;
-
-        /* add it to our list of peers */
-        *P = cbdataReference(p);
-        ++P;
     }
 
     /* Sort our list on weight */
-    qsort(carp_peers, n_carp_peers, sizeof(*carp_peers), peerSortWeight);
+    qsort(rawCarpPeers.data(), rawCarpPeers.size(), sizeof(decltype(rawCarpPeers)::value_type), peerSortWeight);
 
     /* Calculate the load factor multipliers X_k
      *
@@ -123,7 +108,7 @@ carpInit(void)
      * X_k = pow (X_k, {1/(K-k+1)})
      * simplified to have X_1 part of the loop
      */
-    K = n_carp_peers;
+    const auto K = rawCarpPeers.size();
 
     P_last = 0.0;       /* Empty P_0 */
 
@@ -131,9 +116,9 @@ carpInit(void)
 
     X_last = 0.0;       /* Empty X_0, nullifies the first pow statement */
 
-    for (k = 1; k <= K; ++k) {
+    for (size_t k = 1; k <= K; ++k) {
         double Kk1 = (double) (K - k + 1);
-        p = carp_peers[k - 1];
+        const auto p = rawCarpPeers[k - 1];
         p->carp.load_multiplier = (Kk1 * (p->carp.load_factor - P_last)) / Xn;
         p->carp.load_multiplier += pow(X_last, Kk1);
         p->carp.load_multiplier = pow(p->carp.load_multiplier, 1.0 / Kk1);
@@ -141,6 +126,8 @@ carpInit(void)
         X_last = p->carp.load_multiplier;
         P_last = p->carp.load_factor;
     }
+
+    TheCarpPeers.assign(rawCarpPeers.begin(), rawCarpPeers.end());
 }
 
 CachePeer *
@@ -149,24 +136,24 @@ carpSelectParent(PeerSelector *ps)
     assert(ps);
     HttpRequest *request = ps->request;
 
-    int k;
     CachePeer *p = nullptr;
-    CachePeer *tp;
     unsigned int user_hash = 0;
     unsigned int combined_hash;
     double score;
     double high_score = 0;
 
-    if (n_carp_peers == 0)
+    if (TheCarpPeers.empty())
         return nullptr;
 
     /* calculate hash key */
     debugs(39, 2, "carpSelectParent: Calculating hash for " << request->effectiveRequestUri());
 
     /* select CachePeer */
-    for (k = 0; k < n_carp_peers; ++k) {
+    for (const auto &tp: TheCarpPeers) {
+        if (!tp)
+            continue; // peer gone
+
         SBuf key;
-        tp = carp_peers[k];
         if (tp->options.carp_key.set) {
             // this code follows URI syntax pattern.
             // corner cases should use the full effective request URI
@@ -208,8 +195,8 @@ carpSelectParent(PeerSelector *ps)
         debugs(39, 3, *tp << " key=" << key << " combined_hash=" << combined_hash  <<
                " score=" << std::setprecision(0) << score);
 
-        if ((score > high_score) && peerHTTPOkay(tp, ps)) {
-            p = tp;
+        if ((score > high_score) && peerHTTPOkay(tp.get(), ps)) {
+            p = tp.get();
             high_score = score;
         }
     }
@@ -223,7 +210,6 @@ carpSelectParent(PeerSelector *ps)
 static void
 carpCachemgr(StoreEntry * sentry)
 {
-    CachePeer *p;
     int sumfetches = 0;
     storeAppendPrintf(sentry, "%24s %10s %10s %10s %10s\n",
                       "Hostname",
@@ -232,10 +218,15 @@ carpCachemgr(StoreEntry * sentry)
                       "Factor",
                       "Actual");
 
-    for (p = Config.peers; p; p = p->next)
+    for (const auto &p: TheCarpPeers) {
+        if (!p)
+            continue;
         sumfetches += p->stats.fetches;
+    }
 
-    for (p = Config.peers; p; p = p->next) {
+    for (const auto &p: TheCarpPeers) {
+        if (!p)
+            continue;
         storeAppendPrintf(sentry, "%24s %10x %10f %10f %10f\n",
                           p->name, p->carp.hash,
                           p->carp.load_multiplier,