Re: Updated netfilter mark patch

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.

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.

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

   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?

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

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.
  * 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.
  * spelling "varient" is not a word. /s "variant".

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.

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.
  - please retain the NOTE stating this on dumpConfigLine()

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

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

* 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. Please add this (or
equivalent) to the debugs: <<" using "<<markLocalHit<<" instead.");

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.

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

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.8
   Beta testers wanted for 3.2.0.2