Re: [PATCH] %>la for intercepted connections

I am sending the latest patch, which include Alex and Amos suggestions

On 09/08/2011 07:18 AM, Alex Rousskov wrote:
> On 09/07/2011 09:38 PM, Amos Jeffries wrote:
>> On 08/09/11 02:21, Tsantilas Christos wrote:
>>> On 09/07/2011 05:03 AM, Amos Jeffries wrote:
>>>> + case LFT_LOCAL_LISTENING_IP:
>>>> + if (al->cache.port == NULL) {
>>>> + break; // dash
>>>> + } else if (al->cache.port->s.IsAnyAddr()) {
>>>> + if (al->tcpClient != NULL&&
>>>> + !(al->request != NULL&& (al->request->flags.spoof_client_ip ||
>>>> al->request->flags.intercepted))) {
>>>> + out = al->tcpClient->local.NtoA(tmp, sizeof(tmp));
>>>> + } // else dash.
>>>> + } else {
>>>> + out = al->cache.port->s.NtoA(tmp, sizeof(tmp));
>>>> + }
>>>> + break;
>
>>> My original code was not good, I agree...
>>> I have in my mind something like this:
>>> case LFT_LOCAL_LISTENING_IP:
>>> + if ((al->request->flags.spoof_client_ip ||
>>> al->request->flags.intercepted)&& al->cache.port) {
>>> + if (!al->cache.port->s.IsAnyAddr())
>>> + out = al->cache.port->s.NtoA(tmp, sizeof(tmp));
>>> + } else if (al->tcpClient != NULL)
>>> + out = al->tcpClient->local.NtoA(tmp, sizeof(tmp));
>>> + break;
>>>
>>> If it is an intercepted connection use the listening address (if it is
>>> available), else use the IP address used by the client to connect to
>>> squid. Is it OK?
>
>
>> That seems okay. Confusing to read, but is slightly more efficient than
>> what I suggested with the same cases covered.
>
>
> To be honest, neither version is readable to an uninitiated. May I
> suggest giving each condition a name and adding comments? For example
> (with wrong names/comments, I am sure),
>
> // avoid logging a dash if we have reliable info
> const bool interceptedAtKnownPort =
> (al->request->flags.spoof_client_ip ||
> al->request->flags.intercepted)&& al->cache.port);
> if (interceptedAtKnownPort) {
> const bool portAddressConfigured =
> !al->cache.port->s.IsAnyAddr()
> if (portAddressConfigured)
> ... // log listening address
> ...
>
>
> Another option would be to add methods to al->request and al->cache.port
> objects instead of local variables. That way, the same potentially
> useful condition can be reused throughout the code.
>
>
>> For the record these are the permutations we seek to cover...
>>
>>
>> Scenario 1: client 192.168.0.3 connects to google (74.125.237.81). Gets
>> intercepted into Squid.
>>
>> 1a) squid.conf: http_port 3129 intercept|tproxy
>>
>> tcpClient->remote == 192.168.0.3:$random (%>a:%>p)
>> tcpClient->local == 74.125.237.81:80 (%>la:%>lp)
>> al->cache.port->s.local == 0.0.0.0:3129 (%la:%lp) [log "-"]
>>
>>
>> 1b) squid.conf: http_port 192.168.0.1:3129 intercept|tproxy
>>
>> tcpClient->remote == 192.168.0.3:$random (%>a:%>p)
>> tcpClient->local == 74.125.237.81:80 (%>la:%>lp)
>> al->cache.port->s.local == 192.168.0.1:3129 (%la:%lp) [log 192...]
>>
>>
>> Scenario 2: client 192.168.0.3 connects to Squid asking for
>> http://google.com
>>
>> 2a) squid.conf: http_port 3128 [accel]
>>
>> tcpClient->remote == 192.168.0.3:$random (%>a:%>p)
>> tcpClient->local == 192.168.0.1:3128 (%>la:%>lp)
>> al->cache.port->s.local == 0.0.0.0:3128 (%la:%lp) [log 192...]
>>
>> 2b) squid.conf: http_port 192.168.0.1:3128 [accel]
>>
>> tcpClient->remote == 192.168.0.3:$random (%>a:%>p)
>> tcpClient->local == 192.168.0.1:3128 (%>la:%>lp)
>> al->cache.port->s.local == 192.168.0.1:3128 (%la:%lp) [log 192...]
>>
>>
>> Senario 3: squid generates an internal request.
>>
>> tcpClient == NULL (%>a:%>p,%>la:%>lp) [log "-"]
>> al->cache.port == NULL (%la:%lp) [log "-"]
>
>
> Perhaps the above list should be added to the commit message in case we
> need to reshuffle this code later? It sounds rather valuable to me.
>
>
> Thank you,
>
> Alex.
>
>
>