Re: [MERGE] Initial netfilter mark patch for comment

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.

HTH,

Alex.
Received on Fri Aug 20 2010 - 18:45:03 MDT