Re: Patch to add netfilter mark support

On Mon, 2010-09-06 at 02:53 +0000, Amos Jeffries wrote:
> 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?

Yes.

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

with_netfilter_conntrack=yes now set first

netfilterconntrackpath is only set if a path has been specified

> Although
> I believe under the new design that latter should be
> squid_opt_netfilterconntrackpath

Done.

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

Done, although with slightly more detail as to setting the path.

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

Done.

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

That's in the statement above no?

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

Done with xyes to make more readable

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

Done. So if people want to specify an alternative location for the
header files, do they just do that with the appropriate CPP flags?

I was working on the basis that the path specified using
--with-netfilter=PATH would be the path to the library *and* the
headers. I've now changed it to just the library.

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

Fixed.

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

Done.

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

Done.

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

Done.

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

Done.

> * you don't need the second if statement. 'if test
> "$with_netfilter_conntrack" = "yes"'

Done.

> * SQUID_DEFINE_BOOL instead with ${with_netfilter_conntrack:=no} as the
> value to set.

Done.

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

Understood.

Comments on remainder of source code will follow later...

Andy
Received on Thu Sep 09 2010 - 19:08:45 MDT