Re: [PATCH] Reusable ACLChecklist

On 06/10/2011 12:05 PM, Amos Jeffries wrote:
> On 11/06/11 05:28, Tsantilas Christos wrote:
>> 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.

I also think that is the best option.

> 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.

Another plus is that there is no confusion with regard to which method
to use for fast checks.

> 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.

Thank you,

Alex.
Received on Fri Jun 10 2011 - 23:57:41 MDT