Re: [PATCH] Log TOS and Netfilter marks set by Squid

On 07/12/2013 08:06 PM, Amos Jeffries wrote:
> On 13/07/2013 2:05 a.m., Tsantilas Christos wrote:
>> This patch add new logformat codes to log TOS/DSCP values and netfilter
>> marks for client and server connections. If multiple outgoing
>> connections were used, the last used connection value logged.
>> The values printed in hexadecimal form.
>>
>> The logformat codes are:
>> %>tos Client connection tos mark set by Squid
>> %<tos Server connection tos mark set by Squid
>
> Can you call the code "qos" please? And document it exactly as you wrote
> above as "the TOS/DSCP value" removing the word "mark" from those lines
> to avoid confusion.

OK.

>
>
>> %>nfmark Client connection netfilter mark set by Squid
>> %<nfmark Server connection netfilter mark set by Squid
>>
>> I must note that the setsockopt(...,IPPROTO_IP, IP_TOS...) system call
>> we are using to set tos value may not set the configured value. It can
>> set only values which are multiple of 4. In this case we log wrong
>> value, the configured not the value is set.
>>
>> To get the real value we must modify the Ip::Qos::setSockTos method to
>> set the TOS and then read it again from socket to see which value is
>> used. However this is means one more system call to set the tos value.
>
> We should be auto-adjusting the configured value to make it a multiple
> of 4 before setting. So no set,read cycle required.
>
> Please rename these:
>
> + LFT_PEER_INCOMING_TOS,
> + LFT_PEER_OUTGOING_TOS,
> + LFT_PEER_INCOMING_NFMARK,
> + LFT_PEER_OUTGOING_NFMARK,
>
> We already have the naming scheme LFT_CLIENT_LOCAL_* and
> LFT_SERVER_LOCAL_* for the relevant TCP connections local-end details
> (ie. "set by Squid").

Done. it is match better now...

>
> Although I am not certain what you are calling the Client connection
> details set by Squid are in fact set by Squid and not the received ones
> set by the client. Please check that. If so the LFT_CLIENT_LOCAL_ part
> of the naming scheme should be LFT_CLIENT_*

Yes the values set by squid, not by client or web server.
The LFT_CLIENT_LOCAL and LFT_SERVER_LOCAL_ looks good options.

>
> Amos
>