Refactor X-Forwarded-For stats (#834)

Use std::unordered_map instead of hash_table
Clean up unused code
Rename a few functions to better reflect name

Test plan:

$ squidclient mgr:forw_headers
HTTP/1.1 200 OK
Server: squid/6.0.0-VCS

       10 foo
       15 baz
        7 bar

diff --git a/src/client_side_request.cc b/src/client_side_request.cc
index 6b9478320129cd1db54e6d3a1b3def3a1cb751fd..1fe241f9dbb04ba271408decc3a8c3f09cc4069c 100644 (file)
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -1136,7 +1136,7 @@ clientInterpretRequestHeaders(ClientHttpRequest * http)
 
     if (req_hdr->has(Http::HdrType::X_FORWARDED_FOR)) {
         String s = req_hdr->getList(Http::HdrType::X_FORWARDED_FOR);
-        fvdbCountForw(s.termedBuf());
+        fvdbCountForwarded(StringToSBuf(s));
         s.clean();
     }
 

diff --git a/src/log/access_log.cc b/src/log/access_log.cc
index 00b1efcb049b5fa6b98fa7b429fcdb768335ed2e..9e72c95893d7d7afa316ee3727479303ecd47581 100644 (file)
--- a/src/log/access_log.cc
+++ b/src/log/access_log.cc
@@ -57,25 +57,17 @@ static void mcast_encode(unsigned int *, size_t, const unsigned int *);
 
 #if USE_FORW_VIA_DB
 
-typedef struct {
-    hash_link hash;
-    int n;
-} fvdb_entry;
-
 using HeaderValueCountsElement = std::pair<SBuf, uint64_t>;
 /// counts the number of header field value occurrences
 using HeaderValueCounts = std::unordered_map<SBuf, uint64_t, std::hash<SBuf>, std::equal_to<SBuf>, PoolingAllocator<HeaderValueCountsElement> >;
 
 /// counts the number of HTTP Via header field value occurrences
 static HeaderValueCounts TheViaCounts;
+/// counts the number of HTTP X-Forwarded-For header field value occurrences
+static HeaderValueCounts TheForwardedCounts;
 
-static hash_table *forw_table = NULL;
-static void fvdbInit();
-static void fvdbDumpTable(StoreEntry * e, hash_table * hash);
-static void fvdbCount(hash_table * hash, const char *key);
 static OBJH fvdbDumpVia;
-static OBJH fvdbDumpForw;
-static FREE fvdbFreeEntry;
+static OBJH fvdbDumpForwarded;
 static void fvdbClear(void);
 static void fvdbRegisterWithCacheManager();
 #endif
@@ -442,47 +434,17 @@ accessLogInit(void)
             fatal("mcast_encode_key is too short, must be 16 characters");
     }
 
-#endif
-#if USE_FORW_VIA_DB
-
-    fvdbInit();
-
 #endif
 }
 
 #if USE_FORW_VIA_DB
 
-static void
-fvdbInit(void)
-{
-    forw_table = hash_create((HASHCMP *) strcmp, 977, hash4);
-}
-
 static void
 fvdbRegisterWithCacheManager(void)
 {
     Mgr::RegisterAction("via_headers", "Via Request Headers", fvdbDumpVia, 0, 1);
     Mgr::RegisterAction("forw_headers", "X-Forwarded-For Request Headers",
-                        fvdbDumpForw, 0, 1);
-}
-
-static void
-fvdbCount(hash_table * hash, const char *key)
-{
-    fvdb_entry *fv;
-
-    if (NULL == hash)
-        return;
-
-    fv = (fvdb_entry *)hash_lookup(hash, key);
-
-    if (NULL == fv) {
-        fv = static_cast <fvdb_entry *>(xcalloc(1, sizeof(fvdb_entry)));
-        fv->hash.key = xstrdup(key);
-        hash_join(hash, &fv->hash);
-    }
-
-    ++ fv->n;
+                        fvdbDumpForwarded, 0, 1);
 }
 
 void
@@ -492,26 +454,9 @@ fvdbCountVia(const SBuf &headerValue)
 }
 
 void
-fvdbCountForw(const char *key)
-{
-    fvdbCount(forw_table, key);
-}
-
-static void
-fvdbDumpTable(StoreEntry * e, hash_table * hash)
+fvdbCountForwarded(const SBuf &key)
 {
-    hash_link *h;
-    fvdb_entry *fv;
-
-    if (hash == NULL)
-        return;
-
-    hash_first(hash);
-
-    while ((h = hash_next(hash))) {
-        fv = (fvdb_entry *) h;
-        storeAppendPrintf(e, "%9d %s\n", fv->n, hashKeyStr(&fv->hash));
-    }
+    ++TheForwardedCounts[key];
 }
 
 static void
@@ -530,27 +475,17 @@ fvdbDumpVia(StoreEntry * e)
 }
 
 static void
-fvdbDumpForw(StoreEntry * e)
-{
-    fvdbDumpTable(e, forw_table);
-}
-
-static
-void
-fvdbFreeEntry(void *data)
+fvdbDumpForwarded(StoreEntry * e)
 {
-    fvdb_entry *fv = static_cast <fvdb_entry *>(data);
-    xfree(fv->hash.key);
-    xfree(fv);
+    assert(e);
+    fvdbDumpCounts(*e, TheForwardedCounts);
 }
 
 static void
 fvdbClear(void)
 {
     TheViaCounts.clear();
-    hashFreeItems(forw_table, fvdbFreeEntry);
-    hashFreeMemory(forw_table);
-    forw_table = hash_create((HASHCMP *) strcmp, 977, hash4);
+    TheForwardedCounts.clear();
 }
 
 #endif

diff --git a/src/log/access_log.h b/src/log/access_log.h
index 4f973865032863e533d48f0445c1e2816071e96e..a66ee55ba9d229d2fe88306328cbf520ebb3ff02 100644 (file)
--- a/src/log/access_log.h
+++ b/src/log/access_log.h
@@ -14,11 +14,11 @@
 #include "LogTags.h"
 #include "sbuf/forward.h"
 
+/// XXX: these functions preserve all counted values until the next log rotation
 /// count occurrences of the given Via header value
-/// XXX: this function preserves all counted values until the next log rotation
 void fvdbCountVia(const SBuf &);
-
-void fvdbCountForw(const char *key);
+/// count occurrences of the given X-Forwarded-For header value
+void fvdbCountForwarded(const SBuf &);
 
 #if HEADERS_LOG
 class HttpRequestMethod;

diff --git a/src/tests/stub_liblog.cc b/src/tests/stub_liblog.cc
index acc36238371b1844a42da4e60937c0a3b7af2d4a..3fb4304bc84ca46bdf8b03c80abfa90873697aa6 100644 (file)
--- a/src/tests/stub_liblog.cc
+++ b/src/tests/stub_liblog.cc
@@ -31,7 +31,7 @@ const char *accessLogTime(time_t) STUB_RETVAL(nullptr)
 
 #include "log/access_log.h"
 void fvdbCountVia(const SBuf &) STUB
-void fvdbCountForw(const char *) STUB
+void fvdbCountForward(const SBuf &) STUB
 #if HEADERS_LOG
 void headersLog(int, int, const HttpRequestMethod &, void *) STUB
 #endif