Re: [MERGE] Initial netfilter mark patch for comment
My latest revision of the patch for netfilter marking will follow soon. Before I post it, I will reply to comments on the previous version. Question number 2: what is stubQosConfig.cc? Does that also need updating for this patch? stub* are cut down set of all non-inline Ip::QosConfig methods and any globals defined in QosConfig.h. Changes to the API need to be mirrored there. The functions inside usually call fatal() to alert a wrong linkage clearly during testing. In this particular case the parse function will need to be duplicated there. Sorry, I need some clarification on this. I've looked at most of the other stub files, and most of the functions do indeed just return fatal(). However, the existing stubQosConfig.cc is almost identical to QosConfig.cc, with almost all of its code. Shall I change the functions to fatal()? Presumably I should add all the new functions into it (getTosLocalMiss, getNfmarkLocalMiss, getTosLocalHit, getNfmarkLocalHit, getUpstreamTOS, getUpstreamNfMark, isTosActive, isMarkActive, GetNfMarkCallback)? It does need another looking at and possibly stripping down. IIRC it was there from when QosConfig was a member of SquidConfig. I'll run a quick check myself now to see what breaks in the current usage if the currently defined stubs are stripped down. You will need to add the appropriate dead-end stubs for the new functions. I've added stubs for all the non-inline functions. They just all call fatal(). If I've not got this quite right then please let me know. In this version the new methods look they should be in the Ip::Qos namespace. * the new clientReplyContext::tosLocalMissValue() and clientReplyContext::nfmarkLocalMissValue() methods particularly look like they really should take the hier code as a second parameter. Both fd and the hier if passed in should be const. I've moved them to Ip::Qos and added the parameters as const. * the new FwdState getUpstream* methods are in a similar position but not quite so clear cut. If you can find way to cleanly move them there great, otherwise it's not so much a bit deal yet. I've moved these, as well as most of the other QOS functions, into Ip::Qos. I have also removed the QosConfig namespace, as it didn't seem to fit with all these additional functions. We are moving the rest of Squid in general towards a model of Namespace::TheConfig containing the config data values pulled from squid.conf separate from the code which utilizes them. Please wait for Alex as well on this particular change. My thoughts on it are these; I like the moving of functions to IP::Qos including the existing ones. Under the coding guidelines the existence of class Qos calls for it to be in a file src/ip/Qos.h and .cc. I have renamed the files from QosConfig to Qos. As per previous discussions, the functions are now contained in a Qos namespace, and the configuration (and functions) are kept in a Config class. Several of the functions particularly the is*Active() can be inlined for better performance with _SQUID_INLINE_, which calls for a ip/Qos.cci file to define them and be conditionally included in the .h or .cc based on #if _USE_INLINE_. I've moved what I consider to be the most appropriate functions inline. If you disagree with my choices then please tell me. Some set*Mark and set*Tos functions woudl round out the API. To take the same parameters as the get*() ones, but which do the perform the setsockopt. So that paired lines of: unsigned int mark = Ip::Qos.getNfmarkLocalMiss(fd, http-request-hier); comm_set_nfmark(fd,mark); become Ip::Qos.setNfmarkLocalMiss(fd, http-request-hier); ... and thus comm.cc does not need anything about the MARK setsockopt. Mirroring the way it has nothing to do with the MARK/TOS getsockopt etc. As per other emails, I have removed get*s and set*s and replaced with do*s. I did experiment with keeping get*s and set*s and moving the get*s to private space, but in all honesty, it just complicated the code without achieving much. I think it's quite tidy now. I've carried out a fair bit of testing on the mark functionality today, and It Works For Me, but I'll be able to try it better in a production server in the coming weeks. Could you let me know if/what else I need to do before acceptance. I believe there are the following: - Confirm and implement stub function requirements - Factor the configure.in changes into Kinkie's autoconf-refactor? Those are the big ones. Along with getting the namespace change approved. The reconfigure straightening will change your dependency logic model a bit. Specifically such that the absence of --with-netfilter-conntrack (implicit / auto-detect case) results in the same auto-detect currently only done by explicit yes results, but which does not terminate in AC_MSG_ERROR. Done. The above configure
Re: [MERGE] Initial netfilter mark patch for comment
On Fri, 2010-08-13 at 18:19 -0600, Alex Rousskov wrote:
On 08/11/2010 03:25 PM, Andrew Beverley wrote:
I've moved these, as well as most of the other QOS functions, into
Ip::Qos. I have also removed the QosConfig namespace, as it didn't seem
to fit with all these additional functions.
* A patch preamble with the proposed commit message would be nice.
* I am not sure what Qos class is. It is not documented. If it is a QOS
configuration class, I understand why we have a global instance of it,
but the original QosConfig name seems better in that case. If it is not
a configuration class, then I am not sure why we have a global instance
of it. And the QosConfig file name does not seem to match the class name
any more.
Perhaps we need two classes, one for configuration and one for
manipulation? Or a Qos namespace with a configuration class and global
manipulation functions? The latter seems more likely.
The latter has now been implemented.
* My understanding is that class data members and public class methods
should be documented in the header. Others should be documented in the
.cc files. You may want to double check this rule with Amos before
moving comments though.
Comments moved to the header in Doxygen format.
* Many Qos data members are not documented, including new ones.
Now documented, and underscore removed from names.
* Pass HierarchyLogEntry by const reference, avoid copying. Once that is
done, move #include HierarchyLogEntry.h to the .cc file.
Done.
* Do you need #include fde.h in src/ip/QosConfig.h or can you
pre-declare fde and include fde.h in the .cc file?
Now pre-declared.
* s/ 0/ 0/
Done.
* This code:
+if (tos_local_hit || tos_sibling_hit || tos_parent_hit ||
preserve_miss_tos) {
+return true;
+} else {
+return false;
+}
can be simply written as
return tos_local_hit || tos_sibling_hit || tos_parent_hit ||
preserve_miss_tos;
Same for Ip::Qos::isMarkActive code.
Done.
* Ip::Qos::isTosActive and Ip::Qos::isMarkActive should be const. When
that is fixed, you would be able to return const to
Ip::Qos::dumpConfigLine, I guess.
Done.
* Ip::Qos::getNfmarkLocalMiss and many other get*() methods should be const.
Most of these have now been moved out of classes.
* What is the purpose of memsetting Qos members to zero in the
destructor? Please remove the destructor itself if there is no reason to
reset the memory before freeing it.
Removed.
* Use Doxygen /// comments when documenting members, such as upstreamTOS.
Done.
* Do you need an L suffix for large unsigned constants like 0x?
Please investigate. I do not know the answer, but I recall seeing such
suffixes elsewhere:
http://www.google.com/search?q=0x+vs+0xL
From what I can gather, the suffix is important in places like 'if'
statements when there is no assignment to a variable with a type (so
that you are comparing like with like). Therefore, I have added the U
suffix to 'if' statements, but I see no need when assigning values to
defined variables (although there would of course be no harm in doing
so).
Regards,
Andy
Re: [MERGE] Initial netfilter mark patch for comment
On 08/21/2010 05:45 PM, Henrik Nordström wrote:
lör 2010-08-21 klockan 23:41 +0100 skrev Andrew Beverley:
I have documented all the functions and class data members. Could you
clarify whether *every* variable should be documented with doxygen
comments (including short-lived temporary ones within functions), or
just those that are part of classes/structs?
classes/structs.
temporary variables if their use may not be obvious to someone else
reading your code, but preferably code in such shape that variable use
is obvious.
For example, should 'tos' in the function below have doxygen comments?
No need for that imho.
Agreed. Especially true if you select meaningful names for local
variables (e.g., headerBuf instead of b or ipCount instead of c).
BTW, if the documentation comment simply spells out the variable name,
it is worse than no comment. For example,
private:
int isFoo; /// whether we are Foo
is useless but something like
int isFoo; /// caller expects Bar in the result
may shed more light than the variable name itself. Perhaps my
foo/bar-based example is too abstract to make sense, but try to explain
what the variable/function does to a person who could not guess it
correctly from the name alone or, if the name is really obvious/clear,
supply additional information instead of repeating (spelling out) the name.
Also, there is no requirement to document API implementation methods in
kids unless you want to:
/// such and such API
class Parent {
/// checks computations results for exceeding INT_MAX
virtual bool willFit() const = 0;
virtual int calc() = 0; /// ...
...
};
/// Foo and bar, using such and such API
class Child: pubic Parent {
...
/* Parent API */
virtual bool willFit() const;
virtual int calc();
...
};
HTH,
Alex.
Re: [MERGE] Initial netfilter mark patch for comment
On Fri, 2010-08-20 at 12:44 -0600, Alex Rousskov wrote: On 08/20/2010 11:06 AM, Andrew Beverley wrote: On Fri, 2010-08-13 at 18:19 -0600, Alex Rousskov wrote: On 08/11/2010 03:25 PM, Andrew Beverley wrote: I've moved these, as well as most of the other QOS functions, into Ip::Qos. I have also removed the QosConfig namespace, as it didn't seem to fit with all these additional functions. * A patch preamble with the proposed commit message would be nice. * I am not sure what Qos class is. It is not documented. If it is a QOS configuration class, I understand why we have a global instance of it, but the original QosConfig name seems better in that case. If it is not a configuration class, then I am not sure why we have a global instance of it. And the QosConfig file name does not seem to match the class name any more. Perhaps we need two classes, one for configuration and one for manipulation? Or a Qos namespace with a configuration class and global manipulation functions? The latter seems more likely. I was trying to copy the approach used by the Interceptor class, which has its configuration variables and methods in one class, with a global instance of it. I thought that it would keep things nice and tidy by having all qos aspects in one single class (with the configuration variables within private space). Trust me, I feel your pain. Copying existing code on its own does not guarantee much because a lot of code is broken, unfortunately. [ I am not saying Interceptor class is or is not broken because I have not reviewed it. ] I'm happy to go with whatever approach you want though, and can separate the config aspects back out from the functions. Before I go ahead and do this, I want to check which option I should go for. As Alex says, the options are: 1) - Reinstate the QosConfig class with the configuration variables as public members - Create a new class (QosFunctions?) with the relevant functions in - Create one global instance of each class, Rule of thumb: If you can use a namespace instead of a class, use a namespace. For example, classes that only group related stateless functions are usually a bad idea. or alternatively create a new instance of the functions class each time the functions need to be accessed (is the latter inefficient?) It is difficult for me to imagine a worse design (one class/instance per function, perhaps?), but I am sure you can find it in Squid :-). 2) - Reinstate the QosConfig class with the configuration variables as public members - Create the functions as global functions in the namespace This is the best option if functions do not share or keep state. Please note that QosConfig should become Qos::Config once you add Qos namespace. 3) - Keep everything in one class :-) If going with option 2, the functions can no longer be const (which most of them currently are). Well, if your methods do not share or keep state, they should not be const. They should be static! Const means I use but do not modify the state of my object. And if a class has nothing but static methods, it should be a namespace in most cases... In summary, * Work inside Qos namespace. * Keep configuration state (data) inside Qos::Config class. You can use a single global Qos::TheConfig current configuration instance of that class if needed. * Functions that exist to interpret or modify configuration should be Qos::Config methods (const if possible). * Functions that wrap library calls or otherwise manipulate some external state/data that they do not and cannot own should be declared outside Qos::Config and inside Qos. * If there is a non-configuration state/data that you can encapsulate and manipulate in a class setting, then create more Qos::* classes. Thanks Alex. An excellent explanation. I fully understand now, and have gone with option 2 as you suggest, just keeping the isActive functions in the config class. Updated patch to follow soon. Andy
Re: [MERGE] Initial netfilter mark patch for comment
* My understanding is that class data members and public class methods
should be documented in the header. Others should be documented in the
.cc files. You may want to double check this rule with Amos before
moving comments though.
* Many Qos data members are not documented, including new ones.
I have documented all the functions and class data members. Could you
clarify whether *every* variable should be documented with doxygen
comments (including short-lived temporary ones within functions), or
just those that are part of classes/structs?
For example, should 'tos' in the function below have doxygen comments?
int
Ip::Qos::doTosLocalMiss(const int fd, const HierarchyLogEntry *hier)
{
unsigned char tos = 0;
if (Ip::Qos::TheConfig.tos_sibling_hit hier-code==SIBLING_HIT ) {
tos = Ip::Qos::TheConfig.tos_sibling_hit;
debugs(33, 2, QOS: Sibling Peer hit with hier code= hier-code
, TOS= (int)tos);
} else if (Ip::Qos::TheConfig.tos_parent_hit hier-code==PARENT_HIT) {
tos = Ip::Qos::TheConfig.tos_parent_hit;
debugs(33, 2, QOS: Parent Peer hit with hier code= hier-code
, TOS= (int)tos);
} else if (Ip::Qos::TheConfig.preserve_miss_tos
Ip::Qos::TheConfig.preserve_miss_tos_mask) {
tos = fd_table[fd].upstreamTOS
Ip::Qos::TheConfig.preserve_miss_tos_mask;
debugs(33, 2, QOS: Preserving TOS on miss, TOS= int(tos));
}
return setSockTos(fd, tos);
}
Thanks,
Andy
Re: [MERGE] Initial netfilter mark patch for comment
lör 2010-08-21 klockan 23:41 +0100 skrev Andrew Beverley: I have documented all the functions and class data members. Could you clarify whether *every* variable should be documented with doxygen comments (including short-lived temporary ones within functions), or just those that are part of classes/structs? classes/structs. temporary variables if their use may not be obvious to someone else reading your code, but preferably code in such shape that variable use is obvious. For example, should 'tos' in the function below have doxygen comments? No need for that imho. Regards Henrik
Re: [MERGE] Initial netfilter mark patch for comment
On Fri, 2010-08-13 at 18:19 -0600, Alex Rousskov wrote: On 08/11/2010 03:25 PM, Andrew Beverley wrote: I've moved these, as well as most of the other QOS functions, into Ip::Qos. I have also removed the QosConfig namespace, as it didn't seem to fit with all these additional functions. * A patch preamble with the proposed commit message would be nice. * I am not sure what Qos class is. It is not documented. If it is a QOS configuration class, I understand why we have a global instance of it, but the original QosConfig name seems better in that case. If it is not a configuration class, then I am not sure why we have a global instance of it. And the QosConfig file name does not seem to match the class name any more. Perhaps we need two classes, one for configuration and one for manipulation? Or a Qos namespace with a configuration class and global manipulation functions? The latter seems more likely. I was trying to copy the approach used by the Interceptor class, which has its configuration variables and methods in one class, with a global instance of it. I thought that it would keep things nice and tidy by having all qos aspects in one single class (with the configuration variables within private space). I'm happy to go with whatever approach you want though, and can separate the config aspects back out from the functions. Before I go ahead and do this, I want to check which option I should go for. As Alex says, the options are: 1) - Reinstate the QosConfig class with the configuration variables as public members - Create a new class (QosFunctions?) with the relevant functions in - Create one global instance of each class, or alternatively create a new instance of the functions class each time the functions need to be accessed (is the latter inefficient?) 2) - Reinstate the QosConfig class with the configuration variables as public members - Create the functions as global functions in the namespace 3) - Keep everything in one class :-) If going with option 2, the functions can no longer be const (which most of them currently are). Thanks, Andy
Re: [MERGE] Initial netfilter mark patch for comment
Andrew, Seems the netfilter guys found a major problem with strtoul(). Thankfully the same fix should work for us as well. Luciano Coelho wrote: snip Not easily. I found that there is a bug in strtoul (and strtoull for that matter) that causes the long to overflow if there are valid digits after the maximum possible digits for the base. For example if you try to strtoul 0xf (with 9 f's) the strtoul will overflow and come up with a bogus result. I can't easily truncate the string to avoid this problem, because with decimal or octal, the same valid value would take more spaces. I could do some magic here, checking whether it's a hex, dec or oct and truncate appropriately, but that would be very ugly. So the simplest way I came up with was to use strtoull and return -EINVAL if the value exceeds 32 bits. ;) Amos -- Please be using Current Stable Squid 2.7.STABLE9 or 3.1.6 Beta testers wanted for 3.2.0.1
Re: [MERGE] Initial netfilter mark patch for comment
Seems the netfilter guys found a major problem with strtoul(). Thankfully the same fix should work for us as well. Luciano Coelho wrote: snip Not easily. I found that there is a bug in strtoul (and strtoull for that matter) that causes the long to overflow if there are valid digits after the maximum possible digits for the base. For example if you try to strtoul 0xf (with 9 f's) the strtoul will overflow and come up with a bogus result. I can't easily truncate the string to avoid this problem, because with decimal or octal, the same valid value would take more spaces. I could do some magic here, checking whether it's a hex, dec or oct and truncate appropriately, but that would be very ugly. So the simplest way I came up with was to use strtoull and return -EINVAL if the value exceeds 32 bits. ;) Thanks for that Amos. I have to admit that when I was doing my initial browse of the iptables mark module, I noticed that they had their own xtables_strtoui function which limited the size. I'll check this out a bit more thoroughly. I'm away at the moment with minimal internet access, but I hope to pick up again on the patch shortly. Cheers, Andy
Re: [MERGE] Initial netfilter mark patch for comment
On 08/11/2010 03:25 PM, Andrew Beverley wrote:
I've moved these, as well as most of the other QOS functions, into
Ip::Qos. I have also removed the QosConfig namespace, as it didn't seem
to fit with all these additional functions.
* A patch preamble with the proposed commit message would be nice.
* I am not sure what Qos class is. It is not documented. If it is a QOS
configuration class, I understand why we have a global instance of it,
but the original QosConfig name seems better in that case. If it is not
a configuration class, then I am not sure why we have a global instance
of it. And the QosConfig file name does not seem to match the class name
any more.
Perhaps we need two classes, one for configuration and one for
manipulation? Or a Qos namespace with a configuration class and global
manipulation functions? The latter seems more likely.
* My understanding is that class data members and public class methods
should be documented in the header. Others should be documented in the
.cc files. You may want to double check this rule with Amos before
moving comments though.
* Many Qos data members are not documented, including new ones.
* Pass HierarchyLogEntry by const reference, avoid copying. Once that is
done, move #include HierarchyLogEntry.h to the .cc file.
* Do you need #include fde.h in src/ip/QosConfig.h or can you
pre-declare fde and include fde.h in the .cc file?
* s/ 0/ 0/
* This code:
+if (tos_local_hit || tos_sibling_hit || tos_parent_hit ||
preserve_miss_tos) {
+return true;
+} else {
+return false;
+}
can be simply written as
return tos_local_hit || tos_sibling_hit || tos_parent_hit ||
preserve_miss_tos;
Same for Ip::Qos::isMarkActive code.
* Ip::Qos::isTosActive and Ip::Qos::isMarkActive should be const. When
that is fixed, you would be able to return const to
Ip::Qos::dumpConfigLine, I guess.
* Ip::Qos::getNfmarkLocalMiss and many other get*() methods should be const.
* What is the purpose of memsetting Qos members to zero in the
destructor? Please remove the destructor itself if there is no reason to
reset the memory before freeing it.
* Use Doxygen /// comments when documenting members, such as upstreamTOS.
* Do you need an L suffix for large unsigned constants like 0x?
Please investigate. I do not know the answer, but I recall seeing such
suffixes elsewhere:
http://www.google.com/search?q=0x+vs+0xL
Thank you,
Alex.
Re: [MERGE] Initial netfilter mark patch for comment
Alex Rousskov wrote:
On 08/11/2010 03:25 PM, Andrew Beverley wrote:
I've moved these, as well as most of the other QOS functions, into
Ip::Qos. I have also removed the QosConfig namespace, as it didn't seem
to fit with all these additional functions.
* A patch preamble with the proposed commit message would be nice.
* I am not sure what Qos class is. It is not documented. If it is a QOS
configuration class, I understand why we have a global instance of it,
but the original QosConfig name seems better in that case. If it is not
a configuration class, then I am not sure why we have a global instance
of it. And the QosConfig file name does not seem to match the class name
any more.
Perhaps we need two classes, one for configuration and one for
manipulation? Or a Qos namespace with a configuration class and global
manipulation functions? The latter seems more likely.
* My understanding is that class data members and public class methods
should be documented in the header. Others should be documented in the
.cc files. You may want to double check this rule with Amos before
moving comments though.
true.
* Many Qos data members are not documented, including new ones.
* Pass HierarchyLogEntry by const reference, avoid copying. Once that is
done, move #include HierarchyLogEntry.h to the .cc file.
* Do you need #include fde.h in src/ip/QosConfig.h or can you
pre-declare fde and include fde.h in the .cc file?
* s/ 0/ 0/
* This code:
+if (tos_local_hit || tos_sibling_hit || tos_parent_hit ||
preserve_miss_tos) {
+return true;
+} else {
+return false;
+}
can be simply written as
return tos_local_hit || tos_sibling_hit || tos_parent_hit ||
preserve_miss_tos;
Same for Ip::Qos::isMarkActive code.
NP: with () brackets for easier reading of the compound statement please.
* Ip::Qos::isTosActive and Ip::Qos::isMarkActive should be const. When
that is fixed, you would be able to return const to
Ip::Qos::dumpConfigLine, I guess.
* Ip::Qos::getNfmarkLocalMiss and many other get*() methods should be
const.
* What is the purpose of memsetting Qos members to zero in the
destructor? Please remove the destructor itself if there is no reason to
reset the memory before freeing it.
* Use Doxygen /// comments when documenting members, such as upstreamTOS.
* Do you need an L suffix for large unsigned constants like 0x?
Please investigate. I do not know the answer, but I recall seeing such
suffixes elsewhere:
http://www.google.com/search?q=0x+vs+0xL
I thought that indicated long type to be used. When stored as 64-bit
values.
Which brings up a question of whether it really is an architecture
dependent int field or uint32_t for mark.
Amos
--
Please be using
Current Stable Squid 2.7.STABLE9 or 3.1.6
Beta testers wanted for 3.2.0.1
Re: [MERGE] Initial netfilter mark patch for comment
stub* are cut down set of all non-inline Ip::QosConfig methods and any globals defined in QosConfig.h. Changes to the API need to be mirrored there. The functions inside usually call fatal() to alert a wrong linkage clearly during testing. In this particular case the parse function will need to be duplicated there. Sorry, I need some clarification on this. I've looked at most of the other stub files, and most of the functions do indeed just return fatal(). However, the existing stubQosConfig.cc is almost identical to QosConfig.cc, with almost all of its code. Shall I change the functions to fatal()? Presumably I should add all the new functions into it (getTosLocalMiss, getNfmarkLocalMiss, getTosLocalHit, getNfmarkLocalHit, getUpstreamTOS, getUpstreamNfMark, isTosActive, isMarkActive, GetNfMarkCallback)? It does need another looking at and possibly stripping down. IIRC it was there from when QosConfig was a member of SquidConfig. I'll run a quick check myself now to see what breaks in the current usage if the currently defined stubs are stripped down. You will need to add the appropriate dead-end stubs for the new functions. Okay, I'll work on a stripped down fatal() for the time being. I've moved these, as well as most of the other QOS functions, into Ip::Qos. I have also removed the QosConfig namespace, as it didn't seem to fit with all these additional functions. We are moving the rest of Squid in general towards a model of Namespace::TheConfig containing the config data values pulled from squid.conf separate from the code which utilizes them. Please wait for Alex as well on this particular change. I did consider having the config values in a separate Config class, and I'm happy to change to this, I just thought that it was neater having them in the same class. It also means that the configuration values do not need to be public, which I thought was generally a Good Thing. My thoughts on it are these; I like the moving of functions to IP::Qos including the existing ones. Under the coding guidelines the existence of class Qos calls for it to be in a file src/ip/Qos.h and .cc. Several of the functions particularly the is*Active() can be inlined for better performance with _SQUID_INLINE_, which calls for a ip/Qos.cci file to define them and be conditionally included in the .h or .cc based on #if _USE_INLINE_. No problem. My initial thoughts are that I should do get*LocalMiss, get*LocalHit and is*Active (and possibly the new set functions). Some set*Mark and set*Tos functions woudl round out the API. To take the same parameters as the get*() ones, but which do the perform the setsockopt. So that paired lines of: unsigned int mark = Ip::Qos.getNfmarkLocalMiss(fd, http-request-hier); comm_set_nfmark(fd,mark); become Ip::Qos.setNfmarkLocalMiss(fd, http-request-hier); ... and thus comm.cc does not need anything about the MARK setsockopt. Mirroring the way it has nothing to do with the MARK/TOS getsockopt etc. I like that; I'll make the changes. I guess there is then no need for getTosLocalHit() and getNfMarkLocalHit(), as all they do is return the appropriate private variables, although I could leave them there to keep everything consistent. I've carried out a fair bit of testing on the mark functionality today, and It Works For Me, but I'll be able to try it better in a production server in the coming weeks. Could you let me know if/what else I need to do before acceptance. I believe there are the following: - Confirm and implement stub function requirements - Factor the configure.in changes into Kinkie's autoconf-refactor? Those are the big ones. Along with getting the namespace change approved. The reconfigure straightening will change your dependency logic model a bit. Specifically such that the absence of --with-netfilter-conntrack (implicit / auto-detect case) results in the same auto-detect currently only done by explicit yes results, but which does not terminate in AC_MSG_ERROR. No problem. Any idea when the autoconf-refactor will appear in trunk? Andy
Re: [MERGE] Initial netfilter mark patch for comment
Andrew Beverley wrote: stub* are cut down set of all non-inline Ip::QosConfig methods and any globals defined in QosConfig.h. Changes to the API need to be mirrored there. The functions inside usually call fatal() to alert a wrong linkage clearly during testing. In this particular case the parse function will need to be duplicated there. Sorry, I need some clarification on this. I've looked at most of the other stub files, and most of the functions do indeed just return fatal(). However, the existing stubQosConfig.cc is almost identical to QosConfig.cc, with almost all of its code. Shall I change the functions to fatal()? Presumably I should add all the new functions into it (getTosLocalMiss, getNfmarkLocalMiss, getTosLocalHit, getNfmarkLocalHit, getUpstreamTOS, getUpstreamNfMark, isTosActive, isMarkActive, GetNfMarkCallback)? It does need another looking at and possibly stripping down. IIRC it was there from when QosConfig was a member of SquidConfig. I'll run a quick check myself now to see what breaks in the current usage if the currently defined stubs are stripped down. You will need to add the appropriate dead-end stubs for the new functions. Okay, I'll work on a stripped down fatal() for the time being. I've moved these, as well as most of the other QOS functions, into Ip::Qos. I have also removed the QosConfig namespace, as it didn't seem to fit with all these additional functions. We are moving the rest of Squid in general towards a model of Namespace::TheConfig containing the config data values pulled from squid.conf separate from the code which utilizes them. Please wait for Alex as well on this particular change. I did consider having the config values in a separate Config class, and I'm happy to change to this, I just thought that it was neater having them in the same class. It also means that the configuration values do not need to be public, which I thought was generally a Good Thing. Yes. We've thrown various ideas about how to do a live zero-downtime reconfigure around. TheConfig were working towards those. That set are in the gap really. I suppose it depends on it matters if one of the marks is set at the top of a big config and another at the end. The way you have them as privates for now should be okay. Worst case we abstract to a public again later as needed. My thoughts on it are these; I like the moving of functions to IP::Qos including the existing ones. Under the coding guidelines the existence of class Qos calls for it to be in a file src/ip/Qos.h and .cc. Several of the functions particularly the is*Active() can be inlined for better performance with _SQUID_INLINE_, which calls for a ip/Qos.cci file to define them and be conditionally included in the .h or .cc based on #if _USE_INLINE_. No problem. My initial thoughts are that I should do get*LocalMiss, get*LocalHit and is*Active (and possibly the new set functions). Some set*Mark and set*Tos functions woudl round out the API. To take the same parameters as the get*() ones, but which do the perform the setsockopt. So that paired lines of: unsigned int mark = Ip::Qos.getNfmarkLocalMiss(fd, http-request-hier); comm_set_nfmark(fd,mark); become Ip::Qos.setNfmarkLocalMiss(fd, http-request-hier); ... and thus comm.cc does not need anything about the MARK setsockopt. Mirroring the way it has nothing to do with the MARK/TOS getsockopt etc. I like that; I'll make the changes. I guess there is then no need for getTosLocalHit() and getNfMarkLocalHit(), as all they do is return the appropriate private variables, although I could leave them there to keep everything consistent. Hmm, if they are fully unused then it makes more sense to drop and call the set*() methods do*(). I've carried out a fair bit of testing on the mark functionality today, and It Works For Me, but I'll be able to try it better in a production server in the coming weeks. Could you let me know if/what else I need to do before acceptance. I believe there are the following: - Confirm and implement stub function requirements - Factor the configure.in changes into Kinkie's autoconf-refactor? Those are the big ones. Along with getting the namespace change approved. The reconfigure straightening will change your dependency logic model a bit. Specifically such that the absence of --with-netfilter-conntrack (implicit / auto-detect case) results in the same auto-detect currently only done by explicit yes results, but which does not terminate in AC_MSG_ERROR. No problem. Any idea when the autoconf-refactor will appear in trunk? Um two days ago. :) Sign of a good clean merge: nobody notices. Amos -- Please be using Current Stable Squid 2.7.STABLE9 or 3.1.6 Beta testers wanted for 3.2.0.1
Re: [MERGE] Initial netfilter mark patch for comment
Updated patch attached; comments below.
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?
stub* are cut down set of all non-inline Ip::QosConfig methods and any
globals defined in QosConfig.h. Changes to the API need to be mirrored
there. The functions inside usually call fatal() to alert a wrong
linkage clearly during testing. In this particular case the parse
function will need to be duplicated there.
Sorry, I need some clarification on this. I've looked at most of the
other stub files, and most of the functions do indeed just return
fatal(). However, the existing stubQosConfig.cc is almost identical to
QosConfig.cc, with almost all of its code. Shall I change the functions
to fatal()?
Presumably I should add all the new functions into it (getTosLocalMiss,
getNfmarkLocalMiss, getTosLocalHit, getNfmarkLocalHit, getUpstreamTOS,
getUpstreamNfMark, isTosActive, isMarkActive, GetNfMarkCallback)?
I have failed to find any problems with strtoul. Looks like it can be
used. Although we may have to find a GPLv2 compatible implementation.
I have kept the strtoul parts, and changed (almost) all of the tos
variables to unsigned char.
In this version the new methods look they should be in the Ip::Qos
namespace.
* the new clientReplyContext::tosLocalMissValue() and
clientReplyContext::nfmarkLocalMissValue() methods particularly look
like they really should take the hier code as a second parameter. Both
fd and the hier if passed in should be const.
I've moved them to Ip::Qos and added the parameters as const.
* the new FwdState getUpstream* methods are in a similar position but
not quite so clear cut. If you can find way to cleanly move them there
great, otherwise it's not so much a bit deal yet.
I've moved these, as well as most of the other QOS functions, into
Ip::Qos. I have also removed the QosConfig namespace, as it didn't seem
to fit with all these additional functions.
* Thank you for the extra documentation on comm_set_tos().
* Please remove the bits touching comm_set_v6only() its 'tos' field is
not related to QoS. Just acronym confusion.
My mistake, removed.
I've carried out a fair bit of testing on the mark functionality today,
and It Works For Me, but I'll be able to try it better in a production
server in the coming weeks.
Could you let me know if/what else I need to do before acceptance. I
believe there are the following:
- Confirm and implement stub function requirements
- Factor the configure.in changes into Kinkie's autoconf-refactor?
Regards,
Andy
=== modified file 'configure.in'
--- configure.in 2010-08-10 15:37:53 +
+++ configure.in 2010-08-11 13:32:45 +
@@ -1112,6 +1112,34 @@
fi
AC_SUBST(SSLLIB)
+dnl Allow user to specify libnetfilter_conntrack (needed for QOS netfilter marking)
+AC_ARG_WITH(netfilter-conntrack,
+ AS_HELP_STRING([--with-netfilter-conntrack=PATH],
+ [Compile with the Netfilter conntrack libraries. The path to
+ the development libraries and headers
+ installation can be specified if outside of the
+ system standard directories]), [
+case $with_netfilter_conntrack in
+ no)
+: # Nothing special to do here
+;;
+ yes)
+AC_CHECK_LIB([netfilter_conntrack], [nfct_query],,
+ AC_MSG_ERROR([libnetfilter-conntrack library not found. Needed for netfilter-conntrack support]),
+ [-lnetfilter_conntrack])
+AC_CHECK_HEADERS([libnetfilter_conntrack/libnetfilter_conntrack.h \
+ libnetfilter_conntrack/libnetfilter_conntrack_tcp.h])
+;;
+ *)
+if test ! -d $withval ; then
+ AC_MSG_ERROR([--with-netfilter-conntrack path does not point to a directory])
+fi
+LDFLAGS=-L$with_netfilter_conntrack/lib $LDFLAGS
+CPPFLAGS=-I$with_netfilter_conntrack/include $CPPFLAGS
+with_netfilter_conntrack=yes
+;;
+ esac
+])
AC_ARG_ENABLE(forw-via-db,
AS_HELP_STRING([--enable-forw-via-db],[Enable Forw/Via database]), [
@@ -1990,10 +2018,19 @@
SQUID_YESNO([$enableval],
[unrecognized argument to --enable-zph-qos: $enableval])
])
-SQUID_DEFINE_BOOL(USE_ZPH_QOS,${enable_zph_qos:=no},
+SQUID_DEFINE_BOOL(USE_QOS_TOS,${enable_zph_qos:=no},
[Enable Zero Penalty Hit QOS. When set, Squid will alter the
TOS field of HIT responses to help policing network traffic])
AC_MSG_NOTICE([ZPH QOS enabled: $enable_zph_qos])
+if test $enable_zph_qos = yes ; then
+if test $with_netfilter_conntrack = yes ; then
+AC_MSG_NOTICE([QOS netfilter marking enabled: $with_netfilter_conntrack])
+SQUID_DEFINE_BOOL(USE_QOS_NFMARK,$with_netfilter_conntrack,
+ [Enable support for QOS
Re: [MERGE] Initial netfilter mark patch for comment
On Wed, 11 Aug 2010 22:25:45 +0100, Andrew Beverley a...@andybev.com wrote: Updated patch attached; comments below. 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? stub* are cut down set of all non-inline Ip::QosConfig methods and any globals defined in QosConfig.h. Changes to the API need to be mirrored there. The functions inside usually call fatal() to alert a wrong linkage clearly during testing. In this particular case the parse function will need to be duplicated there. Sorry, I need some clarification on this. I've looked at most of the other stub files, and most of the functions do indeed just return fatal(). However, the existing stubQosConfig.cc is almost identical to QosConfig.cc, with almost all of its code. Shall I change the functions to fatal()? Presumably I should add all the new functions into it (getTosLocalMiss, getNfmarkLocalMiss, getTosLocalHit, getNfmarkLocalHit, getUpstreamTOS, getUpstreamNfMark, isTosActive, isMarkActive, GetNfMarkCallback)? It does need another looking at and possibly stripping down. IIRC it was there from when QosConfig was a member of SquidConfig. I'll run a quick check myself now to see what breaks in the current usage if the currently defined stubs are stripped down. You will need to add the appropriate dead-end stubs for the new functions. In this version the new methods look they should be in the Ip::Qos namespace. * the new clientReplyContext::tosLocalMissValue() and clientReplyContext::nfmarkLocalMissValue() methods particularly look like they really should take the hier code as a second parameter. Both fd and the hier if passed in should be const. I've moved them to Ip::Qos and added the parameters as const. * the new FwdState getUpstream* methods are in a similar position but not quite so clear cut. If you can find way to cleanly move them there great, otherwise it's not so much a bit deal yet. I've moved these, as well as most of the other QOS functions, into Ip::Qos. I have also removed the QosConfig namespace, as it didn't seem to fit with all these additional functions. We are moving the rest of Squid in general towards a model of Namespace::TheConfig containing the config data values pulled from squid.conf separate from the code which utilizes them. Please wait for Alex as well on this particular change. My thoughts on it are these; I like the moving of functions to IP::Qos including the existing ones. Under the coding guidelines the existence of class Qos calls for it to be in a file src/ip/Qos.h and .cc. Several of the functions particularly the is*Active() can be inlined for better performance with _SQUID_INLINE_, which calls for a ip/Qos.cci file to define them and be conditionally included in the .h or .cc based on #if _USE_INLINE_. Some set*Mark and set*Tos functions woudl round out the API. To take the same parameters as the get*() ones, but which do the perform the setsockopt. So that paired lines of: unsigned int mark = Ip::Qos.getNfmarkLocalMiss(fd, http-request-hier); comm_set_nfmark(fd,mark); become Ip::Qos.setNfmarkLocalMiss(fd, http-request-hier); ... and thus comm.cc does not need anything about the MARK setsockopt. Mirroring the way it has nothing to do with the MARK/TOS getsockopt etc. I've carried out a fair bit of testing on the mark functionality today, and It Works For Me, but I'll be able to try it better in a production server in the coming weeks. Could you let me know if/what else I need to do before acceptance. I believe there are the following: - Confirm and implement stub function requirements - Factor the configure.in changes into Kinkie's autoconf-refactor? Those are the big ones. Along with getting the namespace change approved. The reconfigure straightening will change your dependency logic model a bit. Specifically such that the absence of --with-netfilter-conntrack (implicit / auto-detect case) results in the same auto-detect currently only done by explicit yes results, but which does not terminate in AC_MSG_ERROR. Amos
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
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.
[MERGE] Initial netfilter mark patch for comment
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. 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. Thanks, Andy # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: a...@andybev.com-20100801233324-uoy4sp2e3uszw5qn # target_branch: file:///home/andrew/squid-repo/trunk/ # testament_sha1: 4bd1e0b517dc3322fa114f30fb26ff2483cb3410 # timestamp: 2010-08-02 00:34:42 +0100 # base_revision_id: squ...@treenet.co.nz-20100801134158-\ # vhc003yv3ikwao7e # # Begin patch === modified file 'configure.in' --- configure.in 2010-08-01 06:29:48 + +++ configure.in 2010-08-01 20:09:13 + @@ -1146,6 +1146,32 @@ fi AC_SUBST(SSLLIB) +dnl Allow user to specify libnetfilter_conntrack (needed for QOS netfilter marking) +AC_ARG_WITH(netfilter-conntrack, + AS_HELP_STRING([--with-netfilter-conntrack=PATH], + [Compile with the Netfilter conntrack libraries. The path to + the development libraries and headers + installation can be specified if outside of the + system standard directories]), [ +case $with_netfilter_conntrack in + no) +: # Nothing special to do here +;; + yes) +AC_CHECK_LIB([netfilter_conntrack], [nfct_query],, + AC_MSG_ERROR([libnetfilter-conntrack library not found. Needed for netfilter-conntrack support]), + [-lnetfilter_conntrack]) +;; + *) +if test ! -d $withval ; then + AC_MSG_ERROR([--with-netfilter-conntrack path does not point to a directory]) +fi +LDFLAGS=-L$with_netfilter_conntrack/lib $LDFLAGS +CPPFLAGS=-I$with_netfilter_conntrack/include $CPPFLAGS +with_netfilter_conntrack=yes +;; + esac +]) AC_ARG_ENABLE(forw-via-db, AS_HELP_STRING([--enable-forw-via-db],[Enable Forw/Via database]), [ @@ -2061,6 +2087,15 @@ [Enable Zero Penalty Hit QOS. When set, Squid will alter the TOS field of HIT responses to help policing network traffic]) AC_MSG_NOTICE([ZPH QOS enabled: $enable_zph_qos]) +if test $enable_zph_qos = yes ; then +if test $with_netfilter_conntrack = yes ; then +SQUID_DEFINE_BOOL(USE_QOS_NFMARK,$with_netfilter_conntrack, + [Enable support for QOS netfilter packet marking]) +else +AC_MSG_WARN([--with-netfilter-conntrack not enabled. QOS features will not support Netfilter marking.]) +fi +AC_MSG_NOTICE([QOS netfilter marking enabled: $with_netfilter_conntrack]) +fi dnl --with-maxfd present for compatibility with Squid-2. dnl undocumented in ./configure --help to encourage using the Squid-3 directive. === modified file 'src/cf.data.pre' --- src/cf.data.pre 2010-07-29 13:04:44 + +++ src/cf.data.pre 2010-08-01 20:09:13 + @@ -1532,18 +1532,23 @@ LOC: Ip::Qos::TheConfig DOC_START Allows you to select a TOS/DSCP value to mark outgoing - connections with, based on where the reply was sourced. + connections with, based on where the reply was sourced. For + platforms using netfilter, allows you to set a netfilter mark + value instead of, or in addition to, a TOS value. TOS values really only have local significance - so you should know what you're specifying. For more information, see RFC2474, RFC2475, and RFC3260. The TOS/DSCP byte must be exactly that - octet value 0x00-0xFF. - Note that in practice often only values up to 0x3F are usable - as the two highest bits have been redefined for use by ECN - (RFC3168). - - This setting is configured by setting the source TOS values: + Note that in practice often only values up to 0x3F are usable as + the two highest bits have been redefined for use by ECN (RFC3168). + +Mark values can be any unsigned integer value (normally up to 0x) + + This setting is configured by setting the following values: + +tos|markWhether to set TOS or netfilter mark values local-hit=0xFF Value to mark local cache hits. @@ -1551,23 +1556,26 @@ parent-hit=0xFF Value to mark hits from parent peers. - - NOTE: 'miss' preserve feature is only
Re: [MERGE] Initial netfilter mark patch for comment
On Mon, 02 Aug 2010 00:47:15 +0100, Andrew Beverley a...@andybev.com
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.
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.
The mess around local port can be cleaned up (see comments below). The
remote ip:port mess seems as clean as it can be in the current code.
Is it too late to get this in v3.2? I will update the appropriate
release-notes once I know.
Not too late. This can come under a header of polish on the existing QoS
feature. We have a a bit more flexibility on that kind of change.
The changes to qos_flows needs to be mentioned in section 3.2 changes to
existing tags and the --with option to section 4.2 new ./configure
options. Both are alphabetical lists.
Audit Results:
configure.in: chunk line 2087
* QOS netfilter marking enabled: $with_netfilter_conntrack if ZPH QoS
is enabled, but regardless of whether NFMARK was defined.
The notice needs to be inside the top part of your inner if statement.
* Please also add an
AC_CHECK_HEADERS([libnetfilter_conntrack/libnetfilter_conntrack.h
libnetfilter_conntrack/libnetfilter_conntrack_tcp.h])
That way you can error-out early in the ./configure build process with a
useful AC_MSG_ERROR() and also wrap the headers in forward.cc according to
squid coding style.
NP: this check is also one that can be cached AC_CHECK_CACHED() I think.
src/cf.data.pre typo:
in the case of TOS preservation require your kernel to be patch with
== s /patch/patched/
src/comm.cc:
* Please use DBG_IMPORTANT instead of '1' and '0' in debugs. Also, IMO
both should indicate WARNING:, unless the one currently logged at level 1
is common and trivial, in which case it should be dropped to level 2+.
The CRITICAL level as much as possible should be restricted to ERROR:
(serious but recoverable or only affecting this users request) and FATAL:
(non-recoverable, Squid about to shutdown).
The IMPORTANT level should be WARNING: and similar. Stuff that can be
administratively fixed etc.
src/forward.cc:
* Wrap system includes in #if HAVE_*
** also they appear to be duplicate included in forward.h and
forward.cc, please only include in the forward.h.
* Please remove the defined(_SQUID_LINUX_) around getNfctMark. We build
on non-linux systems with possible netfilter support (kFreeBSD and hurd).
* our source format requires function result type to be on a separate
line in .cc:
+int
+FwdState::getNfctMark
* also related to the above. The old ZPH TOS _SQUID_LINUX_ wrapper was
only there due to the preserve-miss patch being linux kernel specific. I
think your new preserve for marks not depending on kernel patching is
likely not to need it, so the wrapping there can be re-worked a bit to only
depend on USE_ZPH_QOS and USE_QOS_NFMARK. If you make this change the docs
will also need updating.
* You run the sequence:
+serv_fde_local_conn.InitAddrInfo(addr);
+serv_fde_local_conn.FreeAddrInfo(addr);
+serv_fde_local_conn.GetAddrInfo(addr);
** The GetAddrInfo allocates dynamic memory and needs to be paired with a
FreeAddrInfo() of its own.
** The structures created locally by Init inside addr should be able to
be used throughout the sequence. Unless there is some garbage produced by
getaddrinfo() please remove the Get and move the Free down below where you
are finished with the addr variable and it's content:
+serv_fde_local_conn.FreeAddrInfo(addr);
+} else {
+debugs(17, 1, Failed to allocate new conntrack);
*
* following nfct_query() you can output serv_fde_local_conn directly in
the debugs() argument instead of fiddling with inet_ntop to find its
content. That will handle all the display mess and output src ip:port as a
twinned pair. If the debugs result shows no port it means there was none
set.
* again with the debugs level s/1/DBG_IMPORTANT/ and tagged WARNING: if
it's an error important message. Or bump to level 2+ if its not.
src/ip/QosConfig.cc:
* Ip::Qos::QosConfig::parseConfigLine again with the s/0/DBG_CRITICAL/.
This one I think will screw up TOS settings if it continues so a FATAL:
