On Sun, 05 Sep 2010 16:52:27 -0600, Alex Rousskov <rouss...@measurement-factory.com> wrote: > 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. >
I have not yet looked at the patch, this is just some additions to the points Alex makes... > > * 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. 64-bit marks are on the table at netfilter and with TCP/IPv6 extensions. The same is not true for TCP/IPv4. I have no opinion about the change itself, but IF done use two types please. > > * 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? Indeed, those should be in the src/ip/Qos* headers somewhere. (I know the original TOS was still in protos.h, but lets take the opportunity for the extra cleanup.) Amos
