On 09/05/2010 02:59 PM, Andrew Beverley wrote:
Please find attached the latest version of the patch to add Netfilter
marking support to Squid.
All the previous comments have now been actioned.
One thing that I haven't dealt with yet is the dependency on the
ip_conntrack kernel module. This seems to get loaded automatically after
some use of Squid, but not straight away, which means that the mark
retention does not initially work. I've done some googling but have not
found how to force a kernel module load in a program. Is someone able to
advise please?
Since my last submission (prompted by a request on squid-users), I have
also added tcp_outgoing_mark and clientside_mark to complement
tcp_outgoing_tos and clientside_tos. I am conscious that I have been
copying old code to implement these, some of which does not seem
particularly elegant. However, rather than changing things from my
inexperienced perspective, I thought it best if I post the changes as-is
and action any feedback as appropriate.
As part of this I have added isAclNfmarkActive() and isAclTosActive() to
return whether there should be any active TOS or MARK packet marking. I
added these to fde as that seemed the most appropriate place, but again
please tell me if I should move them elsewhere.
Hi Andrew,
I cannot verify the low-level parts of your work, but I think you
have significantly improved the high-level staff. Thank you.
* I find the terminology inconsistent and confusing: outgoing,
clientside, upstream. No wonder you have to explain the difference
twice. Unless these are all standard RFC-like terms, please use
something consistent like fromClient, toClient, fromServer, toServer.
Others may suggest a better scheme, but this one at least does not
require constant doc lookups to understand where "out" and "up" is.
[Hint: In most cases, you can quickly rename things if you undo a patch,
change the names in the patch file, and apply the changed patch.]
* TOS (unsigned char) and mark (uint32_t) types are repeated numerous
times throughout the patch. Do you thin it would be a good idea to
typedef them? It seems fairly easy for somebody to type them wrong or
mix them up. I cannot insist on this change, but it would be the right
thing to do, IMO.
* If TOS and mark types can be the same uint32_t, we should probably use
the same type and avoid at least some of the code duplication due to the
difference in types.
* The isAclNfmarkActive() and isAclTosActive() functions appear to be
unrelated to src/fde.cc. They are about configuration, right? Why not
make them Qos globals or, of the lists are moved, Qos::Config methods?
* I believe Amos does not want new functions in protos.h unless
necessary. Can you declare getOutgoingTOS() and getOutgoingNfmark() in
src/forward.h? And capitalize the first letter since they are global?
* Did you verify that "make -k check" and "./test-builds.sh" did not
break because of your changes?
* Please remove the following redundant lines (no need to document what
standard methods are):
+ /**
+ * Constructor
+ */
+ /**
+ * Destructor
+ */
* s/Returns true or false depending on whether/whether/g
* Not sure, but perhaps s/whether we should carry out the QOS functions
for/whether to apply QOS functions to/. This would allow you to collapse
the comment to just one line.
* Closing paren missing in isAclNfmarkActive() description.
* You can use /// comments for /** one line comments */ to make them a
little shorter and avoid line wrapping.
Thank you,
Alex.
