[squid-dev] [PATCH] refactor Auth::Config

[Amos Jeffries]

On 21/12/2016 12:30 p.m., Alex Rousskov wrote:
> On 12/20/2016 11:05 AM, Amos Jeffries wrote:
> 
>> +class Config
>> +{
>> +public:
>> +    /// set of auth_params directives
>> +    Auth::ConfigVector schemes;
>> +
>> +    /// set of auth_schemes directives
>> +    std::vector<Auth::SchemesConfig> schemeLists;
>> +
>> +    /// the ACL list for auth_schemes directives
>> +    acl_access *schemeAccess = nullptr;
> 
> Please prohibit copying/assignment of the new Auth::Config class objects
> because the above class members are not ready for that.
> 

Done. I had to add back the =default ctors/dtor to cover for the
implicit deletions after that, but seems okay.


> 
>> +    /// the authenticate_cache_garbage_interval
>> +    time_t authenticateGCInterval;
>> +
>> +    /// the authenticate_ttl
>> +    time_t authenticateTTL;
>> +
>> +    /// the authenticate_ip_ttl
>> +    time_t authenticateIpTTL;
> 
> Please initialize the above members of the new Auth::Config class. I
> know that we currently do not have a good way of specifying "correct"
> defaults for all Auth::Config objects except TheConfig, but we should
> still initialize these members. I would do something like
> 
>     time_t foo = 0; // TheConfig will get the right default
> 

Done.

> 
> Also, there is no good reason to repeat "authenticate" inside the Auth
> namespace. We should simply use gcInterval, credentialsTtl, and ipTtl
> member names IMO.
> 
> 
>> +    for (auto *scheme : Auth::TheConfig.schemes) {
>> +        if (scheme->configured())
>>              ++rv;
>> +    }
> 
> Since configured() is cons, please add const to "scheme" as well.
> 

Good catch. Done.

> 
> The above changes can be done without another review round IMO. I
> believe the rest of the patch is mostly renaming, code shoveling, and
> similar changes that I am bad at reviewing, so I did not check them closely.
> 

Okay, rescheduling tests and applying when it passes.

Thank you for the time and quick audit.

Amos