Re: [PATCH] IPv6 split-stack support for ICP, HTCP, SNMP

On 21/03/2012 7:49 p.m., Amos Jeffries wrote:
> On 20/03/2012 5:10 p.m., Alex Rousskov wrote:
>> On 03/15/2012 05:08 AM, Amos Jeffries wrote:
>>> Run-time testing begins this weekend, but it passes build testing and I
>>> fgured it was ready for an audit check. If anyone else wants to assist
>>> testing it that would be welcome. I am proposing this to go into trunk
>>> for squid-3.3.
>>>
>>>
>>> Split-stack support for ICP, HTCP, SNMP
>> FWIW, it would be better if http_port_list renaming and moving was not
>> done while implementing the primary patch changes IMHO. It seems like
>> you did not need to modify http_port_list itself and now it is not clear
>> which of those modifications I can complain about without being told
>> that any further polishing is outside your patch scope. Besides
>> complicating review, combining functional and style changes would also
>> make conflict resolution more difficult/risky for those of us who are
>> modifying port-related code.
>
> Okay. I'm happy to split this in two parts:
> 1) cosmetic: renaming of http_port_list to AnyP::PortCfg. Renaming
> the incoming_* config directives and improving their documentation
> 2) functional: migration of UDP ports to use AnyP::PortCfg
>
>>
>>
>>> + PortList(const char *aProtocol);
>> Please use "explicit" with any single-parameter constructors.
>>
>> I would also s/PortList/PortCfg/ or similar since each object is not a
>> list but a single port. Ideally, we should use std::list<> or another
>> container to group these Port configurations together but that change
>> would probably be outside your project scope.
>
> Okay. Done the rename.
>
> std::list<> would be structural rather than just symbolic so left for
> now. We are a bit constrained by the legacy parser using raw-pointers.
>
>>
>>> - http_port_list *s;
>>> + AnyP::PortList *s;
>>>
>>> - for (s = Config.Sockaddr.http; s != NULL; s =
>>> (http_port_list *) s->next) {
>>> + for (s = Config.Sockaddr.http; s != NULL; s = s->next) {
>> ...
>>> - }
>>> -
>>> - {
>>> -
>>> - http_port_list *s;
>>>
>> It would be better to make "s" local to each of the two "for" loops if
>> possible instead of making it "more global".
>
> Okay. Done.
>
>>
>>> + // - multiple multiple listening ports
>>> + // - multiple multiple transport types on listening ports
>>> + // - multiple multiple and non-ICP probe ports
>> Multiple multiples?
>
> Oops. The tuples are multiplying. Open the airlock.
>
> Rewritten, but do you have an answer to that question about IRCache?
> Can we arrange an upgraded send-announce format that can support (a)
> Squids existing ports and (b) cope with expected future abilities?
> Or are we limited by IRCache legacy systems?
>
> I wont be doing anything immediately, but it would be nice to know and
> plan for.
>
>>> * Restructure ModPoll, ModSelect to use new list of ports instead
>>> of a fixed-size global array.
>>>
>>> NOTE: It appears that HTTPS and HTCP were only checked once per I/O
>>> loop iteration but HTTP and ICP had optimizations to increase their
>>> receive rates.
>> Does your patch change that design? I am not quite sure it was a good
>> idea for Squid to accept more than it had time to process, but
>> regardless of whether the old design was sound, I would rather avoid
>> peformance-sensitive changes as a part of a seemingly
>> performance-neutral patch.
>
> It does not change the design of the I/O magics. Just adds the HTCP
> and HTTPS listening ports alongside the ICP+HTTP listening sockets. So
> that setups using HTCP instead of ICP and/or HTTPS instead of HTTP get
> the same I/O behaviour. A fairly trivial functional change, but
> unrelated to the *_port update. Reverted for now.
>
>
> I'll re-submit part (1) shortly when I can confirm it still builds.
> Are there any other cosmetic changes to http_port_list you would like
> me to include in that patch?
>
> Amos

Some new definition of "shortly". Here it is. Just the cosmetic
alterations.

- renames http_port_list to AnyP::PortCfg
- de-duplicate https_port_list into AnyP::PortCfg
- shuffles related globals and defines into anyp/PortCfg.*
- renames MAXHTTPPORTS to MAXTCPLISTENPORTS to suit its actual coverage
of HTTP and HTTPS ports.
- shuffled config port clone function into a method.
- rename ICP/HTCP/SNMP API functions to consistent *OpenPorts() and
*ClosePorts()

   NP:following applies to incoming_* and *_poll_cnt directives.
- renames *_icp_* to *_udp_*
- renames *_http_* to *_tcp_*
- shuffles duplicated struct SquidConf options into a shared structure
- shuffles related defines into comm/Loops.h
- documents options better

- various other cosmetic syntax tweaks and polish

One bug fix:
   comm_dns_incoming was not being propigated in StatsHist copy/clone.
Now is. I seem to remember mention of something similar being zero
before, but can't find the bug report.

Amos