From: Amos Jeffries Date: Fri, 25 Mar 2016 20:11:29 +0000 (+1300) Subject: Convert Vary handling to SBuf X-Git-Tag: SQUID_4_0_8~17 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=90ab8f201a7ad60ac69e872dff361d6851bdaf23;p=thirdparty%2Fsquid.git Convert Vary handling to SBuf --- diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc index 6d75c20305..604bb5e0f2 100644 --- 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 b4653f6cbe..4651839c6b 100644 --- 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 8a9ada8a0b..50f0a03d1f 100644 --- 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 6bca732361..04139750e5 100644 --- 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 8393dda240..259fbec675 100644 --- 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 343b55ae59..bd8423ff55 100644 --- 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(value), length); return true; } - if (strcmp(e->mem_obj->vary_headers, (char *)value) != 0) + if (e->mem_obj->vary_headers.cmp(static_cast(value), length) != 0) return false; return true; diff --git a/src/client_side.cc b/src/client_side.cc index 9df9c99ba7..980c7b95a1 100644 --- 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 32fb00055d..507142eba5 100644 --- 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 a54804b28b..fcfea3d963 100644 --- 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 5eb40745f1..c115e08a59 100644 --- 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 3cc6e0cf31..be8142f938 100644 --- 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(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 aff4dfe812..60aeaf7bfb 100644 --- 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 c28352ffe1..36ba631783 100644 --- 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 9ffb4ace95..7e5dd0fe19 100644 --- 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 cb502a2856..92944a6ed1 100644 --- 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 06f64fcbf6..509447fe4c 100644 --- 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())