Re: [PATCH] update port protocol parameter

On 07/06/2013 08:22 AM, Amos Jeffries wrote:

> This patch updates the *_port directives protocol= parameter to accept
> to use AnyP::ProtocolVersion internal storage instead of opaque string
> text.

The above says "instead" but the patch leaves the old string-based
constructor. Do we still need that old one? If yes, perhaps it would be
better to remove the new constructor (because it is used only once
AFAICT) and call setTransport() instead? Having two constructors is a
pain because one has to keep all the initializations in sync...

> PortCfg(const char *aProtocol);
> + PortCfg(const AnyP::ProtocolVersion &aProtocol);

Both constructors are missing an "explicit" keyword.

> + /**
> + * Set this ports transport type from a string representation.

s/ports/port/ or simply remove "this ports ".

> + * Supports: HTTP, HTTP/1.1, HTTPS, HTTPS/1.1.

Please clarify why HTTP/1.0 is not supported even though Squid does
accepts 1.0 (and even 0.9?) requests. A brief comment hint should
suffice IMHO.

> - http->getConn()->port->protocol,
> + URLScheme(conn->port->transport.protocol).const_str(),

I presume http->getConn() is the same as "conn" here, but let's not make
that partial change in this patch. If you want to replace all
http->getConn() calls in that context with "conn", please commit that
change separately.

> + debugs(3, DBG_CRITICAL, "FATAL: " << URLScheme(s->transport.protocol).const_str() << "_port: missing ']' on IPv6 address: " << token);
> + debugs(3, DBG_CRITICAL, "FATAL: " << URLScheme(s->transport.protocol).const_str() << "_port: missing Port in: " << token);
> + debugs(3, DBG_CRITICAL, "FATAL: " << URLScheme(s->transport.protocol).const_str() << "_port: IPv6 is not available.");
> + debugs(3, 3, URLScheme(s->transport.protocol).const_str() << "_port: found Listen on Port: " << port);
> + debugs(3, DBG_CRITICAL, "FATAL: " << URLScheme(s->transport.protocol).const_str() << "_port: missing Port: " << token);
> + debugs(3, DBG_CRITICAL, "FATAL: " << URLScheme(s->transport.protocol).const_str() << "_port: Port cannot be 0: " <<
...

Can you use ostream << printing of ProtocolType instead of repeating
these manipulations?

> - debugs(33, DBG_IMPORTANT, "WARNING: CONNECT method received on " << csd->port->protocol << " Accelerator port " << csd->port->s.port() );
> + debugs(33, DBG_IMPORTANT, "WARNING: CONNECT method received on " << csd->port->transport.protocol << " Accelerator port " << csd->port->s.port());

It is strange that the same manipulation is not done here. If this is
not intentional, then please be consistent.

> This both limits the possible parameter values to one of HTTP, HTTP/1.1,
> HTTPS, or HTTPS/1.1

> + * Unknown transport type representations are ignored.

Should not Squid reject invalid configurations that use protocols that
Squid does not understand and [no longer] uses? What will happen if an
admin makes a one-key-off typo and says protocol=httpd in squid.conf?
Will Squid behavior in that case change after your patch?

All of the above can be addressed without additional reviews IMO.

Thank you,

Alex.
Received on Thu Jul 18 2013 - 01:36:46 MDT