Re: [MERGE] SourceLayout: acl/, take1

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

Amos

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