Re: Updated netfilter mark patch

On Sat, 2010-09-18 at 20:34 +1200, Amos Jeffries wrote:
> On 18/09/10 09:18, Andrew Beverley wrote:
> > Hi,
> >
> > Please find attached updated netfilter mark (and QOS tidy up) patch.
> >
> > It takes into account all the recent feedback, but leaves the
> > tcp_outgoing_* and clientside_* configuration functions in cache_cf.cc
> > as discussed on the mailing list.
> >
> > It remains not fully tested, but is provided for any further comments.
> >
> > Thanks,
> >
> > Andy
> >
>
> (sorry about the empty message).
>
> CREDITS:
> we need the full copyright extract from lib/xstrto.cc in here.

Added.

> configure.in:
> AC_SEARCH_LIBS for the library still needs to be performed. Its
> happened before that newbies see the missing-header text and copy *only*
> the header file onto their box.
> Just dropping it out of the else section next to the headers check
> should be enough.

I've moved it next to the headers check. I have also removed the error
message that was generated if they don't exist. However, this means that
if somebody explicitly sets --with-netfilter-conntrack and the libraries
don't exist, then it will silently fail. Is this the behaviour we want?

> release notes:
> * Please document clientside_mark and tcp_outgoing_mark separately.
> The segments there are auto-included into the online manuals for
> individual directives. Duplication of text (if relevant) is not a
> problem there.

Done.

> * They will then need to be shuffled up a bit. The list is meant to be
> alphabetically on directive name for easier release-notes reading.

Done. (Although the existing windows_ipaddrchangemonitor and
url_rewrite_children are the wrong way round).

> * please remove the mention that they have different dependencies that
> qos_flows. That is already covered by the docs for --with. It creates
> unnecessary questions about why they don't?

Done.

> I see that its because they only depend on SO_MARK. Which implies
> that qos_flows could be partially detached from libnetfilter-conntrack
> for the flows which set a fixed mark, only the pass-thru flow actually
> requires the library yes?

Good point. I have separated the 2 functionalities to SO_MARK and
USE_LIBNETFILTERCONNTRACK as appropriate and updated the documentation.
Only mark preservation is now unavailable without
libnetfilter_conntrack.

One problem though, is that I want to warn people if they are using the
qos_flows mark functions, if mark preservation is unavailable due to the
lack of netfilter-conntrack. I have added a warning into the config
parsing, but debugs() at this stage do not make it into the log files.
Should I just get rid of this warning or is there a better way of doing
it?

>
> src/cache_cf.cc:
> <limits> removal from the squid-headers list section. Its included
> wrapped later in the system-headers section.

Done.

> src/cf.data.pre:
> * Under tcp_outgoing_tos you can remove the "This feature is now
> compatible with persistent server connections." That is text for the
> release notes. Removing the note about problems is sufficient for the
> config file texts.

Done.

> * The texts about ECN have been found to be incorrect. It's the
> rightmost bits 6-7 which are masked out for ECN. Not the
> highest/leftmost 0-1. The trunk code has corrected text for the
> tcp_outgoing_tos but qos_flows is incorrect still. Please update your
> changes to their texts to include that fix.

Fixed, although I have actually been unable to set any bits other than
3, 4 and 5, so the only values I have been able to use are 0-28 in
multiples of 4. Does anyone know if this is the case with other
platforms? If so, that might need mentioning as well.

> * spelling "varient" is not a word. /s "variant".

Hmmm, thought I'd already fixed that. Now done.

> src/forward.cc:
> * remove the text "Persistent connections were previously incompatible
> with outgoing_tos." from the source. future dev don't care what the
> obsolete code did, just what the current code being worked with does.

Done.

> src/ip/Qos.cc:
> * can we avoid including any major namespaces particularly Store.h? it
> pulls in a vast array of dependency which cause loops in the libraries.

Done. Not sure why that was in there - must have been from previous
code.

> - please retain the NOTE stating this on dumpConfigLine()

Done.

> * looks like HierarchyLogEntry.h can be replaced by the much smaller
> heir_code.h by passing just the heir->code fields into the
> do*LocalMiss() rather than the whole logging structure.

Done. Only code now passed (by value).

> * in parseConfigLine() - your "mark" parsing #elif and #else cases
> don't match their debugs( )texts. the linux without NFMARK case says
> "only available on linux" which should be for non-linux and vice-versa.
> This could be simplified to one #else case saying "Linux netfilter
> marking not available."

Done, but with a further test for the Netfilter conntrack library, as
per previous comment.

> * for the parseConfigLine() *-hit entries you output an ERROR without
> failing or indicating what recovery action has taken place.
> ** it _looks_ at face value like the invalid value is stored for
> actual use later for TOS and ignored for mark.

self_destruct() is now called, which IMHO is the correct thing to do.

> Please add this (or
> equivalent) to the debugs: <<" using "<<markLocalHit<<" instead.");

Surely the debugs should display the invalid value that was in the
config line, rather than what was returned by xstrtoui?

> src/ip/Qos.cci:
> * Ip::Qos::Config:: isAclNfmarkActive() and isAclTosActive() need to
> be const. if anything inside either is altering state that is a bug.
> const will allow further compiler optimizations.

Done.

>
>
> ** please change defined(_SQUID_LINUX_) to just _SQUID_LINUX_ everywhere
> added.

Done.

Updated patch attached.

Thanks,

Andy