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.
