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
