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.
