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

[Alex Rousskov]

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.


> +    /// 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


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.


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.


Thank you,

Alex.