Convert Vary handling to SBuf

diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc
index 6d75c203050577e5a71be9af3f230c2cdd02a02d..604bb5e0f2f4b72d861c9a372189a68e1f8ae22d 100644 (file)
--- a/src/HttpRequest.cc
+++ b/src/HttpRequest.cc
@@ -89,7 +89,7 @@ HttpRequest::init()
     peer_login = NULL;      // not allocated/deallocated by this class
     peer_domain = NULL;     // not allocated/deallocated by this class
     peer_host = NULL;
-    vary_headers = NULL;
+    vary_headers = SBuf();
     myportname = null_string;
     tag = null_string;
 #if USE_AUTH
@@ -121,8 +121,7 @@ HttpRequest::clean()
 #if USE_AUTH
     auth_user_request = NULL;
 #endif
-    safe_free(vary_headers);
-
+    vary_headers.clear();
     url.clear();
 
     header.clean();
@@ -197,7 +196,7 @@ HttpRequest::clone() const
 
     copy->lastmod = lastmod;
     copy->etag = etag;
-    copy->vary_headers = vary_headers ? xstrdup(vary_headers) : NULL;
+    copy->vary_headers = vary_headers;
     // XXX: what to do with copy->peer_domain?
 
     copy->tag = tag;

diff --git a/src/HttpRequest.h b/src/HttpRequest.h
index b4653f6cbe3797bafd2c874e36b7ef0b8550c680..4651839c6ba0e26c0bd32eb814a2b497fa8e4f7f 100644 (file)
--- a/src/HttpRequest.h
+++ b/src/HttpRequest.h
@@ -148,7 +148,8 @@ public:
 
     time_t lastmod;     /* Used on refreshes */
 
-    const char *vary_headers;   /* Used when varying entities are detected. Changes how the store key is calculated */
+    /// The variant second-stage cache key. Generated from Vary header pattern for this request.
+    SBuf vary_headers;
 
     char *peer_domain;      /* Configured peer forceddomain */
 

diff --git a/src/MemObject.cc b/src/MemObject.cc
index 8a9ada8a0b0dc9b9cb378cb1d509851956bfcbe2..50f0a03d1f9c0bcb786511f66b4d9015bc51ea68 100644 (file)
--- a/src/MemObject.cc
+++ b/src/MemObject.cc
@@ -145,8 +145,6 @@ MemObject::~MemObject()
     HTTPMSGUNLOCK(request);
 
     ctx_exit(ctx);              /* must exit before we free mem->url */
-
-    safe_free(vary_headers);
 }
 
 void
@@ -230,8 +228,8 @@ void
 MemObject::stat(MemBuf * mb) const
 {
     mb->appendf("\t" SQUIDSBUFPH " %s\n", SQUIDSBUFPRINT(method.image()), logUri());
-    if (vary_headers)
-        mb->appendf("\tvary_headers: %s\n", vary_headers);
+    if (!vary_headers.isEmpty())
+        mb->appendf("\tvary_headers: " SQUIDSBUFPH "\n", SQUIDSBUFPRINT(vary_headers));
     mb->appendf("\tinmem_lo: %" PRId64 "\n", inmem_lo);
     mb->appendf("\tinmem_hi: %" PRId64 "\n", data_hdr.endOffset());
     mb->appendf("\tswapout: %" PRId64 " bytes queued\n", swapout.queue_offset);

diff --git a/src/MemObject.h b/src/MemObject.h
index 6bca732361e86f49924eec78577185d9553315c4..04139750e560d20e189cfd034378de99428c7b3b 100644 (file)
--- a/src/MemObject.h
+++ b/src/MemObject.h
@@ -13,6 +13,7 @@
 #include "dlink.h"
 #include "http/RequestMethod.h"
 #include "RemovalPolicy.h"
+#include "sbuf/SBuf.h"
 #include "SquidString.h"
 #include "stmem.h"
 #include "StoreIOBuffer.h"
@@ -170,7 +171,7 @@ public:
     unsigned int chksum;
 #endif
 
-    const char *vary_headers;
+    SBuf vary_headers;
 
     void delayRead(DeferredRead const &);
     void kickReads();

diff --git a/src/MemStore.cc b/src/MemStore.cc
index 8393dda240f4d5e50390a73867290869aa017903..259fbec675ac6345458e27b2d9e56cfd9d10c588 100644 (file)
--- a/src/MemStore.cc
+++ b/src/MemStore.cc
@@ -618,7 +618,7 @@ MemStore::shouldCache(StoreEntry &e) const
 
     assert(e.mem_obj);
 
-    if (e.mem_obj->vary_headers) {
+    if (!e.mem_obj->vary_headers.isEmpty()) {
         // XXX: We must store/load SerialisedMetaData to cache Vary in RAM
         debugs(20, 5, "Vary not yet supported: " << e.mem_obj->vary_headers);
         return false;

diff --git a/src/StoreMetaVary.cc b/src/StoreMetaVary.cc
index 343b55ae59ac21e870eabea15e8cd0f83a277b1f..bd8423ff55c1c29188eb93195944c95b1080333e 100644 (file)
--- a/src/StoreMetaVary.cc
+++ b/src/StoreMetaVary.cc
@@ -18,14 +18,14 @@ StoreMetaVary::checkConsistency(StoreEntry *e) const
 {
     assert (getType() == STORE_META_VARY_HEADERS);
 
-    if (!e->mem_obj->vary_headers) {
+    if (e->mem_obj->vary_headers.isEmpty()) {
         /* XXX separate this mutator from the query */
         /* Assume the object is OK.. remember the vary request headers */
-        e->mem_obj->vary_headers = xstrdup((char *)value);
+        e->mem_obj->vary_headers.assign(static_cast<const char *>(value), length);
         return true;
     }
 
-    if (strcmp(e->mem_obj->vary_headers, (char *)value) != 0)
+    if (e->mem_obj->vary_headers.cmp(static_cast<const char *>(value), length) != 0)
         return false;
 
     return true;

diff --git a/src/client_side.cc b/src/client_side.cc
index 9df9c99ba79a312030103da30099b87a4f849734..980c7b95a1a7037e385e4d28c5a0da574a0b2877 100644 (file)
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -3570,7 +3570,7 @@ clientConnectionsClose()
 int
 varyEvaluateMatch(StoreEntry * entry, HttpRequest * request)
 {
-    const char *vary = request->vary_headers;
+    SBuf vary(request->vary_headers);
     int has_vary = entry->getReply()->header.has(Http::HdrType::VARY);
 #if X_ACCELERATOR_VARY
 
@@ -3578,12 +3578,12 @@ varyEvaluateMatch(StoreEntry * entry, HttpRequest * request)
         entry->getReply()->header.has(Http::HdrType::HDR_X_ACCELERATOR_VARY);
 #endif
 
-    if (!has_vary || !entry->mem_obj->vary_headers) {
-        if (vary) {
+    if (!has_vary || entry->mem_obj->vary_headers.isEmpty()) {
+        if (!vary.isEmpty()) {
             /* Oops... something odd is going on here.. */
             debugs(33, DBG_IMPORTANT, "varyEvaluateMatch: Oops. Not a Vary object on second attempt, '" <<
                    entry->mem_obj->urlXXX() << "' '" << vary << "'");
-            safe_free(request->vary_headers);
+            request->vary_headers.clear();
             return VARY_CANCEL;
         }
 
@@ -3597,8 +3597,8 @@ varyEvaluateMatch(StoreEntry * entry, HttpRequest * request)
          */
         vary = httpMakeVaryMark(request, entry->getReply());
 
-        if (vary) {
-            request->vary_headers = xstrdup(vary);
+        if (!vary.isEmpty()) {
+            request->vary_headers = vary;
             return VARY_OTHER;
         } else {
             /* Ouch.. we cannot handle this kind of variance */
@@ -3606,18 +3606,18 @@ varyEvaluateMatch(StoreEntry * entry, HttpRequest * request)
             return VARY_CANCEL;
         }
     } else {
-        if (!vary) {
+        if (vary.isEmpty()) {
             vary = httpMakeVaryMark(request, entry->getReply());
 
-            if (vary)
-                request->vary_headers = xstrdup(vary);
+            if (!vary.isEmpty())
+                request->vary_headers = vary;
         }
 
-        if (!vary) {
+        if (vary.isEmpty()) {
             /* Ouch.. we cannot handle this kind of variance */
             /* XXX This cannot really happen, but just to be complete */
             return VARY_CANCEL;
-        } else if (strcmp(vary, entry->mem_obj->vary_headers) == 0) {
+        } else if (vary.cmp(entry->mem_obj->vary_headers) == 0) {
             return VARY_MATCH;
         } else {
             /* Oops.. we have already been here and still haven't

diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc
index 32fb00055d783afc6c6c7cf1eb7c58decdf94967..507142eba591c8265d4552f5cec1c9047cb1a345 100644 (file)
--- a/src/client_side_reply.cc
+++ b/src/client_side_reply.cc
@@ -1007,9 +1007,8 @@ clientReplyContext::purgeDoPurgeHead(StoreEntry *newEntry)
     }
 
     /* And for Vary, release the base URI if none of the headers was included in the request */
-
-    if (http->request->vary_headers
-            && !strstr(http->request->vary_headers, "=")) {
+    if (!http->request->vary_headers.isEmpty()
+            && http->request->vary_headers.find('=') != SBuf::npos) {
         // XXX: performance regression, c_str() reallocates
         SBuf tmp(http->request->effectiveRequestUri());
         StoreEntry *entry = storeGetPublic(tmp.c_str(), Http::METHOD_GET);

diff --git a/src/http.cc b/src/http.cc
index a54804b28bc84edab41fc9ec75d4f8cfebaff74f..fcfea3d963f4111f5cc11f79b12ce4da1bf0060d 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -576,9 +576,9 @@ HttpStateData::cacheableReply()
 /*
  * For Vary, store the relevant request headers as
  * virtual headers in the reply
- * Returns false if the variance cannot be stored
+ * Returns an empty SBuf if the variance cannot be stored
  */
-const char *
+SBuf
 httpMakeVaryMark(HttpRequest * request, HttpReply const * reply)
 {
     String vary, hdr;
@@ -586,20 +586,21 @@ httpMakeVaryMark(HttpRequest * request, HttpReply const * reply)
     const char *item;
     const char *value;
     int ilen;
-    static String vstr;
+    SBuf vstr;
+    static const SBuf asterisk("*");
 
-    vstr.clean();
     vary = reply->header.getList(Http::HdrType::VARY);
 
     while (strListGetItem(&vary, ',', &item, &ilen, &pos)) {
-        static const SBuf asterisk("*");
         SBuf name(item, ilen);
         if (name == asterisk) {
-            vstr.clean();
+            vstr.clear();
             break;
         }
         name.toLower();
-        strListAdd(&vstr, name.c_str(), ',');
+        if (!vstr.isEmpty())
+            vstr.append(", ", 2);
+        vstr.append(name);
         hdr = request->header.getByName(name);
         value = hdr.termedBuf();
         if (value) {
@@ -619,10 +620,15 @@ httpMakeVaryMark(HttpRequest * request, HttpReply const * reply)
     vary = reply->header.getList(Http::HdrType::HDR_X_ACCELERATOR_VARY);
 
     while (strListGetItem(&vary, ',', &item, &ilen, &pos)) {
-        char *name = (char *)xmalloc(ilen + 1);
-        xstrncpy(name, item, ilen + 1);
-        Tolower(name);
-        strListAdd(&vstr, name, ',');
+        SBuf name(item, ilen);
+        if (name == asterisk) {
+            vstr.clear();
+            break;
+        }
+        name.toLower();
+        if (!vstr.isEmpty())
+            vstr.append(", ", 2);
+        vstr.append(name);
         hdr = request->header.getByName(name);
         safe_free(name);
         value = hdr.termedBuf();
@@ -640,8 +646,8 @@ httpMakeVaryMark(HttpRequest * request, HttpReply const * reply)
     vary.clean();
 #endif
 
-    debugs(11, 3, "httpMakeVaryMark: " << vstr);
-    return vstr.termedBuf();
+    debugs(11, 3, vstr);
+    return vstr;
 }
 
 void
@@ -942,15 +948,15 @@ HttpStateData::haveParsedReplyHeaders()
             || rep->header.has(Http::HdrType::HDR_X_ACCELERATOR_VARY)
 #endif
        ) {
-        const char *vary = httpMakeVaryMark(request, rep);
+        const SBuf vary(httpMakeVaryMark(request, rep));
 
-        if (!vary) {
+        if (vary.isEmpty()) {
             entry->makePrivate();
             if (!fwd->reforwardableStatus(rep->sline.status()))
                 EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT);
             varyFailure = true;
         } else {
-            entry->mem_obj->vary_headers = xstrdup(vary);
+            entry->mem_obj->vary_headers = vary;
         }
     }
 

diff --git a/src/http.h b/src/http.h
index 5eb40745f11124f9739f7bc28fc59159c776dc43..c115e08a59f3b3c0087145060fa6ad0b330ff1df 100644 (file)
--- a/src/http.h
+++ b/src/http.h
@@ -13,6 +13,7 @@
 #include "comm.h"
 #include "http/forward.h"
 #include "HttpStateFlags.h"
+#include "sbuf/SBuf.h"
 
 class FwdState;
 class HttpHeader;
@@ -132,7 +133,7 @@ private:
 
 int httpCachable(const HttpRequestMethod&);
 void httpStart(FwdState *);
-const char *httpMakeVaryMark(HttpRequest * request, HttpReply const * reply);
+SBuf httpMakeVaryMark(HttpRequest * request, HttpReply const * reply);
 
 #endif /* SQUID_HTTP_H */
 

diff --git a/src/ipc/mem/Segment.cc b/src/ipc/mem/Segment.cc
index 3cc6e0cf3147ec1118199a3b71bf4cdc37ae0d52..be8142f93885b16bfbad011c0d9347ebac3534d2 100644 (file)
--- a/src/ipc/mem/Segment.cc
+++ b/src/ipc/mem/Segment.cc
@@ -209,7 +209,7 @@ Ipc::Mem::Segment::lock()
     if (mlock(theMem, theSize) != 0) {
         const int savedError = errno;
         fatalf("shared_memory_locking on but failed to mlock(%s, %" PRId64 "): %s\n",
-               theName.termedBuf(), theSize, xstrerr(savedError));
+               theName.termedBuf(),static_cast<int64_t>(theSize), xstrerr(savedError));
     }
     // TODO: Warn if it took too long.
     debugs(54, 7, "mlock(" << theName << ',' << theSize << ") OK");

diff --git a/src/store.cc b/src/store.cc
index aff4dfe81236581b0ff15e1751dbf04f9ca4f5b0..60aeaf7bfb601744739446c1f3f8948444226a76 100644 (file)
--- a/src/store.cc
+++ b/src/store.cc
@@ -658,31 +658,27 @@ StoreEntry::setPublicKey()
     if (mem_obj->request) {
         HttpRequest *request = mem_obj->request;
 
-        if (!mem_obj->vary_headers) {
+        if (mem_obj->vary_headers.isEmpty()) {
             /* First handle the case where the object no longer varies */
-            safe_free(request->vary_headers);
+            request->vary_headers.clear();
         } else {
-            if (request->vary_headers && strcmp(request->vary_headers, mem_obj->vary_headers) != 0) {
+            if (!request->vary_headers.isEmpty() && request->vary_headers.cmp(mem_obj->vary_headers) != 0) {
                 /* Oops.. the variance has changed. Kill the base object
                  * to record the new variance key
                  */
-                safe_free(request->vary_headers);       /* free old "bad" variance key */
+                request->vary_headers.clear();       /* free old "bad" variance key */
                 if (StoreEntry *pe = storeGetPublic(mem_obj->storeId(), mem_obj->method))
                     pe->release();
             }
 
             /* Make sure the request knows the variance status */
-            if (!request->vary_headers) {
-                const char *vary = httpMakeVaryMark(request, mem_obj->getReply());
-
-                if (vary)
-                    request->vary_headers = xstrdup(vary);
-            }
+            if (request->vary_headers.isEmpty())
+                request->vary_headers = httpMakeVaryMark(request, mem_obj->getReply());
         }
 
         // TODO: storeGetPublic() calls below may create unlocked entries.
         // We should add/use storeHas() API or lock/unlock those entries.
-        if (mem_obj->vary_headers && !storeGetPublic(mem_obj->storeId(), mem_obj->method)) {
+        if (!mem_obj->vary_headers.isEmpty() && !storeGetPublic(mem_obj->storeId(), mem_obj->method)) {
             /* Create "vary" base object */
             String vary;
             StoreEntry *pe = storeCreateEntry(mem_obj->storeId(), mem_obj->logUri(), request->flags, request->method);

diff --git a/src/store_key_md5.cc b/src/store_key_md5.cc
index c28352ffe1c729a3e823658b07ab4bb102084ba8..36ba631783b6c505334e48a68d1d337f0bbbfcf7 100644 (file)
--- a/src/store_key_md5.cc
+++ b/src/store_key_md5.cc
@@ -124,8 +124,8 @@ storeKeyPublicByRequestMethod(HttpRequest * request, const HttpRequestMethod& me
     SquidMD5Update(&M, &m, sizeof(m));
     SquidMD5Update(&M, (unsigned char *) url.rawContent(), url.length());
 
-    if (request->vary_headers) {
-        SquidMD5Update(&M, (unsigned char *) request->vary_headers, strlen(request->vary_headers));
+    if (!request->vary_headers.isEmpty()) {
+        SquidMD5Update(&M, request->vary_headers.rawContent(), request->vary_headers.length());
         debugs(20, 3, "updating public key by vary headers: " << request->vary_headers << " for: " << url);
     }
 

diff --git a/src/store_swapmeta.cc b/src/store_swapmeta.cc
index 9ffb4ace9522171e3ebb60d7108f6c347934f172..7e5dd0fe1928cb799358876085c417287c21e076 100644 (file)
--- a/src/store_swapmeta.cc
+++ b/src/store_swapmeta.cc
@@ -39,7 +39,6 @@ storeSwapMetaBuild(StoreEntry * e)
 {
     tlv *TLV = NULL;        /* we'll return this */
     tlv **T = &TLV;
-    const char *vary;
     assert(e->mem_obj != NULL);
     const int64_t objsize = e->mem_obj->expectedReplySize();
 
@@ -87,10 +86,12 @@ storeSwapMetaBuild(StoreEntry * e)
     }
 
     T = StoreMeta::Add(T, t);
-    vary = e->mem_obj->vary_headers;
+    SBuf vary(e->mem_obj->vary_headers);
 
-    if (vary) {
-        t =StoreMeta::Factory(STORE_META_VARY_HEADERS, strlen(vary) + 1, vary);
+    if (!vary.isEmpty()) {
+        // TODO: do we still need +1 here? StoreMetaVary::checkConsistency
+        //       no longer relies on nul-termination, but other things might.
+        t = StoreMeta::Factory(STORE_META_VARY_HEADERS, vary.length() + 1, vary.c_str());
 
         if (!t) {
             storeSwapTLVFree(TLV);

diff --git a/src/tests/stub_MemObject.cc b/src/tests/stub_MemObject.cc
index cb502a285662748c37b67c8ae96251ede90cc919..92944a6ed1b2252a33b98837080b8b7d5089e911 100644 (file)
--- a/src/tests/stub_MemObject.cc
+++ b/src/tests/stub_MemObject.cc
@@ -38,7 +38,6 @@ MemObject::MemObject() :
     id(0),
     object_sz(-1),
     swap_hdr_sz(0),
-    vary_headers(NULL),
     _reply(NULL)
 {
     memset(&clients, 0, sizeof(clients));

diff --git a/src/tests/stub_http.cc b/src/tests/stub_http.cc
index 06f64fcbf619099949261c81d25424069d6a130e..509447fe4c6fb207140b3cdce919a273abee9c45 100644 (file)
--- a/src/tests/stub_http.cc
+++ b/src/tests/stub_http.cc
@@ -7,12 +7,12 @@
  */
 
 #include "squid.h"
-
 #include "HttpReply.h"
 #include "HttpRequest.h"
+#include "sbuf/SBuf.h"
 
 #define STUB_API "http.cc"
 #include "tests/STUB.h"
 
-const char * httpMakeVaryMark(HttpRequest * request, HttpReply const * reply) STUB_RETVAL(NULL)
+SBuf httpMakeVaryMark(HttpRequest *, HttpReply const *) STUB_RETVAL(SBuf())