Re: [MERGE] SourceLayout: acl/, take1

Amos Jeffries wrote:
> Alex Rousskov wrote:
>> SourceLayout: acl/, take 1
>>
>> Moved src/ACL* and a few related files into src/acl/.
>> Renamed ACL source files from ACLFoo.{cc,cci,h} to Foo.{cc,cci,h}.
>>
>> Added acl/ libraries, reorganized auth/ libraries, and split
>> ACLChecklist class to avoid circular dependencies among libraries.
>>
>> Many targets in src/Makefile.am depended on selected ACL*cc and related
>> sources. These targets depend on acl/* libraries now. As a part of this
>> cleanup, the number of ufsdump sources went from about 160 to about 20.
>>
>> No functionality changes were intended. Source code changes were kept to
>> the minimum. All my build tests are successful, including "make
>> distcheck". However, since I had to move a lot of files, move some code
>> pieces, and split ACLChecklist, it is possible that some targets will no
>> longer build in some environments and some authentication code will
>> break. Please test.
>>
>> For details, please see individual commit messages below.
>>
>> Thank you,
>>
>> Alex.
>> bb:approve
>>
>
> Kudos on this. I was expecting it to take a while longer.
> You really seem to be churning through the layout stuff like you know
> what your doing :).
>
>
>> ---------------- change log ------------------
>> * Moved src/ACL* and a few related files into src/acl/.
>> Renamed ACL source files from ACLFoo.{cc,cci,h} to Foo.{cc,cci,h}.
>>
>> Many targets in src/Makefile.am depended on selected ACL ACL*cc and
>> related
>> sources. These targets depend on acl/* libraries now.
>>
>>
>> * Extracted transaction state storage and related checks from
>> ACLChecklist into
>> ACLFilledChecklist.
>>
>> ACLChecklist contained many data members representing the state of the
>> current
>> transaction (in a broad sense). These members and related methods
>> depended
>> on complex types such as HttpRequest and ConnStateData. Any Squid code
>> using
>> ACLChecklist (and there is a lot of that code) was, hence, dependent
>> on these
>> types. These dependencies caused, among other things, huge SOURCES
>> lists in
>> src/Makefile.am, often for trivial targets such as ufsdump and test
>> cases.
>>
>> ACLChecklist is an abstract class now (to make sure we do not
>> accidentally
>> create it). ACLChecklist has only one kid: ACLFilledChecklist. The
>> Filled()
>> global function can be used to cast ACLChecklist* to
>> ACLFilledChecklist*.
>> Since all ACLChecklist objects have to be ACLFilledChecklist objects,
>> the cast
>> is fast and safe. The cast allows us to avoid bloating ACLChecklist
>> with
>> virtual methods that only make sense in ACLFilledChecklist context.
>>
>> ACLFilledChecklist now contains state-specific members while
>> ACLChecklist
>> contains basic check list logic. The code that organizes or passes
>> through
>> ACL checks does not need to be exposed to ACLFilledChecklist and the
>> data
>> types it depends on.
>>
>> Furthermore, ACLFilledChecklist should not contain complicated checks
>> either.
>> It should focus on maintaining the state. The checks should go into
>> specific
>> ACLs. Otherwise, complex checks cause dependency cycles with
>> higher-level
>> libraries that provide code for those checks and yet depend on having
>> access
>> to ACLFilledChecklist to implement specific ACLs. Currently, only the
>> authenticated() method got moved to auth/Acl.{cc,h} to break the
>> circular
>> dependency between acl/libs and auth/libs. More work in that direction
>> will
>> probably be required as we move more src/* code into libraries.
>>
>> ACLFilledChecklist constructor replaces aclChecklistCreate global. This
>> simplifies the initiating code of all fast ACL checks: the checks no
>> longer
>> need to do manual state locking, duplicating aclChecklistCreate code.
>>
>>
>> * Split auth/libauth into two libraries:
>>
>> - auth/libauth containing core authentication code (used, in part,
>> by the acl/libstate library) and not using acl/ libraries; and
>> - auth/libacls containing authentication-related ACL code (used to
>> build
>> executables) and using acl/libstate.
>>
>> The split was necessary to prevent circular dependencies among acl/
>> and auth/
>> libraries.
>>
>> Added conditionally built libraries to libauth, eliminating the need
>> for
>> AUTH_LIBS_TO_ADD. Use libtool to build those libraries.
>>
>>
>> * Moved authenticated() method from ACL[Filled]Checklist into
>> auth/Acl.{cc,h} to
>> break the circular dependency between acl/libs and auth/libs.
>>
>>
>> * Split ACL.{cc,h} and src/acl_noncore.cc into acl/Acl and acl/Gadgets,
>> moving
>> high-level global functions into Gadgets and leaving basic API types
>> in Acl.
>>
>> Moved horrific acl_access::containsPURGE into aclPurgeMethodInUse to
>> avoid
>> exposing basic ACL API to "strategy" templates and HTTP-specific PURGE
>> method.
>> The aclPurgeMethodInUse global lives in acl/libacls, which is a
>> top-level
>> library that already contains a lot of data-specific code.
>>
>>
>> * Synced #includes after moving files around.
>>
>> Use newly added ACLFilledChecklist for fast ACL checks. Its
>> constructor locks
>> request and accessList, simplifying the caller code.
>>
>> Use newly added ACLFilledChecklist for state-specific ACL code.
>> Also, the
>> ACLChecklist::authenticated() method is now an AuthenticateAcl global
>> function. See ACLFilledChecklist addition log for rationale.
>>
>>
>> * Removed some 140 SOURCEs of ufsdump, adding a few stubs. The program
>> seems to
>> work on simple ufs cache files.
>>
>> urlCanonical is currently an always-asserting stub. I am not sure what
>> pulls
>> in urlCanonical. I know storeKeyPublicByRequest* require it, but I am
>> not sure
>> which source requires storeKeyPublicByRequest. If the stub assertion
>> fails on
>> some cache files, we will need to pull more sources or re-implement
>> urlCanonical.
>
> URL stuff is about to become a stand-alone class dependent only on
> StringNg and provide all URL/URI manipulation and storage.
> I would rather wait for StringNg, but some of it can be pushed forward
> and use old-String if it has to.
>
>>
>> The more sources are moved into libraries, the more difficult it may
>> be to
>> write isolated, compact test cases or tools because test case stubs and
>> customizations may start to conflict with names defined in the
>> libraries and
>> because pulling in a whole library might require defining more stubs.
>> It is
>> not clear yet how real this concern is in general, but a lot of acl/
>> SourceLayout time was spent on making ufsdump build...
>>
>
> The stub model still applies, just the unit focus changes from
> linked-files to linked-libraries.
>
> Each library should have a stub alternative for its whole API.
> So we link to the libraries we need, and stub all the libraries we don't
> need to link.
>
> It's only a real concern for unit-tests at present, thus the above
> design will work happily.
>
> ... now off to read the actual patch...
>

Well, I can't see anything glaring in a quick once-over. Just the class
name casing. But that should probably go in for a followup due to the size.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE6 or 3.0.STABLE13
   Current Beta Squid 3.1.0.6