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 0xFFFFFFFF?
> Please investigate. I do not know the answer, but I recall seeing such
> suffixes elsewhere:
> http://www.google.com/search?q=0xFFFFFFFF+vs+0xFFFFFFFFL

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