Re: [PATCH] Reusable ACLChecklist

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