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.
