Re: [PATCH] update port protocol parameter

On 07/17/2013 08:16 PM, Amos Jeffries wrote:
> On 18/07/2013 1:36 p.m., Alex Rousskov wrote:
>> On 07/06/2013 08:22 AM, Amos Jeffries wrote:
>>> + 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 ...

Then please add a simple function or method to encapsulate these
repeated manipulations.

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

As always, I think Squid should reject the invalid config instead of
generating traffic that is likely to differ from what the admin wants.

Cheers,

Alex.
Received on Thu Jul 18 2013 - 04:12:51 MDT