Re: Patch to add netfilter mark support

On Sun, 05 Sep 2010 21:59:34 +0100, Andrew Beverley <andy_at_andybev.com>
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.
>
> Thanks,
>
> Andy

I'm unable to check the configure.in logics at present; however they
should be operating to auto-detect libnetfilter_contrack unless --without
is specified. correct?

configure.in:
The feature should be defaulting to auto-on. This means...

 * A default value for with_netfilter_conntrack=yes and maybe unset
netfilterconntrackpath needs to be set before the ARG_WITH check. Although
I believe under the new design that latter should be
squid_opt_netfilterconntrackpath

 * Help text needs to be talking about using --without-* to remove/disable
the library use (and via that the feature). Also I think the text
description you have there is more suited to the release notes detailed
explanation of the new option. The ./configure can state just this:
[--without-netfilter-conntrack],[Do not use Netfilter conntrack libraries
for packet marking.
 A path to alternative library location may be specified.
 Default: auto-detect.]

 * ARG_WITH if-yes clause then becomes:
  case "$withval" in
    yes|no) with_netfilter_conntrack=$withval ;;
    *) netfilterconntrackpath=$withval ;;
  esac

 * ARG_WITH needs an if-no clause doing with_netfilter_conntrack=no to
override the default.

 * Then the enable/disable test becomes: 'if test
"x$with_netfilter_conntrack" != "xno"; '

 * The alternate path is only an alternative source of the
libnetfilter-conntrack correct? that means the "else" part of the if
statement needs to be run regardless of what the flags contain. The flags
will merely cause that source to be used if needed.

 * The error about a directory being "not executable" is incorrect. using
-d and "does not exist".

 * please replace AC_CHECK_LIB with AC_SEARCH_LIBS and set
with_netfilter_conntrack=no when it emits that error.

 * AC_CHECK_HEADERS is needed regardless of whether the system or a custom
library is used.

 * With default-on the AC_CHECK_HEADERS gets two extra parameters:
     AC_CHECK_HEADERS[ ... ],[:],[with_netfilter_conntrack=no])
  which cleanly disables the feature if any header is missing.

@line 2046:
 * please us the X prefix on string tests for 'if test "$enable_zph_qos" =
"yes"'

 * you don't need the second if statement. 'if test
"$with_netfilter_conntrack" = "yes"'
 * SQUID_DEFINE_BOOL instead with ${with_netfilter_conntrack:=no} as the
value to set.
 * Just the notice about the marking yes/no state is sufficient,
interested people can see that QoS was enabled but marking part of it
disabled the very next line.

src/Parsing.*:
 * Please palce the strtou*() functions to a file under lib/ with an .h in
include/. At this stage they get linked in globally through lib/libmisc.la.

 * Can you also mention in the function docs the overflow reason for
(re-)defining our own wrappers.

src/cache_cf.cc:
 * You include <limits>. This needs to be wrapped in #if HAVE_LIMITS and
listed with the other system file includes, not the squid internal
includes.
 - If any squid .h requires it to be defined first then it MUST be
included directly by that .h to pass dependency checks.

 * "Changed from sscanf for greater flexibility for inputs and error
checking" comment is not needed.

 * Config.accessList.outgoingTos, Config.accessList.clientsideTos,
Config.accessList.outgoingNfmark, Config.accessList.clientsideNfmark can
become members of the Qos scope Config object. All the parsing /free stuff
can be moved there too with some #define parse_...() etc for the legacy
parser.

Along with Alex comment about protos, I think this unwraps any need of the
QoS stuff to include structs.h,protos.h and typedefs.h

cf.data.pre:
 * the old docs text about the above ACL tests being "based on the
username or source address making the request" is probably quite wrong.
It's based on ANY request/connection criteria is it not? and possibly reply
details as well for the reply packets.

 * please also use CIDR masks in the new documentation. class masks should
have been killed long ago.

src/ip/QosConfig.h:
 * please wrap the system includes separately, with a check for each in
configure.in.

I think thats most of it now.

Amos
Received on Mon Sep 06 2010 - 02:53:39 MDT