On 11/06/11 05:28, Tsantilas Christos wrote:
Sorry,
I did a mistake I send an intermediate patch. This one is the correct.
The comments in my previous mail are correct (I hope).
I am marking the new patch as v4.
On 06/10/2011 07:58 PM, Tsantilas Christos wrote:
Hi all,
The problem has been discussed here:
http://www.squid-cache.org/mail-archive/squid-dev/201105/0116.html>
This patch add the method ACLChecklist::reusableFastCheck which is
similar to the ACLChecklist::fastCheck method but it can be called many
times, and each time called a new independed access list check done.
Also I found some problems in ACLChecklist::fastCheck which the current
patch does not try to solve them, but we may take care of.
1) Documentation of ::fastCheck does not match implementation
--------------------------------------------------------------
Currently the documentation of the ACLChecklist::fastCheck does not
match the implementation. From the documentation:
* If there is no access list to check the default is to return DENIED.
...
* \retval 1/true Access Allowed
* \retval 0/false Access Denied
But in the case no access list exist (accessList = NULL) we have inside
::fastCheck():
currentAnswer(ACCESS_DENIED);
while (accessList) {
....
}
return currentAnswer() == ACCESS_DENIED;
In the case no access list defined, it return true ("access allowed").
We should fix the ::fastCheck or its documentation.
2) Bug in ::fastCheck() method
-------------------------------
Now, when the ::fastCheck() called for a second time or third time, the
accessList is NULL always:
- if not match because list exhausted in first call
- if match because of the "cbdataReferenceDone(accessList);" line
(src/acl/Checklist.cc line 350).
So a second call of the ::fastCheck() will return access allowed, even
if the initial decision was access denied...
This is not normal (I think it is a bug).
I propose to replace the old ::fastCheck() method with the new
::reusableFastCheck() code so the fastCheck() calls have simpler
semantics and are always reusable.
Since this approach was taken. My vote goes towards just having
fastCheck() altered to do this behaviour. Saving on some code duplication.
Pros: The existing non-broken code all resets the list manually before
fastCheck(). So no compatibility problems. If anything a bit of cleanup
simplification later on as we find places this can optimize.
Some fluffy bits:
"The new methd does not change the internal state of the ACLChecklist
object"
* this is false. The funtion clearly changes currentAnswer().
* the PROF_* bits used by fastCheck() need to be kept for benchmark
profiling.
* can the local cbdata references be made CbcPointer please?
That should clear up all the pointer shuffling.
Amos
--
Please be using
Current Stable Squid 2.7.STABLE9 or 3.1.12
Beta testers wanted for 3.2.0.8 and 3.1.12.2
