Re: [MERGE] Initial netfilter mark patch for comment

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

Okay, I'll work on a stripped down fatal() for the time being.

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

I did consider having the config values in a separate Config class, and
I'm happy to change to this, I just thought that it was neater having
them in the same class. It also means that the configuration values do
not need to be public, which I thought was generally a Good Thing.

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

No problem. My initial thoughts are that I should do get*LocalMiss,
get*LocalHit and is*Active (and possibly the new set functions).

> 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 like that; I'll make the changes. I guess there is then no need for
getTosLocalHit() and getNfMarkLocalHit(), as all they do is return the
appropriate private variables, although I could leave them there to keep
everything consistent.

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

No problem. Any idea when the autoconf-refactor will appear in trunk?

Andy
Received on Thu Aug 12 2010 - 07:57:26 MDT