Re: Patch to add netfilter mark support

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.

Hi Andrew,

     I cannot verify the low-level parts of your work, but I think you
have significantly improved the high-level staff. Thank you.

* I find the terminology inconsistent and confusing: outgoing,
clientside, upstream. No wonder you have to explain the difference
twice. Unless these are all standard RFC-like terms, please use
something consistent like fromClient, toClient, fromServer, toServer.
Others may suggest a better scheme, but this one at least does not
require constant doc lookups to understand where "out" and "up" is.

[Hint: In most cases, you can quickly rename things if you undo a patch,
change the names in the patch file, and apply the changed patch.]

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

* 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?

* Did you verify that "make -k check" and "./test-builds.sh" did not
break because of your changes?

* Please remove the following redundant lines (no need to document what
standard methods are):

> + /**
> + * Constructor
> + */

> + /**
> + * Destructor
> + */

* s/Returns true or false depending on whether/whether/g

* Not sure, but perhaps s/whether we should carry out the QOS functions
for/whether to apply QOS functions to/. This would allow you to collapse
the comment to just one line.

* Closing paren missing in isAclNfmarkActive() description.

* You can use /// comments for /** one line comments */ to make them a
little shorter and avoid line wrapping.

Thank you,

Alex.
Received on Sun Sep 05 2010 - 22:52:47 MDT