Andrew Beverley wrote:
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.
Yes. We've thrown various ideas about how to do a live zero-downtime
reconfigure around. TheConfig were working towards those. That set are
in the gap really. I suppose it depends on it matters if one of the
marks is set at the top of a big config and another at the end.
The way you have them as privates for now should be okay. Worst case we
abstract to a public again later as needed.
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.
Hmm, if they are fully unused then it makes more sense to drop and call
the set*() methods do*().
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?
Um two days ago. :)
Sign of a good clean merge: nobody notices.
Amos
--
Please be using
Current Stable Squid 2.7.STABLE9 or 3.1.6
Beta testers wanted for 3.2.0.1
