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

>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
Received on Sat Sep 04 2010 - 14:49:29 MDT