Re: Updated netfilter mark patch

On Sun, 2010-09-19 at 04:24 +1200, Amos Jeffries wrote:
> 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.

There'll be the default AC_SEARCH_LIBS notice in any case, and then it
will also be shown later assuming --enable-zph-qos is set. I've just
realised though that by default the QOS functions are disabled. I
thought the new concept was that everything was enabled by default?
Should I change the default to enabled for --enable-zph-qos?

I've also now added a $withval check anyway for netfilter-conntrack,
which ERRORs if it has been explicitly stated and the library has not
been found.

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

I couldn't see anything in syslog, unless I'm mistaken. If you're happy
as it is though, I'll leave it be.

>
> >> * 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
Received on Sat Sep 18 2010 - 17:54:18 MDT