Re: [PATCH] Add DNS lookup step to ICAP connect

On 12/07/11 20:08, Tsantilas Christos wrote:
> Oops, forgot to attach the patch.
> I am attaching here the v6 version
>
>
> On 07/12/2011 02:41 AM, Alex Rousskov wrote:
>> On 07/01/2011 02:44 PM, Tsantilas Christos wrote:
>>
>>
>>> - theIdleConns("ICAP Service",NULL),
>>
>> Please initialize to NULL in case we throw before we reach the
>> allocation lines below.
>
> Done.
>
>>
>>
>>> + theIdleConns = new IdleConnList("ICAP Service",NULL);
>>> + assert(theIdleConns);
>>
>> Please remove the assert(). The new operator cannot return NULL.
> OK.
>
>>
>>
>>> + if ((reused = Comm::IsConnOpen(connection))) {
>>> debugs(93,3, HERE<< "reused pconn "<< connection);
>>> - ++theBusyConns;
>>> }
>>> + ++theBusyConns;
>>>
>>> return connection;
>>> }
>>
>> Unless I am missing something, this can be simplified and optimized as:
>>
>> ++theBusyConns;
>> debugs(93,3, HERE<< "got connection: "<< connection);
>> return connection;
>>
>> When the connection is printed, we will see whether it is open, right?
>
> yes, we just need the "reused = Comm::IsConnOpen(connection)" line. Now
> exist alone, without the "if" test.
>
>>
>> The above changes are cosmetic and should not block the commit if the
>> patch passes your tests, of course.
>
> OK.
> If there is not any objection I will commit it to trunk.
>

+1. Fine by me if it works for you.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.14
   Beta testers wanted for 3.2.0.9