Re: [MERGE] Bug 2305 - Auth ref-counting upgrade

Alex Rousskov wrote:
> Hi Amos,
>
> If you want to avoid including auth/UserRequest.h and other "heavy"
> headers just to get access to AuthUserRequest::Pointer and such,
> consider creating auth/forward.h where you can typedef
> AuthUserRequestPointer without pulling in full AuthUserRequest
> declaration. We already have similar code in src/adaptation/forward.h
>
>
> * I am concerned that done() is deleting/cleaning stuff in spite of the
> object possibly still being in use by others. Elsewhere, this kind of
> "early cleaning" approach led to bugs. Consider moving the actual
> cleanup to the destructor (but it is not trivial because other code may
> test for cleaned up state as an indication that the "end is close and we
> should not do much").
>
>
> * Consider renaming done() to detach() to emphasize that the object may
> still be in use for a while.

The above two are referring to authConfig right?

Other than removing the legacy typedefs I have not touched AuthConfig.

The config objects are located as needed by AuthConfig::Find(type) and
used immediately. If you find things other than the config vector
holding on to pointers of them that is a bug.

>
> * In the change below, can you leave the tighter name scope and remove
> the extra parenthesis around (request->auth_user_request)?
> - if (char const *name = auth->username()) {
> + char const *name = (request->auth_user_request)->username();
> + if (name) {
>

Fixed.

>
>
> * A bunch of new files are named with a small first letter and with a
> file name prefix duplicating the directory name, violating the file
> naming rules. For example:
>
> === added file 'src/auth/basic/basicUserRequest.cc'
> === added file 'src/auth/digest/digestUserRequest.cc'
>
> Do we want to do that for symmetry with nearby old files?
>

I've tried making these src/auth/basic/UserRequest.cc etc.

It fails on bootstrap due to the src/auth/Makefile.am building the
sub-libraries instead of a recursive directory makefile.

This will have to be fixed by the SourceLayout project upgrades later.

>
> * There are many whitespace-only changes in the patch. Is that due to
> the parent branch being behind on source formatting? This is not a big
> deal, of course, but it makes an already large patch even bigger and,
> hence, more costly to review. Please keep this in mind for future patches.
>
>
> It looks like the patch simplified authentication code and may have
> fixed a few nasty leaks/loops. I cannot verify all the details, but I
> certainly do not object to it (I guess it is +0).
>

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.3