Re: [PATCH] Reusable ACLChecklist

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