Re: Updated netfilter mark patch

On 19/09/10 00:47, Andrew Beverley wrote:
> 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
>>>
>> 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?

Hmm, we at least want to MSG_NOTICE for both cases, with preferrably a
hard error if its explicitly stated.

This is where the yes/no/auto comes in handy to switch the type of fail
message.

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

Aye thanks for the reminder.

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

It's logged in syslog for people who know to check there on startup.
Displayed to screen for -k check and cache.log on reconfigure.

A number of other features in this position so don't worry about it.
Fixing that tangle is outside the scope of this update.

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

Strange that bits 0,1,2 were 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.
>
> self_destruct() is now called, which IMHO is the correct thing to do.

Fine. That solves the below as well.

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

I was meaning to show both. you already had the invalid tag displayed.
But no mention of what value may have been parsed out of it.
ie ERROR: invalid tag '0x01ouch' using '0x01' instead.

Now that its going to abort instead of use anything its fine.

Amos

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