Re: [PATCH] Port 2.7: logformat %oa

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.

> + <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"

> + <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 "<".

> - 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;

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

> + 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.

> + 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.

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

Thank you,

Alex.
Received on Tue Mar 29 2011 - 15:47:02 MDT