Re: [MERGE] Initial netfilter mark patch for comment

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.

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

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

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

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

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

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

* Please document who calls getNfctMark() and when.

* 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()?

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

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

+ struct nf_conntrack *ct;
+ ct = nfct_new();
+ if (ct) {

should be written as

   if (struct nf_conntrack *ct = nfct_new()) {

The "struct nfct_handle *h" case is much worse as the declaration and
use are very far apart.

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

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

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

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

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

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

* The USE_QOS_NFMARK and USE_ZPH_QOS naming seems inconsistent.

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.

Thank you,

Alex.

> My comments are:
>
> - 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.
>
> - 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?
>
> - The code in forward.cc to obtain all the data needed to locate the
> connection information is messy. Is there a better way to achieve it?
> Conntrack needs to be passed local and remote port numbers and IP
> addresses.
>
> Is it too late to get this in v3.2? I will update the appropriate
> release-notes once I know.
Received on Mon Aug 02 2010 - 18:04:05 MDT