Re: [PATCΗ] Quoted values in squid.conf

On 05/26/2013 06:30 AM, Amos Jeffries wrote:
> On 24/05/2013 7:38 a.m., Tsantilas Christos wrote:
>> This patch :
>>
>> - adds support for quoted values in the entire squid.conf
>>
>> - warn about or prohibit values that can no longer be interpreted as
>> either quoted strings or simple tokens
>>
>> - support file:"path/to/file.name" syntax to load external configuration
>> files
>>
>> - support macros in "double quoted" values
>>
>> - support 'single quoted' values that do not expand macros
>>
>> - replaces the strtok() calls with calls to the new
>> ConfigParser::NextToken()
>>
>> - modify strtokFile to use new ConfigParser::NextToken()
>>
>> - Add the new configuration_includes_quoted_values configuration option,
>> to control the squid parser behaviour. If set to on Squid will recognize
>> each "quoted string" after a configuration directive as a single
>> parameter.
>>
>>
>> This is a Measurement Factory project
>
> This is a big patch with multiple features combined into one massive
> submission. So I am going to audit this in stages and lets sort out each
> stage before moving on to issues wth the others.
>
> Firstly, Design simplicity:
>
> I do not believe we need to present such a wide range of potential
> quoting and escaping mechanisms for squid.conf. We need to pick one
> style which will be familiar to our administrators and ignore the other
> styles. Double-quoting with \-escaping of " characters for strings is
> very well known and the type of encoding most often requested.
> IMO, we should drop the '-quoted string handling from this. It is the
> first step on a slipery-slope toward arbitrary quoting styles and "why
> not also add {-quoted strings or [-quoted strings?". Those are becoming
> equally popular as people become familiar with YAML/SAS/JSON syntax.

I am not seeing any complexity in '-macros. They can be used to simplify
configuration avoiding characters escaping.

The [-quoted and {-quoted strings will confuse system administrators.
The '['/']' and '{'/'}' characters are mostly used to quote objects not
strings, so they have completely different purpose.

The '-quoted and "-quoted strings used from the beginnings of computer
science...

> Also, your design for MacroUser requires parser components to
> register/unregister their Format class before parsing each "-quoted
> string which might include macros. Which makes toggling support for it
> on the quoting delimiter redundant and nothing more than extra confusion
> for users.

Not all of the configuration options support %macros. Moreover the
options which support %macros may support different set of %macros. For
example the logformat options and the external_acl_type.

The MacroUser class is an easy way to enable %macros only for options
support them, and warn system administrators for options which does not
support it.

>
> ==> Please drop '-quoted strings.
>
>
> Secondly, multiple features in one patch:
>
> The MacroUser system you are adding here is combining some, but not
> enough, of the next step in the libformat project. Thank you for doing
> that, but I do not think it appropriate to merge it into this whitespace
> and quote handling patch. It was planned to be done as a followup once
> the parser was updated in a way such as what this patch is doing.

The MacroUser just only check if %macros supported or not, and which
macros supported. Also the user of this class may select to not do any
check at all, just use it to enable macros in the next parsed token.
I do not think that it is related to libformat ...

> Also by naming the class MacroUser you are adding a fourth term (macro)
> to describe these %things, we already have %-tokens, %-tags, %-format,
> and %-codes in use today. I think we should start to redux those terms
> usage rather than expanding with another variation on the theme.

The naming is not something important, we can always rename this class,
before commit the patch or later when we find a better name ...

>
> ==> Please separate the MacroUser functionality out into a followup patch.
>
>
> Amos
>
>
Received on Sun May 26 2013 - 14:07:36 MDT