Re: [MERGE] Initial netfilter mark patch for comment

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