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
Received on Sat Aug 21 2010 - 22:08:17 MDT