Re: [MERGE] Initial netfilter mark patch for comment

My latest revision of the patch for netfilter marking will follow soon.

Before I post it, I will reply to comments on the previous version.

> >> >
> >> > Question number 2: what is stubQosConfig.cc? Does that also need
> >> > updating for this patch?
> >> >
> >>
> >> stub* are cut down set of all non-inline Ip::QosConfig methods and any
> >> globals defined in QosConfig.h. Changes to the API need to be mirrored
> >> there. The functions inside usually call fatal() to alert a wrong
> >> linkage clearly during testing. In this particular case the parse
> >> function will need to be duplicated there.
> >
> > Sorry, I need some clarification on this. I've looked at most of the
> > other stub files, and most of the functions do indeed just return
> > fatal(). However, the existing stubQosConfig.cc is almost identical to
> > QosConfig.cc, with almost all of its code. Shall I change the functions
> > to fatal()?
> >
> > Presumably I should add all the new functions into it (getTosLocalMiss,
> > getNfmarkLocalMiss, getTosLocalHit, getNfmarkLocalHit, getUpstreamTOS,
> > getUpstreamNfMark, isTosActive, isMarkActive, GetNfMarkCallback)?
>
> It does need another looking at and possibly stripping down. IIRC it was
> there from when QosConfig was a member of SquidConfig. I'll run a quick
> check myself now to see what breaks in the current usage if the currently
> defined stubs are stripped down.
>
> You will need to add the appropriate dead-end stubs for the new functions.

I've added stubs for all the non-inline functions. They just all call
fatal(). If I've not got this quite right then please let me know.

>
> >>
> >> In this version the new methods look they should be in the Ip::Qos
> >> namespace.
> >>
> >> * the new clientReplyContext::tosLocalMissValue() and
> >> clientReplyContext::nfmarkLocalMissValue() methods particularly look
> >> like they really should take the hier code as a second parameter. Both
> >> fd and the hier if passed in should be const.
> >
> > I've moved them to Ip::Qos and added the parameters as const.
> >
> >> * the new FwdState getUpstream* methods are in a similar position but
> >> not quite so clear cut. If you can find way to cleanly move them there
> >> great, otherwise it's not so much a bit deal yet.
> >
> > 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.
>
> We are moving the rest of Squid in general towards a model of
> Namespace::TheConfig containing the config data values pulled from
> squid.conf separate from the code which utilizes them.
>
> Please wait for Alex as well on this particular change.
>
> My thoughts on it are these;
>
> I like the moving of functions to IP::Qos including the existing ones.
> Under the coding guidelines the existence of class Qos calls for it to be
> in a file src/ip/Qos.h and .cc.

I have renamed the files from QosConfig to Qos.

As per previous discussions, the functions are now contained in a Qos
namespace, and the configuration (and functions) are kept in a Config
class.

> Several of the functions particularly the is*Active() can be inlined for
> better performance with _SQUID_INLINE_, which calls for a ip/Qos.cci file
> to define them and be conditionally included in the .h or .cc based on #if
> _USE_INLINE_.

I've moved what I consider to be the most appropriate functions inline.
If you disagree with my choices then please tell me.

> Some set*Mark and set*Tos functions woudl round out the API. To take the
> same parameters as the get*() ones, but which do the perform the
> setsockopt. So that paired lines of:
> unsigned int mark = Ip::Qos.getNfmarkLocalMiss(fd,
> http->request->hier);
> comm_set_nfmark(fd,mark);
> become
> Ip::Qos.setNfmarkLocalMiss(fd, http->request->hier);
>
> ... and thus comm.cc does not need anything about the MARK setsockopt.
> Mirroring the way it has nothing to do with the MARK/TOS getsockopt etc.

As per other emails, I have removed get*s and set*s and replaced with
do*s. I did experiment with keeping get*s and set*s and moving the get*s
to private space, but in all honesty, it just complicated the code
without achieving much. I think it's quite tidy now.

> >
> > I've carried out a fair bit of testing on the mark functionality today,
> > and It Works For Me, but I'll be able to try it better in a production
> > server in the coming weeks.
> >
> > Could you let me know if/what else I need to do before acceptance. I
> > believe there are the following:
> >
> > - Confirm and implement stub function requirements
> > - Factor the configure.in changes into Kinkie's autoconf-refactor?
>
> Those are the big ones. Along with getting the namespace change approved.
>
> The reconfigure straightening will change your dependency logic model a
> bit. Specifically such that the absence of --with-netfilter-conntrack
> (implicit / auto-detect case) results in the same auto-detect currently
> only done by explicit "yes" results, but which does not terminate in
> AC_MSG_ERROR.

Done.

The above configure concept would tie in with removing the --enable-qos
option altogether. There's no reason for the QOS code not to be included
that I can see (it has no dependencies, apart from the optional upstream
kernel patch), and with this patch and the isTosActive(), it's enabled
at runtime only when needed anyway. Is there any reason to keep the
option?

I propose just having the --with-netfilter-conntrack option and removing
--enable-qos. The upstream part would remain (as it is now) only enabled
on Linux platforms.

Andy
Received on Sat Sep 04 2010 - 13:37:04 MDT