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

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

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

==> Please separate the MacroUser functionality out into a followup patch.

Amos
Received on Sun May 26 2013 - 03:30:10 MDT