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 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

Thank you,

Alex.
Received on Sat Aug 14 2010 - 00:19:43 MDT