Re: [PATCH] update port protocol parameter

On 07/21/2013 07:10 PM, Amos Jeffries wrote:
> 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.

> * ProtocolType being an enum *always* require manipulations to display
> its string equivalent.

One more reason to add a simple function or method to encapsulate these
repeated manipulations.

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

Not sure why this is relevant. The simple function or method will depend
on s->transport.protocol value, of course.

> * the URLScheme().const_str() *is* the wrapper keeping the repeated
> manipulation actions away from the above code lines.

There are obviously repeated manipulations besides what URLScheme() and
const_str() do (like the fact that we have to combine those two together).

This code duplication is minor, of course, so if you really do not feel
like removing it, so be it.

Alex.

> 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 - 02:01:33 MDT