Re: Patch to add netfilter mark support

On Sun, 05 Sep 2010 16:52:27 -0600, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 09/05/2010 02:59 PM, Andrew Beverley wrote:
>> Please find attached the latest version of the patch to add Netfilter
>> marking support to Squid.
>>
>> All the previous comments have now been actioned.
>>
>> One thing that I haven't dealt with yet is the dependency on the
>> ip_conntrack kernel module. This seems to get loaded automatically
after
>> some use of Squid, but not straight away, which means that the mark
>> retention does not initially work. I've done some googling but have not
>> found how to force a kernel module load in a program. Is someone able
to
>> advise please?
>>
>> Since my last submission (prompted by a request on squid-users), I have
>> also added tcp_outgoing_mark and clientside_mark to complement
>> tcp_outgoing_tos and clientside_tos. I am conscious that I have been
>> copying old code to implement these, some of which does not seem
>> particularly elegant. However, rather than changing things from my
>> inexperienced perspective, I thought it best if I post the changes
as-is
>> and action any feedback as appropriate.
>>
>> As part of this I have added isAclNfmarkActive() and isAclTosActive()
to
>> return whether there should be any active TOS or MARK packet marking. I
>> added these to fde as that seemed the most appropriate place, but again
>> please tell me if I should move them elsewhere.
>

I have not yet looked at the patch, this is just some additions to the
points Alex makes...

>
> * TOS (unsigned char) and mark (uint32_t) types are repeated numerous
> times throughout the patch. Do you thin it would be a good idea to
> typedef them? It seems fairly easy for somebody to type them wrong or
> mix them up. I cannot insist on this change, but it would be the right
> thing to do, IMO.
>
> * If TOS and mark types can be the same uint32_t, we should probably use

> the same type and avoid at least some of the code duplication due to the

> difference in types.

64-bit marks are on the table at netfilter and with TCP/IPv6 extensions.
The same is not true for TCP/IPv4.

I have no opinion about the change itself, but IF done use two types
please.

>
> * The isAclNfmarkActive() and isAclTosActive() functions appear to be
> unrelated to src/fde.cc. They are about configuration, right? Why not
> make them Qos globals or, of the lists are moved, Qos::Config methods?
>
> * I believe Amos does not want new functions in protos.h unless
> necessary. Can you declare getOutgoingTOS() and getOutgoingNfmark() in
> src/forward.h? And capitalize the first letter since they are global?

Indeed, those should be in the src/ip/Qos* headers somewhere. (I know the
original TOS was still in protos.h, but lets take the opportunity for the
extra cleanup.)

Amos
Received on Mon Sep 06 2010 - 00:57:37 MDT