Re: [MERGE] Initial netfilter mark patch for comment

On Mon, 2010-08-02 at 12:03 -0600, Alex Rousskov wrote:
> On 08/01/2010 05:47 PM, Andrew Beverley wrote:
> > Please find attached the first version of the netfilter mark patch. I've
> > not yet tested it extensively, but would welcome some initial feedback
> > or comments.

Thanks for the prompt reply. Please find attached an updated version of
the patch, which includes fixes to all the feedback below. It remains
not-extensively tested, but is attached for further comments.

> * It would be nice to get a proposed commit message describing the
> changes as a patch preamble. Among other things, this will allow
> reviewers to understand the overall scope and intent of your work.

Sorry about that, this is new to me. As you've probably gathered, the
aim of this patch is to implement the existing TOS functionality, but as
netfilter marking.

> * comm_set_mark() has a very generic name. Many things can be "marked",
> for many reasons. Can you come up with a better, more specific one?

Renamed to comm_set_nfmark.

> * comm_set_mark() is not documented but is a part of the public Comm API.
>

Now documented in the source code. Is that the only place to document?

> * getNfctMark() definition #ifdef conditions are inconsistent with its
> declaration and use #ifdefs.

Fixed.

> * getNfctMark() is static but does not start with a capital letter.

Fixed.

> * getNfctMark() might belong to fde rather than FwdState because it does
> not use FwdState. I am not sure about this one, but it may indicate a
> general design problem -- a callback with no connection to the caller.

I thought about moving to fde, but I feel it sits better in FwdState as
although it is not called directly within it, it is called as a result
of other code in there.

getNfctMark() has been renamed to GetNfMarkCallback

> * Please document who calls getNfctMark() and when.
>

Comments added.

> * If getNfctMark() is an async callback that will be called some time
> after the nfct_callback_register returns, how do you know that clientFde
> is still a valid pointer _and_ is pointing to the same connection
> information?
>
> * If getNfctMark() is an async callback that will be called at some
> random time after the nfct_callback_register returns, is it safe to use
> debugs()?

I'm pretty sure it's synchronous: if I add a 3 second delay in the
callback, then the rest of the code isn't executed until the callback
has finished, and the conntrack information is still found.

> * Please use Doxygen /** or /// comments for method descriptions.
>

Fixed.

> * Please declare local variables when you first use them, if possible.
> For example,

Fixed.

> * Please add a comment why ct = nfct_new() is not leaking memory despite
> no matching delete/free OR fix the leak.

nfct_destory() added. Thanks for pointing this out.

> * upstreamMark member documentation should be fixed. What does that
> member store/mean? I understand that you were copying the documentation
> bug from upstreamTOS, but I hope we do not have to replicate that.

upstreamMark and upstreamTOS fixed.

> * Please break out new code into a new FwdState method(s) instead of
> making FwdState::dispatch longer and uglier.

I have added 2 new methods: getUpstreamTOS() and getUpstreamNfMark()

> * Same comment applies to src/client_side_reply.cc code, including the
> old QOS code already there. It should be extracted from regular methods
> as they are getting difficult to follow, especially with all the ifdefs.

I have added tosLocalMissValue() and nfmarkLocalMissValue()

> * The new code implements a non-critical feature because errors do not
> terminate transactions. Yet, most errors are reported at level 1 to
> cache.log. We often have to modify the code to remove excessive
> cache.log pollution because it hurts busy proxies. Do you need to use
> level-1 reporting? Of every error? Or perhaps the code should just
> increment some stats counters?

I have moved most of the general errors to level 2.

> * Is there a way to defer most support checks to runtime (like
> comm_set_mark does it), to minimize the use of #ifdefs in the core code?
> For example, can we use one #ifdef variable for both USE_QOS_NFMARK and
> USE_ZPH_QOS code, in most contexts? They are intermixed in the code in a
> complex ways that I find difficult to follow.

I have simplified this and removed as many as possible. I have added
isTosActive() and isMarkActive() as suggested by Amos, and used these
instead. Some of the code relies on the libnetfilter_conntrack
libraries, so I have had to keep that inside #ifdefs.

This leads me on to my first question: why not just remove USE_ZPH_QOS
and the compilation option --enable-zph-qos? With the attached patch,
the code only runs when specified in squid.conf, and it has no other
dependencies. The ZPH kernel patch part can remain in the _SQUID_LINUX
tests.

> * The USE_QOS_NFMARK and USE_ZPH_QOS naming seems inconsistent.
>

I have renamed USE_ZPH_QOS to USE_QOS_TOS. However, see my question
above.

> The above review does not answer your questions below, and I am sorry
> for that. I hope Amos or others do better. I agree with many Amos'
> comments, and I especially encourage you to reduce and simplify #ifs and
> #ifdefs if possible, following Amos' hints.

Hopefully the code is a lot clearer now, especially with the #defs
removed. Replies to Amos' comments will follow shortly.

> > - The existing TOS patch cannot be disabled at runtime. As such, this
> > mark patch cannot be either. Would it be preferable to only enable them
> > both if the qos_flows config option is present? This would also have the
> > advantage of being able to add CAP_NET_ADMIN as appropriate at runtime.

They are now both enabled at runtime.

> > - I have used sscanf instead of strtoul for both TOS and MARK in
> > QosConfig.cc (sscanf doesn't auto-detect the format of unsigned long
> > int). As a result, the tos variable could be changed to type char, which
> > is what it should be in my opinion. Should I do this?

If we can move to strtoul, I would like to change 'tos' to char
throughout. Currently it is possible to set it to invalid values in
squid.conf, which then cause problems with dumpConfigLine.

Question number 2: what is stubQosConfig.cc? Does that also need
updating for this patch?

Many thanks,

Andy