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