Re: [PATCH] SSL server certificate validator implementation

On 23/11/2012 5:56 a.m., Tsantilas Christos wrote:
> One-two clarifications
>
> On 11/22/2012 02:10 PM, Amos Jeffries wrote:
>> On 22/11/2012 9:14 a.m., Tsantilas Christos wrote:
>> * please enable concurrency by default. Since this is a new interface we
>> have no legacy excuses to hold us back on good performance.
> Not a real concurrency. It is just a testing helper, can not be used in
> production.
> Is it OK?

If we bundle it with Squid it becomes the example demonstration helper
which people will copy-and-paste from.
It would be best to demonstrate concurrency from the start even in our
test helpers.

>
>> src/cf.data.pre:
>> * missing documentation about concurrency= support on this interface.
>> * if the bundled helper is upgraded to concurrency support the config
>> default can be changed to enable some number of channels.
> Is it OK to set "concurrency=1"
>
>>
>> src/forward.cc:
>> * Please check for BH status being returned by the helper differently
>> from the "NULL reply" error case. The handler can receive a reply with
>> BH having content. Ssl::CertValidationHelper::sslSubmit() being one
>> place which uses a message for Squid-side errors in the helper transaction.
> If there is not any problem I will fix this one to "helper validator
> cache" patch, to minimize the the conflicts during merging the patches
> to trunk.

I was going to leave it out of this review BUT found that omitting it
will cause failures on the Suqid-side error responses as noted later in
the review.

Amos
Received on Fri Nov 23 2012 - 01:51:31 MST