ACLChecklist reuse

Hello,

    While investigating a bug report, I have realized that ACLChecklist
objects are currently meant to be used only once (for a either a fast or
slow check). This limitation comes from a self-destructing nature of the
check() method which can be sketched like this (Sketch A):

> void
> ACLChecklist::check()
> {
> while (accessList != NULL) {
> if (matched)
> return ...
> accessList = next in accessList;
> }
> return ...
> }

Since accessList is a data member of ACLChecklist, the checking loop
removes information with every iteration when accessList is set to the
next list item.

I do not know why ACLChecklist::check() and other similar ACL code was
written like this, but I suspect it was just easier than preserving
accessList items during the check and cleaning up only when the object
is destroyed. Here is how that more complex code would look like (Sketch B):

> void
> ACLChecklist::check()
> {
> for (current = accessList; current; current = next) {
> matched = ...
> if (matched)
> return ...
> }
> return ...
> }

I am omitting cbdata locking steps in both cases. While Sketch B
destructor would need to unlock both accessList and current (if set), I
do not think this is relevant to this discussion.

I found Squid code that, under certain conditions, reuses an
ACLChecklist object, with bad consequences. Our options are:

1) Continue to use Sketch A design. Preserve/lock enough information
about the request, response, connection, etc. to be able to create a new
ACLChecklist object when it is needed in the rather low-level context
where that object is being [re]used now.

The primary problem with this approach is that we have to create and
maintain a new "pre-ACLChecklist" object that is used only to create
ACLChecklist objects, which already have the necessary code to
preserve/lock information used in the checks.

2) Switch to Sketch B design so that any ACLChecklist object can be
reused if needed.

3) Add an ACLChecklist member to switch between sketch A and sketch B
behavior. The primary problem with this approach is that it is more
complex than either #1 or #2. It would be more difficult to implement it
correctly without changing a lot of current ACLChecklist class
implementation at least a little.

I would like to go with option #2 (Sketch B) because it is simple and
yet allows ACLChecklist object reuse. I think it can be implemented
without many changes by adding a initialAccessList or a similar data
member so that the existing accessList member can still represent the
current check and be re-initialized in the beginning of each check.

Am I missing some problems with option #2? Do you see better solutions?

Thank you,

Alex.
Received on Thu May 19 2011 - 18:36:02 MDT