Re: [PATCH] cert validation cache

On 13.12.2012 03:06, Tsantilas Christos wrote:
> On 12/12/2012 03:33 AM, Amos Jeffries wrote:
>> On 12.12.2012 05:02, Alex Rousskov wrote:
>>> On 12/11/2012 03:50 AM, Amos Jeffries wrote:
>>>> On 11/12/2012 9:19 p.m., Tsantilas Christos wrote:
>>>>> If there is not any objection I will apply the latest "cert
>>>>> validation
>>>>> cache" patch to trunk.
>>>
>>>> This patch is also threaded with "#if 1 //
>>>> USE_SSL_CERT_VALIDATOR" just
>>>> like the other one and will need re-testing without it.
>>>
>>> I think we should either use proper USE_SSL_CERT_VALIDATOR
>>> conditional
>>> or make this code unconditional. Iff nobody has strong opinions
>>> about
>>> it, I suggest making this code unconditional (no #ifs).
>>>
>>> The certificate validator is not enabled by default and the extra
>>> code
>>> does not add a lot of performance overhead, does it?
>>>
>>
>> Up to you guys whether the conditionals exist or not. But it
>> definitely
>> will need USE_SSL around the SSL-specific code.
>
> I think the USE_SSL exist in all places required.
> If there is not any objection:
> - I will move the LruMap under the src/base directory
> - I will apply this patch
> - I will remove with a second patch all "#if 1
> //USE_SSL_CERT_VALIDATOR"
>

I do object. The last two steps are adding useless patches. Please
remove the conditionals *before* applying to trunk.

>
>> PS. Christos, can you please avoid even developing with the "#if 1
>> //
>> USE_BLAH" style of force-enabling. If it is worth putting the
>> conditionals in at all it is worth adding a ./configure option and
>> testing during development with that options set/unset.
>
> In this project I use it as a comment. Something like "//Here starts
> cert validator code" and "//here stops the cert validator code".
> There are cases inside squid where we have similar blocks of code
> inside
> "#if 1" or "#if 0".
> For this project it is not worth such configure option.
> I agree it may confuse other developers, so it is better to just
> remove
> it...
>

OK.

Amos
Received on Wed Dec 12 2012 - 22:45:27 MST