From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Sat, 29 Aug 2015 17:59:28 +0000 (-0700)
Subject: Better alternative to rev.14267
X-Git-Tag: SQUID_4_0_1~88
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=3d29e12688e58a3d302855a130b76693cbfb160a;p=thirdparty%2Fsquid.git

Better alternative to rev.14267

Encapsulate the accessList pointer change logic so that it can be kept
consistent and CBDATA operations are not exposed to callers.
---

diff --git a/src/acl/Checklist.cc b/src/acl/Checklist.cc
index b48f4b8f9f..80b0b9b002 100644
--- a/src/acl/Checklist.cc
+++ b/src/acl/Checklist.cc
@@ -192,7 +192,7 @@ ACLChecklist::~ACLChecklist()
 {
     assert (!asyncInProgress());
 
-    cbdataReferenceDone(accessList);
+    changeAcl(nullptr);
 
     debugs(28, 4, "ACLChecklist::~ACLChecklist: destroyed " << this);
 }
@@ -314,9 +314,7 @@ ACLChecklist::fastCheck(const Acl::Tree * list)
 
     // Concurrent checks are not supported, but sequential checks are, and they
     // may use a mixture of fastCheck(void) and fastCheck(list) calls.
-    const Acl::Tree * const savedList = accessList;
-
-    accessList = cbdataReference(list);
+    const Acl::Tree * const savedList = changeAcl(list);
 
     // assume DENY/ALLOW on mis/matches due to action-free accessList
     // matchAndFinish() takes care of the ALLOW case
@@ -325,8 +323,7 @@ ACLChecklist::fastCheck(const Acl::Tree * list)
     if (!finished())
         markFinished(ACCESS_DENIED, "ACLs failed to match");
 
-    cbdataReferenceDone(accessList);
-    accessList = savedList;
+    changeAcl(savedList);
     occupied_ = false;
     PROF_stop(aclCheckFast);
     return currentAnswer();
diff --git a/src/acl/Checklist.h b/src/acl/Checklist.h
index 837aa0c689..bae3e8efce 100644
--- a/src/acl/Checklist.h
+++ b/src/acl/Checklist.h
@@ -164,6 +164,17 @@ public:
     virtual bool hasRequest() const = 0;
     virtual bool hasReply() const = 0;
 
+    /// change the current ACL list
+    /// \return a pointer to the old list, or NULL if that is no longer CBDATA-valid
+    const Acl::Tree *changeAcl(const Acl::Tree *t) {
+        const Acl::Tree *old = accessList;
+        if (t != accessList) {
+            accessList = cbdataReference(t);
+            cbdataReferenceDone(accessList);
+        }
+        return cbdataReferenceValid(old) ? old : nullptr;
+    }
+
 private:
     /// Calls non-blocking check callback with the answer and destroys self.
     void checkCallback(allow_t answer);
@@ -173,8 +184,8 @@ private:
     void changeState(AsyncState *);
     AsyncState *asyncState() const;
 
-public:
     const Acl::Tree *accessList;
+public:
 
     ACLCB *callback;
     void *callback_data;
diff --git a/src/acl/FilledChecklist.cc b/src/acl/FilledChecklist.cc
index 16af5315f6..fb8bf2f8a3 100644
--- a/src/acl/FilledChecklist.cc
+++ b/src/acl/FilledChecklist.cc
@@ -158,9 +158,7 @@ ACLFilledChecklist::ACLFilledChecklist(const acl_access *A, HttpRequest *http_re
     dst_addr.setEmpty();
     rfc931[0] = '\0';
 
-    // cbdataReferenceDone() is in either fastCheck() or the destructor
-    if (A)
-        accessList = cbdataReference(A);
+    changeAcl(A);
 
     if (http_request != NULL) {
         request = http_request;
diff --git a/src/client_side.cc b/src/client_side.cc
index 52102fbc8b..84ef380593 100644
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -3484,8 +3484,7 @@ ConnStateData::start()
 
             /* pools require explicit 'allow' to assign a client into them */
             if (pools[pool].access) {
-                cbdataReferenceDone(ch.accessList);
-                ch.accessList = cbdataReference(pools[pool].access);
+                ch.changeAcl(pools[pool].access);
                 allow_t answer = ch.fastCheck();
                 if (answer == ACCESS_ALLOWED) {