Re: [MERGE] Initial netfilter mark patch for comment

On Wed, 11 Aug 2010 22:25:45 +0100, Andrew Beverley <andy_at_andybev.com>
wrote:
> Updated patch attached; comments below.
>
>> > If we can move to strtoul, I would like to change 'tos' to char
>> > throughout. Currently it is possible to set it to invalid values in
>> > squid.conf, which then cause problems with dumpConfigLine.
>> >
>> > 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.

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

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.

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

Amos
Received on Thu Aug 12 2010 - 04:44:22 MDT