Re: [PATCH] update port protocol parameter

On 18/07/2013 1:36 p.m., Alex Rousskov wrote:
> 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?

Unfortunately no ...

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

... the URL-scheme and the transport protocol string representations are
different. Un particular they are different cases, and in some protocols
different scheme name from transport name (protocol="HTTP-draft-04" ->
URLScheme="http"), although we do not have that (yet) in Squid this
patch is part of the groundwork for those possibilities.

The earlier URLScheme(*).const_str() manipulations are for outputting
the lowercase "http_port" / "https_port" texts. Whereas this latter one
is for outputting the uppercase "HTTP Accelerator port" text or equivalent.

>
>> 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?

Yes. Previously Squid would silently accept it and general URLs with
"httpd://" scheme names to upstream proxies, resulting in rejected
traffic and some problems outside of the broken proxy.

After patch Squid will report the WARNING at level 1 (and -k parse) then
use the previous (usually default) scheme for that port. Ensuring that
all traffic generated from the proxy is at least a valid supported
protocol, instead of causing problems for the upstream network(s).

Thank you.

Amos
Received on Thu Jul 18 2013 - 02:17:03 MDT