Re: [PATCH] Port 2.7: logformat %oa

On 30/03/11 04:46, Alex Rousskov wrote:
> On 03/28/2011 10:37 PM, Amos Jeffries wrote:
>
>> This adds the log format to log the local IP address used on outgoing
>> connections to peers and servers. Squid-2.7 called this %oa.
>>
>> However it is a perfectly matching part of the existing set of %la and
>> %lp (local inbound) and %<lp (local outbound port).
>>
>> As such, the %oa is accepted as input for backward compatibility, but
>> the Squid-3 version is: %<la
>
> Since Squid3 has never supported %oa, I think there are not enough
> reasons to remain backward compatible [with Squid2] here. Your call, but
> I would just add %<la.

2.7 remains very popular and still gaining installs. I think this is
worth doing to ease those future upgrades should the admin see this log
tag. Other than a little code there is no cost to doing this.

>
>
>> + <p><em>%&gt;la</em> Local IP address used by last transaction with HTTP servers.
>> + <p><em>%&gt;lp</em> Local TCP port used by transactions with HTTP servers.
>
> Please make this consistent: "used by the last transaction" or something
> like that, in both cases.
>
> What about FTP servers and cache peers? If they are supported, you can
> say something like "Local ... used by the last transaction with the next
> HTTP hop"

They are supported. The only ones omitted are ICAP peers.
It's not limited to HTTP hops either, CONNECT is also supported.

Updated the release notes to match cf.data.pre.

>
>> + <p><em>%&gt;la</em> Local IP address used by last transaction with HTTP servers. Ported from 2.7 where it is called<em>%oa</em>
>> + <p><em>%&gt;lp</em> Local TCP port used by transactions with HTTP servers.
>> + <la Local IP address of the last server or peer connection
>> <lp Local port number of the last server or peer connection
>
> &gt; is ">", not"<".

Fixed.

The existing %<lp in release note was already broken. Thanks.

>
>> - request->hier.peer_local_port = comm_local_port(sock); // for %<lp logging
>> + comm_local_port(sock);
>> + request->hier.peer_local_addr = fd_table[sock].local_addr;
>
> This and similar changes make the code slightly less safe. Before the
> change, negative sock/fd value as well as closed sockets were handled
> correctly. After the change, they may not be. You can do this instead:
>
> if (comm_local_port(sock))
> request->hier.peer_local_addr = fd_table[sock].local_addr;
>

Right. Fixed. Thanks.

> or add a dedicated comm_local_address function to implement the above
> logic. You use it several times.

Not worth it I think. This is clean enough for now and the comm-cleanup
makes those particular lines obsolete.

>
>
>> + out = al->hier.peer_local_addr.NtoA(tmp,1024);
>
> Please use sizeof(tmp) instead. We currently have a mix of 1024 and
> sizeof() in that code. Let's tilt it towards the right usage.

Done.

>
>
>> + debugs(46, 0, "WARNING: the \"oa\" formating code is deprecated use the \"<la\" instead");
>
> Use DBG_CRITICAL? There is also a delimiter missing after "deprecated".
> Also, "formatting" seems to be a far more popular spelling. However, we
> could preserve all these to stay consistent with what we use elsewhere,
> I guess. Or change them all at once separately.

Oops, thanks. Fixed.

>
>
> No need to repost just because of the above changes, IMO.
>

Thanks for the vote. Fixed and Applied.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.11
   Beta testers wanted for 3.2.0.5