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 b48f4b8f9fc498c88e4a4a7b84e2937ac2191b06..80b0b9b0024eb5b8e19aecf465bc7bb24f46265a 100644 (file)
--- 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 837aa0c6899b25bdbd3229473e09d472e62010b8..bae3e8efce88197a45a1c7a2f5466dbf82d4b0c4 100644 (file)
--- 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 16af5315f61b22a8af97142322131aefa17f58d8..fb8bf2f8a39cdb35c304c788516680c61a5e72ea 100644 (file)
--- 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 52102fbc8bbbb5126180d83992c0ff31b98a2c51..84ef380593ef947855f20f259dcb28cf8c62456f 100644 (file)
--- 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) {