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

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.

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

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

> + // - multiple multiple listening ports
> + // - multiple multiple transport types on listening ports
> + // - multiple multiple and non-ICP probe ports

Multiple multiples?

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

Thank you,

Alex.
Received on Tue Mar 20 2012 - 04:10:29 MDT