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

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
Received on Wed Mar 21 2012 - 06:50:00 MDT