Re: [PATCH] update port protocol parameter

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

Maybe I should explain.
  * Adding the operator claims collision with a default
ostream::operator<<(unsigned int).
  * ProtocolType being an enum *always* require manipulations to display
its string equivalent.
  * any given call to the parse function will only hit one of these
debugs() AND may have a different transport.protocol value, even if the
PortCfg is the same.
  * the URLScheme().const_str() *is* the wrapper keeping the repeated
manipulation actions away from the above code lines.

I can try adding an operator << to the URLScheme object to get rid of
the const_str() part, but the rest is needless optimization.

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

Okay, fine by me. self_destruct instead of debugs() removes a dependency
issues I hit when building this on clang. So I'll go along with that.

Amos
Received on Mon Jul 22 2013 - 01:10:31 MDT