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.
