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