Reduced the chance of using an absent HttpHdrCc directive value.

Also reduced code duplication inside HttpReply::hdrExpirationTime().

diff --git a/src/HttpHdrCc.cc b/src/HttpHdrCc.cc
index 34ddd4628f27273aefed5f3659df181aafed773b..cd0797829899498ff00dcac5fc317b727adf0f8d 100644 (file)
--- a/src/HttpHdrCc.cc
+++ b/src/HttpHdrCc.cc
@@ -264,13 +264,13 @@ HttpHdrCc::packInto(Packable * p) const
             case HttpHdrCcType::CC_PUBLIC:
                 break;
             case HttpHdrCcType::CC_PRIVATE:
-                if (Private().size())
-                    p->appendf("=\"" SQUIDSTRINGPH "\"", SQUIDSTRINGPRINT(Private()));
+                if (private_.size())
+                    p->appendf("=\"" SQUIDSTRINGPH "\"", SQUIDSTRINGPRINT(private_));
                 break;
 
             case HttpHdrCcType::CC_NO_CACHE:
-                if (noCache().size())
-                    p->appendf("=\"" SQUIDSTRINGPH "\"", SQUIDSTRINGPRINT(noCache()));
+                if (no_cache.size())
+                    p->appendf("=\"" SQUIDSTRINGPH "\"", SQUIDSTRINGPRINT(no_cache));
                 break;
             case HttpHdrCcType::CC_NO_STORE:
                 break;
@@ -281,24 +281,24 @@ HttpHdrCc::packInto(Packable * p) const
             case HttpHdrCcType::CC_PROXY_REVALIDATE:
                 break;
             case HttpHdrCcType::CC_MAX_AGE:
-                p->appendf("=%d", maxAge());
+                p->appendf("=%d", max_age);
                 break;
             case HttpHdrCcType::CC_S_MAXAGE:
-                p->appendf("=%d", sMaxAge());
+                p->appendf("=%d", s_maxage);
                 break;
             case HttpHdrCcType::CC_MAX_STALE:
                 /* max-stale's value is optional.
                   If we didn't receive it, don't send it */
-                if (maxStale()!=MAX_STALE_ANY)
-                    p->appendf("=%d", maxStale());
+                if (max_stale != MAX_STALE_ANY)
+                    p->appendf("=%d", max_stale);
                 break;
             case HttpHdrCcType::CC_MIN_FRESH:
-                p->appendf("=%d", minFresh());
+                p->appendf("=%d", min_fresh);
                 break;
             case HttpHdrCcType::CC_ONLY_IF_CACHED:
                 break;
             case HttpHdrCcType::CC_STALE_IF_ERROR:
-                p->appendf("=%d", staleIfError());
+                p->appendf("=%d", stale_if_error);
                 break;
             case HttpHdrCcType::CC_IMMUTABLE:
                 break;

diff --git a/src/HttpHdrCc.h b/src/HttpHdrCc.h
index 6672e590634cfdb2c97130b2b70808891f02235a..2aa300f716c8e3f2128549e4b1059e36512de663 100644 (file)
--- a/src/HttpHdrCc.h
+++ b/src/HttpHdrCc.h
@@ -67,13 +67,11 @@ public:
 
     //manipulation for Cache-Control: public header
     bool hasPublic() const {return isSet(HttpHdrCcType::CC_PUBLIC);}
-    bool Public() const {return isSet(HttpHdrCcType::CC_PUBLIC);}
     void Public(bool v) {setMask(HttpHdrCcType::CC_PUBLIC,v);}
     void clearPublic() {setMask(HttpHdrCcType::CC_PUBLIC,false);}
 
     //manipulation for Cache-Control: private header
-    bool hasPrivate() const {return isSet(HttpHdrCcType::CC_PRIVATE);}
-    const String &Private() const {return private_;}
+    bool hasPrivate(const String **val = nullptr) const { return hasDirective(HttpHdrCcType::CC_PRIVATE, &private_, val); }
     void Private(const String &v) {
         setMask(HttpHdrCcType::CC_PRIVATE,true);
         if (!v.size())
@@ -86,8 +84,9 @@ public:
     void clearPrivate() {setMask(HttpHdrCcType::CC_PRIVATE,false); private_.clean();}
 
     //manipulation for Cache-Control: no-cache header
-    bool hasNoCache() const {return isSet(HttpHdrCcType::CC_NO_CACHE);}
-    const String &noCache() const {return no_cache;}
+    bool hasNoCacheWithParameters() const { return hasNoCache() && no_cache.size(); }
+    bool hasNoCacheWithoutParameters() const { return hasNoCache() && !no_cache.size(); }
+    bool hasNoCache(const String **val = nullptr) const { return hasDirective(HttpHdrCcType::CC_NO_CACHE, &no_cache, val); }
     void noCache(const String &v) {
         setMask(HttpHdrCcType::CC_NO_CACHE,true);
         if (!v.size())
@@ -101,43 +100,36 @@ public:
 
     //manipulation for Cache-Control: no-store header
     bool hasNoStore() const {return isSet(HttpHdrCcType::CC_NO_STORE);}
-    bool noStore() const {return isSet(HttpHdrCcType::CC_NO_STORE);}
     void noStore(bool v) {setMask(HttpHdrCcType::CC_NO_STORE,v);}
     void clearNoStore() {setMask(HttpHdrCcType::CC_NO_STORE,false);}
 
     //manipulation for Cache-Control: no-transform header
     bool hasNoTransform() const {return isSet(HttpHdrCcType::CC_NO_TRANSFORM);}
-    bool noTransform() const {return isSet(HttpHdrCcType::CC_NO_TRANSFORM);}
     void noTransform(bool v) {setMask(HttpHdrCcType::CC_NO_TRANSFORM,v);}
     void clearNoTransform() {setMask(HttpHdrCcType::CC_NO_TRANSFORM,false);}
 
     //manipulation for Cache-Control: must-revalidate header
     bool hasMustRevalidate() const {return isSet(HttpHdrCcType::CC_MUST_REVALIDATE);}
-    bool mustRevalidate() const {return isSet(HttpHdrCcType::CC_MUST_REVALIDATE);}
     void mustRevalidate(bool v) {setMask(HttpHdrCcType::CC_MUST_REVALIDATE,v);}
     void clearMustRevalidate() {setMask(HttpHdrCcType::CC_MUST_REVALIDATE,false);}
 
     //manipulation for Cache-Control: proxy-revalidate header
     bool hasProxyRevalidate() const {return isSet(HttpHdrCcType::CC_PROXY_REVALIDATE);}
-    bool proxyRevalidate() const {return isSet(HttpHdrCcType::CC_PROXY_REVALIDATE);}
     void proxyRevalidate(bool v) {setMask(HttpHdrCcType::CC_PROXY_REVALIDATE,v);}
     void clearProxyRevalidate() {setMask(HttpHdrCcType::CC_PROXY_REVALIDATE,false);}
 
     //manipulation for Cache-Control: max-age header
-    bool hasMaxAge() const {return isSet(HttpHdrCcType::CC_MAX_AGE);}
-    int32_t maxAge() const { return max_age;}
+    bool hasMaxAge(int32_t *val = nullptr) const { return hasDirective(HttpHdrCcType::CC_MAX_AGE, max_age, val); }
     void maxAge(int32_t v) {setValue(max_age,v,HttpHdrCcType::CC_MAX_AGE); }
     void clearMaxAge() {setValue(max_age,MAX_AGE_UNKNOWN,HttpHdrCcType::CC_MAX_AGE,false);}
 
     //manipulation for Cache-Control: s-maxage header
-    bool hasSMaxAge() const {return isSet(HttpHdrCcType::CC_S_MAXAGE);}
-    int32_t sMaxAge() const { return s_maxage;}
+    bool hasSMaxAge(int32_t *val = nullptr) const { return hasDirective(HttpHdrCcType::CC_S_MAXAGE, s_maxage, val); }
     void sMaxAge(int32_t v) {setValue(s_maxage,v,HttpHdrCcType::CC_S_MAXAGE); }
     void clearSMaxAge() {setValue(s_maxage,MAX_AGE_UNKNOWN,HttpHdrCcType::CC_S_MAXAGE,false);}
 
     //manipulation for Cache-Control: max-stale header
-    bool hasMaxStale() const {return isSet(HttpHdrCcType::CC_MAX_STALE);}
-    int32_t maxStale() const { return max_stale;}
+    bool hasMaxStale(int32_t *val = nullptr) const { return hasDirective(HttpHdrCcType::CC_MAX_STALE, max_stale, val); }
     // max-stale has a special value (MAX_STALE_ANY) which correspond to having
     // the directive without a numeric specification, and directs to consider the object
     // as always-expired.
@@ -145,25 +137,22 @@ public:
     void clearMaxStale() {setValue(max_stale,MAX_STALE_UNKNOWN,HttpHdrCcType::CC_MAX_STALE,false);}
 
     //manipulation for Cache-Control:min-fresh header
-    bool hasMinFresh() const {return isSet(HttpHdrCcType::CC_MIN_FRESH);}
-    int32_t minFresh() const { return min_fresh;}
+    bool hasMinFresh(int32_t *val = nullptr) const { return hasDirective(HttpHdrCcType::CC_MIN_FRESH, max_stale, val); }
     void minFresh(int32_t v) {if (v < 0) return; setValue(min_fresh,v,HttpHdrCcType::CC_MIN_FRESH); }
     void clearMinFresh() {setValue(min_fresh,MIN_FRESH_UNKNOWN,HttpHdrCcType::CC_MIN_FRESH,false);}
 
     //manipulation for Cache-Control: only-if-cached header
     bool hasOnlyIfCached() const {return isSet(HttpHdrCcType::CC_ONLY_IF_CACHED);}
-    bool onlyIfCached() const {return isSet(HttpHdrCcType::CC_ONLY_IF_CACHED);}
     void onlyIfCached(bool v) {setMask(HttpHdrCcType::CC_ONLY_IF_CACHED,v);}
     void clearOnlyIfCached() {setMask(HttpHdrCcType::CC_ONLY_IF_CACHED,false);}
 
     //manipulation for Cache-Control: stale-if-error header
-    bool hasStaleIfError() const {return isSet(HttpHdrCcType::CC_STALE_IF_ERROR);}
-    int32_t staleIfError() const { return stale_if_error;}
+    bool hasStaleIfError(int32_t *val = nullptr) const { return hasDirective(HttpHdrCcType::CC_STALE_IF_ERROR, stale_if_error, val); }
     void staleIfError(int32_t v) {setValue(stale_if_error,v,HttpHdrCcType::CC_STALE_IF_ERROR); }
     void clearStaleIfError() {setValue(stale_if_error,STALE_IF_ERROR_UNKNOWN,HttpHdrCcType::CC_STALE_IF_ERROR,false);}
 
     //manipulation for Cache-Control: immutable header
-    bool Immutable() const {return isSet(HttpHdrCcType::CC_IMMUTABLE);}
+    bool hasImmutable() const {return isSet(HttpHdrCcType::CC_IMMUTABLE);}
     void Immutable(bool v) {setMask(HttpHdrCcType::CC_IMMUTABLE,v);}
     void clearImmutable() {setMask(HttpHdrCcType::CC_IMMUTABLE,false);}
 
@@ -190,6 +179,17 @@ private:
     String private_; ///< List of headers sent as value for CC:private="...". May be empty/undefined if the value is missing.
     String no_cache; ///< List of headers sent as value for CC:no-cache="...". May be empty/undefined if the value is missing.
 
+    /// implements typical has*() method logic
+    template<class Value>
+    bool hasDirective(const HttpHdrCcType hdrType, const Value &parsedVal, Value *outVal = nullptr) const {
+        if (isSet(hdrType)) {
+            if (outVal)
+                *outVal = parsedVal;
+            return true;
+        }
+        return false;
+    }
+
     /// low-level part of the public set method, performs no checks
     void setMask(HttpHdrCcType id, bool newval=true) {
         if (newval)

diff --git a/src/HttpReply.cc b/src/HttpReply.cc
index a0a0abedaefcfcb5fb73b6bdfd9a0833ef71d920..c715d0708f2abf679a4c36834466c7d7ec49b885 100644 (file)
--- a/src/HttpReply.cc
+++ b/src/HttpReply.cc
@@ -262,24 +262,13 @@ HttpReply::hdrExpirationTime()
     /* The s-maxage and max-age directive takes priority over Expires */
 
     if (cache_control) {
-        if (date >= 0) {
-            if (cache_control->hasSMaxAge())
-                return date + cache_control->sMaxAge();
-
-            if (cache_control->hasMaxAge())
-                return date + cache_control->maxAge();
-        } else {
-            /*
-             * Conservatively handle the case when we have a max-age
-             * header, but no Date for reference?
-             */
-
-            if (cache_control->hasSMaxAge())
-                return squid_curtime;
-
-            if (cache_control->hasMaxAge())
-                return squid_curtime;
-        }
+        int maxAge = -1;
+        /*
+         * Conservatively handle the case when we have a max-age
+         * header, but no Date for reference?
+         */
+        if (cache_control->hasSMaxAge(&maxAge) || cache_control->hasMaxAge(&maxAge))
+            return (date >= 0) ? date + maxAge : squid_curtime;
     }
 
     if (Config.onoff.vary_ignore_expire &&

diff --git a/src/HttpRequest.cc b/src/HttpRequest.cc
index 9cca8cbe394fc4fc42161f516417335014a61538..1d21ff3fd537f8699dad293448731dcbd54f4fbe 100644 (file)
--- a/src/HttpRequest.cc
+++ b/src/HttpRequest.cc
@@ -548,7 +548,7 @@ HttpRequest::maybeCacheable()
         //
         // NP: refresh_pattern ignore-no-store only applies to response messages
         //     this test is handling request message CC header.
-        if (!flags.ignoreCc && cache_control && cache_control->noStore())
+        if (!flags.ignoreCc && cache_control && cache_control->hasNoStore())
             return false;
         break;
 

diff --git a/src/client_side_request.cc b/src/client_side_request.cc
index 27a7996adb0737eb7dc23262fecd007756b90911..b955bd5a90fbe20bde1357081a4e0c87a1eb3883 100644 (file)
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -193,7 +193,7 @@ ClientHttpRequest::onlyIfCached()const
 {
     assert(request);
     return request->cache_control &&
-           request->cache_control->onlyIfCached();
+           request->cache_control->hasOnlyIfCached();
 }
 
 /**

diff --git a/src/http.cc b/src/http.cc
index 3d7fb5c814ef4ec9ecb32c4c8843415f3e2f32c2..c193a5de890412d9f492268f8f02dacf34b54077 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -284,7 +284,7 @@ HttpStateData::processSurrogateControl(HttpReply *reply)
         HttpHdrScTarget *sctusable = reply->surrogate_control->getMergedTarget(Config.Accel.surrogate_id);
 
         if (sctusable) {
-            if (sctusable->noStore() ||
+            if (sctusable->hasNoStore() ||
                     (Config.onoff.surrogate_is_remote
                      && sctusable->noStoreRemote())) {
                 surrogateNoStore = true;
@@ -361,14 +361,14 @@ HttpStateData::cacheableReply()
         // for now we are not reliably doing that so we waste CPU re-checking request CC
 
         // RFC 2616 section 14.9.2 - MUST NOT cache any response with request CC:no-store
-        if (request && request->cache_control && request->cache_control->noStore() &&
+        if (request && request->cache_control && request->cache_control->hasNoStore() &&
                 !REFRESH_OVERRIDE(ignore_no_store)) {
             debugs(22, 3, HERE << "NO because client request Cache-Control:no-store");
             return 0;
         }
 
         // NP: request CC:no-cache only means cache READ is forbidden. STORE is permitted.
-        if (rep->cache_control && rep->cache_control->hasNoCache() && rep->cache_control->noCache().size() > 0) {
+        if (rep->cache_control && rep->cache_control->hasNoCacheWithParameters()) {
             /* TODO: we are allowed to cache when no-cache= has parameters.
              * Provided we strip away any of the listed headers unless they are revalidated
              * successfully (ie, must revalidate AND these headers are prohibited on stale replies).
@@ -382,7 +382,7 @@ HttpStateData::cacheableReply()
         // NP: other request CC flags are limiters on HIT/MISS. We don't care about here.
 
         // RFC 2616 section 14.9.2 - MUST NOT cache any response with CC:no-store
-        if (rep->cache_control && rep->cache_control->noStore() &&
+        if (rep->cache_control && rep->cache_control->hasNoStore() &&
                 !REFRESH_OVERRIDE(ignore_no_store)) {
             debugs(22, 3, HERE << "NO because server reply Cache-Control:no-store");
             return 0;
@@ -419,12 +419,12 @@ HttpStateData::cacheableReply()
 
         bool mayStore = false;
         // HTTPbis pt6 section 3.2: a response CC:public is present
-        if (rep->cache_control->Public()) {
+        if (rep->cache_control->hasPublic()) {
             debugs(22, 3, HERE << "Authenticated but server reply Cache-Control:public");
             mayStore = true;
 
             // HTTPbis pt6 section 3.2: a response CC:must-revalidate is present
-        } else if (rep->cache_control->mustRevalidate()) {
+        } else if (rep->cache_control->hasMustRevalidate()) {
             debugs(22, 3, HERE << "Authenticated but server reply Cache-Control:must-revalidate");
             mayStore = true;
 
@@ -433,13 +433,13 @@ HttpStateData::cacheableReply()
             // HTTPbis WG verdict on this is that it is omitted from the spec due to being 'unexpected' by
             // some. The caching+revalidate is not exactly unsafe though with Squids interpretation of no-cache
             // (without parameters) as equivalent to must-revalidate in the reply.
-        } else if (rep->cache_control->hasNoCache() && rep->cache_control->noCache().size() == 0) {
+        } else if (rep->cache_control->hasNoCacheWithoutParameters()) {
             debugs(22, 3, HERE << "Authenticated but server reply Cache-Control:no-cache (equivalent to must-revalidate)");
             mayStore = true;
 #endif
 
             // HTTPbis pt6 section 3.2: a response CC:s-maxage is present
-        } else if (rep->cache_control->sMaxAge()) {
+        } else if (rep->cache_control->hasSMaxAge()) {
             debugs(22, 3, HERE << "Authenticated but server reply Cache-Control:s-maxage");
             mayStore = true;
         }
@@ -997,10 +997,10 @@ HttpStateData::haveParsedReplyHeaders()
             // For security reasons we do so even if storage was caused by refresh_pattern ignore-* option
 
             // CC:must-revalidate or CC:proxy-revalidate
-            const bool ccMustRevalidate = (rep->cache_control->proxyRevalidate() || rep->cache_control->mustRevalidate());
+            const bool ccMustRevalidate = (rep->cache_control->hasProxyRevalidate() || rep->cache_control->hasMustRevalidate());
 
             // CC:no-cache (only if there are no parameters)
-            const bool ccNoCacheNoParams = (rep->cache_control->hasNoCache() && rep->cache_control->noCache().size()==0);
+            const bool ccNoCacheNoParams = rep->cache_control->hasNoCacheWithoutParameters();
 
             // CC:s-maxage=N
             const bool ccSMaxAge = rep->cache_control->hasSMaxAge();

diff --git a/src/refresh.cc b/src/refresh.cc
index 9a72ad600b34d5bfe299201db20e89c8dd4c5561..dceb824e1c8bf945ec9b6064801e1f56ecb589b6 100644 (file)
--- a/src/refresh.cc
+++ b/src/refresh.cc
@@ -309,8 +309,8 @@ refreshCheck(const StoreEntry * entry, HttpRequest * request, time_t delta)
 
     if (request && !request->flags.ignoreCc) {
         const HttpHdrCc *const cc = request->cache_control;
-        if (cc && cc->hasMinFresh()) {
-            const int32_t minFresh=cc->minFresh();
+        int minFresh = -1;
+        if (cc && cc->hasMinFresh(&minFresh)) {
             debugs(22, 3, "\tage + min-fresh:\t" << age << " + " <<
                    minFresh << " = " << age + minFresh);
             debugs(22, 3, "\tcheck_time + min-fresh:\t" << check_time << " + "
@@ -330,9 +330,10 @@ refreshCheck(const StoreEntry * entry, HttpRequest * request, time_t delta)
     const HttpReplyPointer reply(entry->mem_obj && entry->mem_obj->getReply() ? entry->mem_obj->getReply() : nullptr);
 
     // stale-if-error requires any failure be passed thru when its period is over.
+    int staleIfError = -1;
     if (request && reply && reply->cache_control &&
-            reply->cache_control->hasStaleIfError() &&
-            reply->cache_control->staleIfError() < staleness) {
+            reply->cache_control->hasStaleIfError(&staleIfError) &&
+            staleIfError < staleness) {
 
         debugs(22, 3, "stale-if-error period expired. Will produce error if validation fails.");
         request->flags.failOnValidationError = true;
@@ -416,32 +417,34 @@ refreshCheck(const StoreEntry * entry, HttpRequest * request, time_t delta)
         if (NULL != cc) {
 
             // max-age directive
-            if (cc->hasMaxAge()) {
+            int maxAge = -1;
+            if (cc->hasMaxAge(&maxAge)) {
 
                 // draft-mcmanus-immutable-00: reply contains CC:immutable then ignore client CC:max-age=N
-                if (reply && reply->cache_control && reply->cache_control->Immutable()) {
-                    debugs(22, 3, "MAYBE: Ignoring client CC:max-age=" << cc->maxAge() << " request - 'Cache-Control: immutable'");
+                if (reply && reply->cache_control && reply->cache_control->hasImmutable()) {
+                    debugs(22, 3, "MAYBE: Ignoring client CC:max-age=" << maxAge << " request - 'Cache-Control: immutable'");
 
 #if USE_HTTP_VIOLATIONS
                     // Ignore of client "Cache-Control: max-age=0" header
-                } else if (R->flags.ignore_reload && cc->maxAge() == 0) {
+                } else if (R->flags.ignore_reload && maxAge == 0) {
                     debugs(22, 3, "MAYBE: Ignoring client reload request - trying to serve from cache (ignore-reload option)");
 #endif
 
                     // Honour client "Cache-Control: max-age=x" header
-                } else if (age > cc->maxAge() || cc->maxAge() == 0) {
-                    debugs(22, 3, "YES: Revalidating object - client 'Cache-Control: max-age=" << cc->maxAge() << "'");
+                } else if (age > maxAge || maxAge == 0) {
+                    debugs(22, 3, "YES: Revalidating object - client 'Cache-Control: max-age=" << maxAge << "'");
                     return STALE_EXCEEDS_REQUEST_MAX_AGE_VALUE;
                 }
             }
 
             // max-stale directive
-            if (cc->hasMaxStale() && staleness > -1) {
-                if (cc->maxStale()==HttpHdrCc::MAX_STALE_ANY) {
+            int maxStale = -1;
+            if (cc->hasMaxStale(&maxStale) && staleness > -1) {
+                if (maxStale==HttpHdrCc::MAX_STALE_ANY) {
                     debugs(22, 3, "NO: Client accepts a stale response of any age - 'Cache-Control: max-stale'");
                     return FRESH_REQUEST_MAX_STALE_ALL;
-                } else if (staleness < cc->maxStale()) {
-                    debugs(22, 3, "NO: Client accepts a stale response - 'Cache-Control: max-stale=" << cc->maxStale() << "'");
+                } else if (staleness < maxStale) {
+                    debugs(22, 3, "NO: Client accepts a stale response - 'Cache-Control: max-stale=" << maxStale << "'");
                     return FRESH_REQUEST_MAX_STALE_VALUE;
                 }
             }